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
Fix most compiler warnings #565
Conversation
src/test/java/org/junitpioneer/jupiter/CartesianProductTestExtensionTests.java
Show resolved
Hide resolved
@@ -92,8 +92,6 @@ void abortedIssueTestCaseCreated() { | |||
assertAll(() -> assertThat(issueTestSuite.issueId()).isEqualTo("#123"), | |||
() -> assertThat(issueTestSuite.tests().size()).isEqualTo(1)); | |||
|
|||
IssueTestCase testCase = issueTestSuite.tests().get(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable was unused and assertion below seems to cover that already.
src/test/java/org/junitpioneer/testkit/assertion/suite/TestSuiteFailureAssert.java
Outdated
Show resolved
Hide resolved
I don't know what to think about this, my gut reaction is that it's not really necessary. |
Why not? The current amount of warnings makes it more difficult to find newly introduced issues, and some of the warnings (at least the unused variable one in |
Personally, I am fine with these changes but I'd like someone with more experience to okay them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Marcono1234, I really like the PR. Fewer warnings are always a good thing in my book and I'd like to take this even further and see whether we can get it to zero warnings, so we can turn them into errors with the compiler flag -Werror
. More on that in another comment.
src/main/java/org/junitpioneer/jupiter/AbstractEntryBasedExtension.java
Outdated
Show resolved
Hide resolved
src/main/java/org/junitpioneer/jupiter/params/RangeSourceArgumentsProvider.java
Outdated
Show resolved
Hide resolved
src/main/java/org/junitpioneer/vintage/ExpectedExceptionExtension.java
Outdated
Show resolved
Hide resolved
src/test/java/org/junitpioneer/jupiter/CartesianProductTestExtensionTests.java
Show resolved
Hide resolved
src/test/java/org/junitpioneer/jupiter/CartesianProductTestExtensionTests.java
Outdated
Show resolved
Hide resolved
src/test/java/org/junitpioneer/jupiter/CartesianProductTestExtensionTests.java
Show resolved
Hide resolved
src/test/java/org/junitpioneer/jupiter/CartesianProductTestNameFormatterTests.java
Outdated
Show resolved
Hide resolved
src/test/java/org/junitpioneer/vintage/TestIntegrationTests.java
Outdated
Show resolved
Hide resolved
src/test/java/org/junitpioneer/testkit/assertion/suite/TestSuiteFailureAssert.java
Outdated
Show resolved
Hide resolved
It can't. In Java 9+ it ceases to be a warning, but as long as we're on 8, we'll have to stick with it. What works, paradoxically, is to replace each use of the class with its fully qualified name (thus allowing us to remove the import) and then suppress the resulting warnings in the methods.
I didn't look into that (and don't have time to do it right now), but can we switch to an alternative API? If that requires a newer Jupiter version, that's no problem - there's a PR floating around somewhere that ups it to the most recent release (5.8.2, at the time of writing). If that helps, we can update in this PR and switch to the new API to get rid of the deprecation warning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the vararg parameter was unnecessary for the withExceptionInstanceOf
, let's change it for these as well, since they are also always invoked with a single argument.
src/main/java/org/junitpioneer/internal/PioneerAnnotationUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/junitpioneer/internal/PioneerAnnotationUtils.java
Outdated
Show resolved
Hide resolved
This and the other one are actually fine (and don't cause any warnings) because For Do you want this to be changed nonetheless? |
Co-authored-by: Nicolai Parlog <nicolai@nipafx.dev>
Have done this. Now the main sources compile without warnings.
The issue is that JUnit Pioneer currently uses the version matrix It would be good if one of you check this as well (when you have time) because you can better judge what the correct behavior should be. |
…into compiler-warnings
Looks good. Now we need to fix the merge conflicts - but first there's #603 to merge because it touches classes that are edited here. |
Unfortunately, we can't merge this as is because there are three spurious warnings/errors:
Looks like we need to update our build to JDK 11 soon. |
src/test/java/org/junitpioneer/jupiter/issue/IssueExtensionIntegrationTests.java
Outdated
Show resolved
Hide resolved
…into compiler-warnings
…nit-pioneer into compiler-warnings
Not |
Thank you for your assitance with this, or rather for taking over this pull request. Though you apparently removed GitHub's |
* <p>The dependencies on Jupiter modules could be marked as <code>transitive</code> but that would | ||
* allow users who depend on this module to not `require` org.junit.*, which would be backwards. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nipafx, this comment is a bit late but should this use {@code ...}
?
* <p>The dependencies on Jupiter modules could be marked as <code>transitive</code> but that would | |
* allow users who depend on this module to not `require` org.junit.*, which would be backwards. | |
* <p>The dependencies on Jupiter modules could be marked as {@code transitive} but that would | |
* allow users who depend on this module to not {@code require} org.junit.*, which would be backwards. |
Proposed commit message:
In the main sources there is still one warning left in
RangeSourceArgumentsProvider
for the import of the deprecatedCartesianAnnotationConsumer
. However, apparently that warning for the import itself cannot be suppressed.In the test sources there are warnings left for usage of
TestPlan.add(TestIdentifier)
inIssueExtensionExecutionListenerTests
. I have not suppressed them because eventually it might be necessary to find a replacement for that. Currently these calls are only made against a mock instance, so it is most likely not an issue that the method implementation was changed recently to always throw an exception (junit-team/junit5@8b910ec).PR checklist
The following checklist shall help the PR's author, the reviewers and maintainers to ensure the quality of this project.
It is based on our contributors guidelines, especially the "writing code" section.
It shall help to check for completion of the listed points.
If a point does not apply to the given PR's changes, the corresponding entry can be simply marked as done.
Documentation (general)
.adoc
file in thedocs
folder, e.g.docs/report-entries.adoc
.adoc
files)Documentation (new extension)
docs/docs-nav.yml
navigation has an entry for the new extensionpackage-info.java
contains information about the new extensionCode
Contributing
README.md
mentions the new contribution (real name optional)I hereby agree to the terms of the JUnit Pioneer Contributor License Agreement.