Migrate to full junit 5 and add some associated code expected to support junit 5 - fixes #2596 #2605

hazendaz · 2023-10-05T02:55:03Z

gtoison · 2023-10-05T05:56:20Z

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue2120Test.java


    @Test
-    public void test() {
+    @DisabledOnJre(JRE.JAVA_11)


Isn't it different compared to assumeThat(javaVersion, is(greaterThanOrEqualTo(11))); ?

See the files used under test, they only work on jdk 17 of the build. So yes its meant to skip. The important thing is that the build as it is was using toolchains and not adhering to what it had stated. While some of these were jdk 14 added or what not, it only matters what we build against as we store those in singular packages to not get out of control. So anything noted with 14 is simply able to build at 17.

You would not see the specific issue on this PR, it shows when jdk 11 is then used to build since all the files used for test were flagged only working with higher jdk.

The test was assuming assumeThat(javaVersion, is(greaterThanOrEqualTo(11))); so it ran on JDK 11 but now its not: @DisabledOnJre({ JRE.JAVA_8, JRE.JAVA_11 })

I think it is disabled on Java 11, because the test sample files are in the java17 directory. So the containing directory and the assumptions about the Java versions are not consistent. @hazendaz Did you check this test, does it fail on Java 11?

Yes to both here. Due to the folder being used, I added the flag in that way to reflect that. Will review all that though and squash the tests back into normal folders and use junit flags for same as necessary. That will clean up the build a bit. Not sure we should prolong this PR though to do that work. At the moment, these are all flagged to generally match the folders we claim the tests to be in per the jdk we use to build. Possibly this is a follow up PR that addresses properly each introduced jdk usage directly and just cleans up the gradle build scripts for same. Otherwise I fear this continues to grow and makes it harder to review than it already is.

Let me know what makes most sense there.

Looks good to me, thank you!

gtoison · 2023-10-05T06:00:36Z

...psePlugin-junit/src/test/java/de/tobject/findbugs/reporter/ThrottledProgressMonitorTest.java

@@ -5,7 +5,7 @@
 import static org.mockito.Mockito.verify;

 import org.eclipse.core.runtime.IProgressMonitor;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;

 public class ThrottledProgressMonitorTest {


The public qualifier was removed from most classes since it's not required by JUnit 5 but it remained on that one, I guess it's fine

Did this all mostly by hand, must have missed that one, fixing and will send up patch.

Ok I had done that against only the larger package so there were a good number more to cleanup. Rebuilding now and will push in a few.

gtoison · 2023-10-05T07:03:36Z

Fixing the Sonar setup would be great too, the code coverage was broken and is zero. That PR is also not showing up in https://sonarcloud.io/project/pull_requests_list?id=com.github.spotbugs.spotbugs
So it makes it harder to check the impact of that PR

JuditKnoll · 2023-10-05T07:02:14Z

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/ba/Issue1254Test.java

-        assumeFalse(System.getProperty("java.specification.version").startsWith("1."));
-        int javaVersion = Integer.parseInt(System.getProperty("java.specification.version"));
-        assumeThat(javaVersion, is(greaterThanOrEqualTo(11)));
-    }


Are you sure that we don't need to check the Java version for this test?

We only build with jdk 17 currently, after my other pr we will be able to build jdk 11, 17, and 20. So yes this was not needed as we cannot build with anything lower than jdk 11.

I think we should keep building the test case samples on various JDK, including 8, so we can check that we can still analyse older bitecode.

This code is about the JRE, which runs the test. However, the ./gradlew build command both builds and runs the tests in the project, there may be a chance that someone only builds the project with one JDK, and runs it on another system with different Java version.
Also, I agree with @gtoison, the files to analyze could have been built with an entirely different Java version, so the test case samples should represent this. It's better if we have the tests fail as an early warning system, then receive new issues about a problem much later.

jdk 8 is likely to be hard pass as there is too much involved reverting us backwards to support it. I'll look at that on the separate PR I have.

On this tough, I went through and branded all now as indicated regardless of what we are using so it matches the build expectations if jdk 8 were to be working.

JuditKnoll · 2023-10-05T07:31:31Z

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/ba/Issue408Test.java

-        assumeFalse(System.getProperty("java.specification.version").startsWith("1."));
-        int javaVersion = Integer.parseInt(System.getProperty("java.specification.version"));
-        assumeThat(javaVersion, is(greaterThanOrEqualTo(11)));
-    }


