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

chore: Dynamically create test jars for JarCommandTest #4210

Merged
merged 3 commits into from
Mar 14, 2024

Conversation

meltsufin
Copy link
Contributor

Follow-up to #4205.

@meltsufin meltsufin requested review from suztomo and mpeddada1 March 7, 2024 16:17
Comment on lines 352 to 354
JavaCompiler.CompilationTask task = compiler.getTask(null, fileManager, null, options, null, compilationUnits);
boolean success = task.call();
assertThat(success).isTrue();
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we use an if-else statement that proceeds with the jar building stage below if task.call() is true otherwise logs an error message if the compilation fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we just silently log the message, then we might not notice the failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry, good point!

[Optional] Alternatively, to have a clear separation between the "arrange", "act" and "assert" blocks in our tests - consider using Preconditions.checkState(task.call(), "<error message>");.

@mpeddada1
Copy link
Contributor

Thanks for addressing this @meltsufin! Running ./gradlew clean goJF should fix the formatting issues the CI checks are showing.

@meltsufin meltsufin requested a review from mpeddada1 March 13, 2024 23:24
@meltsufin meltsufin merged commit a8fd424 into master Mar 14, 2024
11 checks passed
@meltsufin meltsufin deleted the create-test-jars branch March 14, 2024 14:57
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.

2 participants