Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test as dependency to daffodil-test-integration #1333

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

olabusayoT
Copy link
Contributor

  • update failing udf test

DAFFODIL-2933

Copy link
Member

@stevedlawrence stevedlawrence left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, thought it's not clear to me why the test needed to be updated


runCLI(args"-v parse -s $schema -r user_func1") { cli =>
runCLI(args"-v parse -s $schema -r user_func1", classpath) { cli =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was it intentional to not add the udfClasspath for this tests since this test is about not having any UDF's? Probably worth adding a comment to make that clear.

Though, I would have expected the CLI to succeed with UDF's on the classpath so maybe I'm missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think the addition of the daffodil-test as a dependency caused the internal classpath to have the UDFs available which caused the test to fail with the changes, so we had to explicitly set the the udfClasspath to be empty for it to pass.

Before we didn't need to because the daffodil-test (which contains the UDF dependencies was missing)

I can update with a comment for clarity though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the integration tests use the same classpath used for normal tests with SBT/IDE. These tests fork witha completely custom classpath, so changes to the SBT config shouldn't affect, I think...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, and that's the problem, this test doesn't fork so it will use the normal classpath. If you specify classpath then it will fork, which is way that fixed it. Probably all the tests in here want to either specify fork = true or ensure that something is defined that causes forking, like a non-empty classpath. I think in this test case we explicitly don't want a classpath to ensure no UDFs are loaded, so we should set fork = true

Copy link
Contributor

@jadams-tresys jadams-tresys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

- currently, we only ensure the daffodil-cli and daffodil-uds jars are built, so when some of our tests fork they can correctly reference the jars in the classpaths we set up for them. Many tests reference daffodil-test, so when you try to run daffodil-test-integration/test from a clean repo, some tests fail so we have to ensure daffodil-test is built and available as well.
- add forks to cli project

DAFFODIL-2933
@olabusayoT olabusayoT merged commit b5d6914 into apache:main Oct 11, 2024
12 checks passed
@olabusayoT olabusayoT deleted the daf-2933-addTestAsDependency branch October 11, 2024 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants