Fix most compiler warnings #565

Marcono1234 · 2021-12-25T17:28:28Z

Proposed commit message:

Fix most compiler warnings (#565)

Fixes most compiler warnings about unchecked casts and raw types.

PR:  #565

In the main sources there is still one warning left in RangeSourceArgumentsProvider for the import of the deprecated CartesianAnnotationConsumer. 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) in IssueExtensionExecutionListenerTests. 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)

There is documentation (Javadoc and site documentation; added or updated)
There is implementation information to describe why a non-obvious source code / solution got implemented
Site documentation has its own .adoc file in the docs folder, e.g. docs/report-entries.adoc
Only one sentence per line (especially in .adoc files)
Javadoc uses formal style, while sites documentation may use informal style

Documentation (new extension)

The docs/docs-nav.yml navigation has an entry for the new extension
The package-info.java contains information about the new extension

Code

Code adheres to code style, naming conventions etc.
Successful tests cover all changes
There are checks which validate correct / false usage / configuration of a functionality and there are tests to verify those checks
Tests use AssertJ or our own PioneerAssert (which are based on AssertJ)

Contributing

A prepared commit message exists
The list of contributions inside README.md mentions the new contribution (real name optional)

I hereby agree to the terms of the JUnit Pioneer Contributor License Agreement.

src/test/java/org/junitpioneer/jupiter/CartesianProductTestExtensionTests.java

Marcono1234 · 2021-12-25T17:32:32Z

src/test/java/org/junitpioneer/jupiter/issue/IssueExtensionExecutionListenerTests.java

@@ -92,8 +92,6 @@ void abortedIssueTestCaseCreated() {
 		assertAll(() -> assertThat(issueTestSuite.issueId()).isEqualTo("#123"),
 			() -> assertThat(issueTestSuite.tests().size()).isEqualTo(1));

-		IssueTestCase testCase = issueTestSuite.tests().get(0);


Variable was unused and assertion below seems to cover that already.

src/test/java/org/junitpioneer/testkit/assertion/PioneerAssert.java

src/test/java/org/junitpioneer/testkit/assertion/suite/TestSuiteFailureAssert.java

Michael1993 · 2021-12-26T15:52:09Z

I don't know what to think about this, my gut reaction is that it's not really necessary.

Marcono1234 · 2021-12-26T17:32:44Z

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 IssueExtensionExecutionListenerTests) are legit and should be fixed.
For example CartesianProductTestExtensionTests has dozens of warnings due to usage of the deprecated class. This makes it rather distracting (at least in Eclipse). Of course you could edit the IDE settings to ignore certain warning types, but ideally these false positive warnings should be suppressed so that javac is not creating warnings for it either.

Michael1993 · 2022-01-06T15:16:23Z

Personally, I am fine with these changes but I'd like someone with more experience to okay them.

nipafx

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

src/main/java/org/junitpioneer/jupiter/params/RangeSourceArgumentsProvider.java

src/main/java/org/junitpioneer/vintage/ExpectedExceptionExtension.java

src/main/java/org/junitpioneer/vintage/TimeoutExtension.java

src/test/java/org/junitpioneer/jupiter/CartesianProductTestExtensionTests.java

src/test/java/org/junitpioneer/jupiter/CartesianProductTestNameFormatterTests.java

src/test/java/org/junitpioneer/vintage/TestIntegrationTests.java

src/test/java/org/junitpioneer/testkit/assertion/suite/TestSuiteFailureAssert.java

nipafx · 2022-01-06T18:26:13Z

In the main sources there is still one warning left in RangeSourceArgumentsProvider for the import of the deprecated CartesianAnnotationConsumer. However, apparently that warning for the import itself cannot be suppressed.

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.

In the test sources there are warnings left for usage of TestPlan.add(TestIdentifier) in IssueExtensionExecutionListenerTests.

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.

Michael1993

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

Marcono1234 · 2022-01-26T13:42:19Z

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.