Are you sure that we don't need to check the Java version for this test?

We only build with jdk 17 currently, after my other pr we will be able to build jdk 11, 17, and 20. So yes this was not needed as we cannot build with anything lower than jdk 11.

JuditKnoll · 2023-10-05T07:50:53Z

...st/java/edu/umd/cs/findbugs/detect/IncorrectSelfComparisonInstanceOfPatternMatchingTest.java

-        int javaVersion = Integer.parseInt(System.getProperty("java.specification.version"));
-        assumeThat(javaVersion, is(greaterThanOrEqualTo(14)));
-
+    @DisabledOnJre(JRE.JAVA_11)


In the original version Java 14 and later is assumed, but here you disable on Java 11.

Yes because we only build (in future pr) with jdk 11, 17, or 21. Thus we don't need to mention any other jdks. So we only need to exclude it against jdk 11 builds.

We could add other JDKs later to build, so let's not loose this info, it would be much harder later to figure it out. Also, it's about the JRE, not the JDK.

JuditKnoll · 2023-10-05T07:52:15Z

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue1771Test.java

-        assumeFalse(System.getProperty("java.specification.version").startsWith("1."));
-        int javaVersion = Integer.parseInt(System.getProperty("java.specification.version"));
-        assumeThat(javaVersion, is(greaterThanOrEqualTo(11)));
-    }


Are you sure that we don't need to check the Java version for this test?

We only build with jdk 17 currently, after my other pr we will be able to build jdk 11, 17, and 20. So yes this was not needed as we cannot build with anything lower than jdk 11.

JuditKnoll · 2023-10-05T07:54:38Z

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue2120Test.java


    @Test
-    public void test() {
+    @DisabledOnJre(JRE.JAVA_11)


Shouldn't this test be disabled for every Java version after 11?

See my comment above.

JuditKnoll · 2023-10-05T07:55:13Z

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue2182Test.java

-        assumeFalse(System.getProperty("java.specification.version").startsWith("1."));
-        int javaVersion = Integer.parseInt(System.getProperty("java.specification.version"));
-        assumeThat(javaVersion, is(greaterThanOrEqualTo(11)));
-    }


Are you sure that we don't need to check the Java version for this test?

We only build with jdk 17 currently, after my other pr we will be able to build jdk 11, 17, and 20. So yes this was not needed as we cannot build with anything lower than jdk 11.

JuditKnoll · 2023-10-05T07:55:25Z

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue2183Test.java

-        assumeFalse(System.getProperty("java.specification.version").startsWith("1."));
-        int javaVersion = Integer.parseInt(System.getProperty("java.specification.version"));
-        assumeThat(javaVersion, is(greaterThanOrEqualTo(11)));
-    }


Are you sure that we don't need to check the Java version for this test?

We only build with jdk 17 currently, after my other pr we will be able to build jdk 11, 17, and 20. So yes this was not needed as we cannot build with anything lower than jdk 11.

JuditKnoll · 2023-10-05T07:56:29Z

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue2184Test.java


    @Test
-    public void test() {
+    @DisabledOnJre(JRE.JAVA_11)


In the original version Java 14 and later is assumed, but here you disable on Java 11. Was this intentional?

See line 19 below, these are for higher jdks (yes originally 14) but we don't use 14 to build as its obsolete. Instead we have certain sets of tests for the jdks we use to build with. The reason this even worked as-is results from fact repo only used jdk 17 to build. Once my other PR is applied, this would in fact fail if jdk 11 was still allowed for consideration.

And note, that also means with removal of toolchains. On Github, toolchains won't work for a few reasons. Mainly because there was no request to download other jdks to start with. Its toolchained to itself currently In my other PR I remove that for now as its not appropriate to demonstrate what we are trying to show and if it were, it would need more in GHA to download other jdks to start with. The overhead of downloading a bunch of jdks isn't too worth the effort when really just trying to show users its as compatible as possible as we can here without going totally overboard.

Then only the test samples should be built with different JDKs, and the tests should be run with different JREs, not the entire project. We don't need to show that the SpotBugs project can be built using Java 8, but that it can analyze code built with Java 8, and can be run by Java 11.

JuditKnoll · 2023-10-05T07:58:02Z

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue2552Test.java

    @Test
-    public void test() {
+    @DisabledOnJre(JRE.JAVA_11)


Are you sure that this test needs to be disabled on Java 11?

Same thing as noted earlier, this is the jdk 17 tests.

JuditKnoll · 2023-10-05T08:01:51Z

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/ba/Issue1367Test.java


    @Test
-    public void test() {
+    @DisabledOnJre(JRE.JAVA_11)


Are you sure that this test needs to be disabled on Java 11?

Yes, it only ever was for jdk > 11, see line 28, its specifically aligned to our usage of jdk 17.

hazendaz · 2023-10-05T17:16:08Z

Fixing the Sonar setup would be great too, the code coverage was broken and is zero. That PR is also not showing up in https://sonarcloud.io/project/pull_requests_list?id=com.github.spotbugs.spotbugs So it makes it harder to check the impact of that PR

Any idea why its not publishing results on the PR? I see it doing so for other PRs not seeing anything in the logs jumping out as a problem.

gtoison · 2023-10-06T05:33:43Z

Since the changes to build on Windows and MacOs sonar should be running 3 times, but searching in the logs of that PR it does not seem to be running at all

hazendaz · 2023-10-06T10:47:25Z

Our build isn't compatible with Java 8 to run. We would have to download grade a lot more. I can look at that separately but bet it's far more complicated to show that. If it's too complicated it would be better to run integration tests using 8 instead like I do with the maven plugin. I'll look at sonar. All the others I saw seem to indicate it only runs sonar if branch is on spotbugs which is incorrect. It does need to run on all but not seeing why it would not. Typically on projects I've worked sonar is part of GHA rather than hidden in the build tool. Sent from my Verizon, Samsung Galaxy smartphone Get Outlook for Android<https://aka.ms/AAb9ysg>

…

________________________________ From: Guillaume Toison ***@***.***> Sent: Friday, October 6, 2023 1:33:54 AM To: spotbugs/spotbugs ***@***.***> Cc: Jeremy Landis ***@***.***>; Assign ***@***.***> Subject: Re: [spotbugs/spotbugs] Migrate to full junit 5 and add some associated code expected to support junit 5 - fixes #2596 (PR #2605) Since the changes to build on Windows and MacOs sonar should be running 3 times, but searching in the logs of that PR it does not seem to be running at all — Reply to this email directly, view it on GitHub<#2605 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAHODIYGJTY5HQRY6UW76TLX56J4FAVCNFSM6AAAAAA5TRBWI6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJQGAYDSMJUGY>. You are receiving this because you were assigned.Message ID: ***@***.***>

Discovered 'DontAssertInstanceofInTests' when upgrading to junit 5. The others seem to imply same need. While 'TestaseDetector' probably is a no-op, its better to mention both. The dumbMethods seems to need the same check or would benefit. And finally just noting on FindNullDeref that its not needed for junit 5 given junit 5 doesn't do it that way.

…ther packages

for java11 or better, add flag to ignore java 8 for java17 or better, add flag to ignore java 8/11 for test that seen failing with java 11, add ignore on java 8/11 Note: We are going to explore showing still building with java 8 separately so this preps based on our coding style here. It does not mean java 8 will work. Further, the reason we are doing this effort is for contributors so they are not scared away by not having a java version to use to test as many are stuck at various points and we get more engagement from developers if they are not so limited. That is a separate effort bot noting as the PR itself has comments in this regard.

hazendaz · 2023-10-07T15:21:43Z

Per various comments, I've added jvm checks to the tests flagged as java 11 to just follow the pattern as it stands at the moment. That pattern is that java11 folder tests must be done on java11 or better, therefore java8 which is the main code base is simply stated to not allow java 8 on those. Depending how we try to handle builds on the other PR I have (possibly adding back 8) will simply follow this pattern of coding without further changes. Sometime later we would condense down and delete unnecessary folder packages by merging code into the main code line. I suspect we do that with version 5 when we bump to java 11 only.

I presume the readthedocs fix @gtoison made also fixed the issue we had been seeing last few days. Thanks, that is working again here too.

For sonar, based on other PRs coming in, its clear we are not running on pull requests properly from other repos. Will look into that as that is clearly a problem.

Fixing spotless now, will push up in a few.

hazendaz · 2023-10-07T19:05:36Z

@gtoison Nothing I can do here to assist with sonar. I'm not admin on it to even touch it, not even part of the org for that one. So that will require someone else on the team to grant more access to even look at it.

The code coverage I'm sure will be tricky and require a lot of work on jacoco usage because the tests were separated a long time ago from the source code so defaults showing zero make sense, it doesn't have the agent data in right area if I had to guess.

As to this PR, code coverage isn't possible to fall here, no tests were removed. They were simply made full junit 5 and we were already using the junit 5 vintage engine. I've done so many junit 5 migrations since 2017 at this point that I have no real concerns in that area. We could explore going forwards other tools. I don't know if coveralls works with gradle but that might be an option. We could also do something in the build to make it not build if coverage is not met. We don't need sonar for that. Sonar is only getting its data from jacoco to start with but again feels like we need a lot of work there due to split of tests from the source so jacoco metrics would be in separate areas. Maybe that is ok and easy to deal with, don't know for sure...

gtoison · 2023-10-09T06:25:26Z

For sonar I think there are two issues:

the analysis does not run on forked repos, I suppose that it's because these repos would need the sonar token and authorizations
the code coverage is not reported, I suppose that the jacoco reports are either not in the expected location or not produced at all?
But sorry to bring that up here, that should be a separate issue

JuditKnoll · 2023-10-09T07:46:46Z

...st/java/edu/umd/cs/findbugs/detect/IncorrectSelfComparisonInstanceOfPatternMatchingTest.java

-        int javaVersion = Integer.parseInt(System.getProperty("java.specification.version"));
-        assumeThat(javaVersion, is(greaterThanOrEqualTo(14)));
-
+    @DisabledOnJre({ JRE.JAVA_8, JRE.JAVA_11 })


Can you please add a comment that Java 14 or greater is needed to not to lose info?

will work this with #2622 and retest each jdk to get the rigth values listed.

JuditKnoll · 2023-10-09T07:50:28Z

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue2184Test.java


    @Test
-    public void test() {
+    @DisabledOnJre({ JRE.JAVA_8, JRE.JAVA_11 })


Can you please add a comment that Java 14 or greater is needed to not to lose info?

yes can do.

JuditKnoll · 2023-10-09T07:58:59Z

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue2120Test.java


    @Test
-    public void test() {
+    @DisabledOnJre(JRE.JAVA_11)


I think it is disabled on Java 11, because the test sample files are in the java17 directory. So the containing directory and the assumptions about the Java versions are not consistent. @hazendaz Did you check this test, does it fail on Java 11?

hazendaz · 2023-10-11T00:11:31Z

Hi @JuditKnoll, I'll double check all the tests to be certain. I know there was 1 test specifically that failed with java 11 so I just flagged it. Some like you noted didn't state one way or the other but we were forcing jdk 17 only to build. I think it makes sense to just roll these all back into normal packages and just use the respective junit flag for specific versions. I don't have most of the old jdks any more to confirm one way or another so it was mostly flagged here based on our usage (11, 17, 21, etc). We could get specific but if we are not allowing that one to build its rather irrelevant. Main thing was to show this does build with java 11 anyways so more contributors are not scared away if stuck on older jdks. But certainly more cleanup will help as we move forwards given junit 5 gives such better way of doing it.

I'll try to get back to this in a day or two.

hazendaz · 2023-10-11T00:17:52Z

For sonar I think there are two issues:

* the analysis does not run on forked repos, I suppose that it's because these repos would need the sonar token and authorizations

* the code coverage is not reported, I suppose that the jacoco reports are either not in the expected location or not produced at all?
  But sorry to bring that up here, that should be a separate issue

I opened two new tickets for the sonar / coverage metrics situation. Agree unrelated here but would have been highly helpful.

hazendaz · 2023-10-11T23:37:33Z

ok opened on other ticket to come back to jdk checks. I'll end up downloading all the older jdks and do a thorough test through the tests then squash those all done per issue #2622. No need to hold up the release any longer. I'm going to re-look at this PR again just to be certain its ok and nothing glaring, will circle back on other issues separately. Once merged likely will go ahead and merge in the build update changes to run back on jdk 11. I know question in here was around jdk 8 as well. When I work the other ticket I'll try to check that far back but think we are ready to release at this point which will calm down the jdk 21 tickets :)

hazendaz self-assigned this Oct 5, 2023

hazendaz requested review from gtoison and JuditKnoll October 5, 2023 02:55

hazendaz changed the title ~~Migrate to full junit 5 and add some associated code expected to support junit 5~~ Migrate to full junit 5 and add some associated code expected to support junit 5 - fixes #2596 Oct 5, 2023

gtoison reviewed Oct 5, 2023

View reviewed changes

JuditKnoll reviewed Oct 5, 2023

View reviewed changes

hazendaz added 6 commits October 7, 2023 11:15

[junit5] Convert to junit 5

fd83e62

[ci] Update change log for junit 5 upgrade

e6ac13e

[ci] Update PR on changelog for junit5

c7b70e5

[tests] Per review, cleanup rest of left over publics from tests in o…

460681c

…ther packages

hazendaz force-pushed the next branch from 92cc1e9 to 2b5774c Compare October 7, 2023 15:16

[ci] formatting

d1fd0bf

JuditKnoll reviewed Oct 9, 2023

View reviewed changes

This was referenced Oct 11, 2023

Build: Fix sonar so it runs on pull requests #2618

Open

Build: Fix jacoco reporting to sonar #2619

Closed

gtoison approved these changes Oct 11, 2023

View reviewed changes

JuditKnoll approved these changes Oct 11, 2023

View reviewed changes

hazendaz mentioned this pull request Oct 11, 2023

Build: Move java11, java17, etc tests into main packages and use junit 5 to check accurate jdk usage #2622

Open

hazendaz merged commit b6fd746 into spotbugs:master Oct 12, 2023
6 checks passed

JuditKnoll mentioned this pull request Oct 16, 2023

Fix escaping exception from DumbMethods #2643

Merged

1 task

hazendaz added this to the SpotBugs 4.8.0 milestone Dec 18, 2023

JuditKnoll mentioned this pull request Jan 12, 2024

Created a new detector for rule MET04-J #1994

Open

7 tasks

Migrate to full junit 5 and add some associated code expected to support junit 5 - fixes #2596 #2605

Migrate to full junit 5 and add some associated code expected to support junit 5 - fixes #2596 #2605

Conversation

hazendaz commented Oct 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtoison commented Oct 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hazendaz commented Oct 5, 2023

gtoison commented Oct 6, 2023

hazendaz commented Oct 6, 2023 via email

hazendaz commented Oct 7, 2023

hazendaz commented Oct 7, 2023

gtoison commented Oct 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hazendaz commented Oct 11, 2023

hazendaz commented Oct 11, 2023

hazendaz commented Oct 11, 2023

hazendaz commented Oct 5, 2023 •

edited