This and the other one are actually fine (and don't cause any warnings) because @SafeVarargs can be used on them, and this seems to be a proper use case for this annotation here, because the array is not modified.

For withExceptionInstancesOf the issue was that @SafeVarargs could not be used because it is not allowed on methods which can be overridden.

Do you want this to be changed nonetheless?

Co-authored-by: Nicolai Parlog <nicolai@nipafx.dev>

…ings

Marcono1234 · 2022-01-26T14:17:38Z

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.

Have done this. Now the main sources compile without warnings.

In the test sources there are warnings left for usage of TestPlan.add(TestIdentifier) in IssueExtensionExecutionListenerTests.

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.

The issue is that TestPlan is an internal JUnit class and it changed in r5.8.0-RC1, see commit (as indicated by this comment in IssueExtensionExecutionListenerTests). I assume that is why the factory method cannot be used, and why the deprecated add method is used. But maybe these add calls are redundant, because TestPlan is already mocked, so the add calls don't seem to do anything. The tests still pass without them.

JUnit Pioneer currently uses the version matrix [ '5.7.2', '5.8.2' ] so it is not possible to use the static factory method without causing issues for either of these versions.

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

nipafx · 2022-03-10T20:58:45Z

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.

nipafx · 2022-03-10T21:50:14Z

Unfortunately, we can't merge this as is because there are three spurious warnings/errors:

the use of NullEnum.class as attribute default in CartesianEnumSource (both deprecated) creates a warning that can't be suppressed - works on JDK 9+ without suppression
there are two rawtype warnings that can probably be suppressed - works on JDK 10+ without suppression

Looks like we need to update our build to JDK 11 soon.

src/test/java/org/junitpioneer/jupiter/issue/IssueExtensionIntegrationTests.java

…into compiler-warnings

…nit-pioneer into compiler-warnings

build.gradle.kts

Bukama · 2022-03-27T15:35:49Z

Not merge-ready as there are compilation errors of test classes

Marcono1234 · 2022-04-10T16:04:10Z

Thank you for your assitance with this, or rather for taking over this pull request. Though you apparently removed GitHub's Co-authored-by from the commit message so the complete commit in main is now attributed to me 😅

Marcono1234 · 2022-10-02T12:09:37Z

src/main/module/module-info.java

+ * <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.


@nipafx, this comment is a bit late but should this use {@code ...}?

Suggested change

* <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.

Fix most compiler warnings

b163e77

Marcono1234 commented Dec 25, 2021

View reviewed changes

Bukama requested review from beatngu13 and aepfli December 30, 2021 17:04

nipafx requested changes Jan 6, 2022

View reviewed changes

Michael1993 requested changes Jan 23, 2022

View reviewed changes

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

Marcono1234 and others added 3 commits January 26, 2022 14:43

Improve deprecation suppression comments

3269bc2

Co-authored-by: Nicolai Parlog <nicolai@nipafx.dev>

Address review feedback

91649fd

Merge remote-tracking branch 'remotes/origin/main' into compiler-warn…

13cf658

…ings

Michael1993 and others added 7 commits March 3, 2022 15:11

Merge branch 'main' of https://github.com/junit-pioneer/junit-pioneer …

16374c3

…into compiler-warnings

Remove varargs/generics combinations

1af3ba3

Prevent unintended fall-through

33d5fff

Remove calls to deprecated methods ON A MOCK

d2eec49

Rename test case class

3971389

Turn all compiler warnings into errors

8ea546c

Document that warnings are treated as errors

1813150

Nicolai Parlog added 2 commits March 10, 2022 22:21

Merge branch 'main' into compiler-warnings

d5cf347

Suppress two new varargs warnings

887c58e

nipafx added the full-build Triggers full build suite on PR label Mar 10, 2022

Replace @SuppressWarnings(deprecation) with @Deprecated

4a5aca3

nipafx approved these changes Mar 10, 2022

View reviewed changes

Michael1993 approved these changes Mar 10, 2022

View reviewed changes

nipafx mentioned this pull request Mar 10, 2022

Update JDK used for builds #606

Closed

Marcono1234 commented Mar 10, 2022

View reviewed changes

src/test/java/org/junitpioneer/jupiter/issue/IssueExtensionIntegrationTests.java Outdated Show resolved Hide resolved

Michael1993 added 4 commits March 17, 2022 19:10

Merge branch 'main' of https://github.com/junit-pioneer/junit-pioneer …

858d7bf

…into compiler-warnings

Merge branch 'compiler-warnings' of https://github.com/Marcono1234/ju…

3287331

…nit-pioneer into compiler-warnings

Requires transitive

3f63820

Warnings ignored in modular build.

bdb9fd8

Marcono1234 commented Mar 17, 2022

View reviewed changes

build.gradle.kts Outdated Show resolved Hide resolved

build.gradle.kts Outdated Show resolved Hide resolved

Nicolai Parlog added 3 commits March 24, 2022 21:42

Ignore 'exports' warnings and document why

5425bc5

Small fixes

a76d1f7

Fix javac 17 warnings

0966e29

nipafx added full-build Triggers full build suite on PR and removed full-build Triggers full build suite on PR labels Mar 24, 2022

Suppress security manager deprecation warnings

8563496

nipafx merged commit a61ca4b into junit-pioneer:main Apr 10, 2022

Marcono1234 deleted the compiler-warnings branch April 10, 2022 16:04

beatngu13 mentioned this pull request Apr 11, 2022

Improve changelog #622

Open

Marcono1234 commented Oct 2, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix most compiler warnings #565

Fix most compiler warnings #565

Marcono1234 commented Dec 25, 2021 •

edited

Marcono1234 Dec 25, 2021

Michael1993 commented Dec 26, 2021

Marcono1234 commented Dec 26, 2021

Michael1993 commented Jan 6, 2022

nipafx left a comment

nipafx commented Jan 6, 2022

Michael1993 left a comment

Marcono1234 commented Jan 26, 2022

Marcono1234 commented Jan 26, 2022

nipafx commented Mar 10, 2022

nipafx commented Mar 10, 2022

Bukama commented Mar 27, 2022

Marcono1234 commented Apr 10, 2022 •

edited

Marcono1234 Oct 2, 2022 •

edited

		* <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.

Fix most compiler warnings #565

Fix most compiler warnings #565

Conversation

Marcono1234 commented Dec 25, 2021 • edited

Marcono1234 Dec 25, 2021

Choose a reason for hiding this comment

Michael1993 commented Dec 26, 2021

Marcono1234 commented Dec 26, 2021

Michael1993 commented Jan 6, 2022

nipafx left a comment

Choose a reason for hiding this comment

nipafx commented Jan 6, 2022

Michael1993 left a comment

Choose a reason for hiding this comment

Marcono1234 commented Jan 26, 2022

Marcono1234 commented Jan 26, 2022

nipafx commented Mar 10, 2022

nipafx commented Mar 10, 2022

Bukama commented Mar 27, 2022

Marcono1234 commented Apr 10, 2022 • edited

Marcono1234 Oct 2, 2022 • edited

Choose a reason for hiding this comment

Marcono1234 commented Dec 25, 2021 •

edited

Marcono1234 commented Apr 10, 2022 •

edited

Marcono1234 Oct 2, 2022 •

edited