8338468: [TestBug] Convert controls tests to JUnit 5 #1561

andy-goryachev-oracle · 2024-09-10T18:25:40Z

Converting control module tests to junit5.

The following notes might help reviewers and people migrating other parts of https://bugs.openjdk.org/browse/JDK-8339170. The direct link to the notes:
https://github.com/andy-goryachev-oracle/Test/blob/main/doc/Tests/JUnit5Migration.md

JUnit5 Migration Notes

Most of the changes are trivial, except for the following:

assertEquals() and similar methods: the message can be confused with the expected argument (junit5 moved the message to the last position)
parameterized tests: junit5 allows for parameterizing individual tests
parameterized @BeforeEach and @AfterEach: (see discussion below)
charts: the test hierarchy for charts mixed parameterized and non-parameterized kinds, necessitating more changes
overridden parameterized tests (must be annotated with @ParameterizedTest @MethodSource

Parameterized Class-Level Tests

junit5 does not support parameterized class-level tests yet (see junit-team/junit5#878)

The workaround is to setup each test explicitly by calling the method that used to be annotated with @Before in each parameterized test method. There might be another solutions (see, for example, https://stackoverflow.com/questions/62036724/how-to-parameterize-beforeeach-in-junit-5/69265907#69265907) but I thought explicit setup might be simpler to deploy.

To summarize:

remove @Before from the setup method
call the setup method from each parameterized method (adding parameters and replacing @Test with

  @ParameterizedTest
  @MethodSource("parameters")

where parameters() is a static method which supplies the parameters. In the case when parameters have more than one element, the following code might be useful:

private static Stream<Arguments> parameters() {
    return Stream.of(
            Arguments.of("a", 1),
            Arguments.of("foo", 3)
    );
}

Migration Tricks

Here are the steps that might speed up the process:

remove all the junit4 imports
paste the following junit5 imports (below)
fix the errors
optimize imports via IDE (command-shift-O in Eclipse on macOS)
after all is done, verify that there is no more junit4 names by running the command mentioned below

junit5 imports (in no particular order):

import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.MethodSource;
import org.junit.jupiter.params.provider.Arguments;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertNotSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertTimeout;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import org.junit.jupiter.api.Assumptions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.params.provider.Arguments;

The following command verifies that there is no junit4 imports and inline fully qualified names:

grep -nr -e "org\.junit" -e "junit\.framework" . | grep -v jupiter

(thank you @lukostyra @arapte @kevinrushforth !)

Acceptance Criteria

Aside from the standard review process, I think the following criteria should be sufficient:

successful GHA (Github Actions) run on all platforms
the same number of tests executed (number of tests minus number of ignored tests)
grep shows no hits for junit4 imports

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8338468: [TestBug] Convert controls tests to JUnit 5 (Enhancement - P4)

Reviewers

Marius Hanl (@Maran23 - Committer)
Lukasz Kostyra (@lukostyra - Committer)
Ambarish Rapte (@arapte - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1561/head:pull/1561
$ git checkout pull/1561

Update a local copy of the PR:
$ git checkout pull/1561
$ git pull https://git.openjdk.org/jfx.git pull/1561/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1561

View PR using the GUI difftool:
$ git pr show -t 1561

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1561.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-09-10T18:25:55Z

👋 Welcome back angorya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-09-10T18:26:30Z

@andy-goryachev-oracle This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8338468: [TestBug] Convert controls tests to JUnit 5

Reviewed-by: mhanl, lkostyra, arapte

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 2 new commits pushed to the master branch:

addf085: 8340405: JavaFX shutdown hook can hang preventing app from exiting
bc5adfa: 8340208: Additional WebKit 619.1 fixes from WebKitGTK 2.44.4

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Maran23 · 2024-09-10T22:40:35Z

@MethodSource, @ValueSource or @EnumSource are the best replacements for parameterized tests. But whatever option you choose, it is a bit of refactoring involved indeed. I would suggest to do the parameterized tests in a new ticket, after converting the 'easy' cases to JUnit5 for that particular module/package.

I can also help on that if desired since I also think this is a good idea and logical step for the future of our tests (and all tests that will be written in the future of course).

andy-goryachev-oracle · 2024-09-10T23:11:12Z

Thank you @Maran23 !

It's pretty surprising that junit people thought it will be a good idea to drop support for class-wide parameterized tests...

Maran23 · 2024-09-10T23:32:31Z

It's pretty surprising that junit people thought it will be a good idea to drop support for class-wide parameterized tests...

Well, I would rephrase it:

In Junit4, you were very restricted how to write parameterized tests. You always need to do on a new class + class level annotation + configuration
In Junit5, you can do it with the different Source annotations, which enable more flexibility as you don't need a separate class with and all the configuration attached to it. The Parameters are now essentially MethodSources, and nothing more is needed other than to mark the Test as ParameterizedTest

So I would not compare it directly but rather say it is a new, better and more flexible way to write them.

andy-goryachev-oracle · 2024-09-11T14:54:43Z

I would rephrase it:

To clarify, the main problem introduced by the decision not to support class-level parameterization is backward compatibility (our use case).

The issue is that in many tests we have, the @Before and sometimes @After methods are also effectively parameterized. Yes, one can bring a bunch of extra files (see https://stackoverflow.com/questions/62036724/how-to-parameterize-beforeeach-in-junit-5/69265907#69265907) but one still has to modify all the existing @Tests with the parameters, basically negating the possible savings.

Keeping backward compatibility should have been a priority.

/rant

…rols

mlbridge · 2024-09-13T18:27:20Z

Webrevs

andy-goryachev-oracle · 2024-09-13T18:28:53Z

For reviewers: it might be easier to review commit-by-commit, just keep in mind that in several places I had to go back to fix things (just check the final version).

@aghaisas and/or @lukostyra could you be the reviewers please?

@Maran23 I can't ask you to do a (full) review, but if you could do a spot check, I'd appreciate!

thank you all in advance!

Maran23

Looks good overall. Could not spot any obvious error.
Left some comments regarding code style to reduce warnings or use newer/better API.

Maran23 · 2024-09-15T13:01:49Z

...ols/src/test/java/test/com/sun/javafx/scene/control/LambdaMultipleObservableHandlerTest.java

        IntegerProperty p = new SimpleIntegerProperty();
-        assertNull(unregisterListeners(p));
+        assertTrue(unregisterListeners(useChangeListener, p) == null);


This could be assertNull still?

yes! I don't know what I was thinking... good catch!

Maran23 · 2024-09-15T13:02:00Z

...ols/src/test/java/test/com/sun/javafx/scene/control/LambdaMultipleObservableHandlerTest.java

+    @ParameterizedTest
+    @MethodSource("data")
+    public void testUnregistersNull(boolean useChangeListener) {
+        assertTrue(unregisterListeners(useChangeListener, null) == null);


This could be assertNull still?

Maran23 · 2024-09-15T13:03:18Z

...ols/src/test/java/test/com/sun/javafx/scene/control/LambdaMultipleObservableHandlerTest.java

-    @Parameterized.Parameters //(name = "{index}: changeListener {0} ")
-    public static Collection<Object[]> data() {
+    /** parameters */
+    private static Collection<Object[]> data() {


Minor: This could also be just a Stream of Boolean Arguments

Maran23 · 2024-09-15T13:03:50Z

...controls/src/test/java/test/com/sun/javafx/scene/control/behavior/AccordionBehaviorTest.java

-    @Test public void focusGainedIsCaughtByBehavior() {
-
+    @Test
+    public void focusGainedIsCaughtByBehavior() {


Wait, is this an empty test? Why?

Intentionally did not want to change the number of tests, which should be double checked during the review. Leaving as is for now.

Maran23 · 2024-09-15T13:06:41Z

...rols/src/test/java/test/com/sun/javafx/scene/control/infrastructure/MouseEventFirerTest.java

    }

 // ------------- parameterized in not/alternative mouseEvent creation

-    private boolean useAlternative;
-
-    @Parameterized.Parameters
    public static Collection<Object[]> data() {


Minor: Could also be simplified with the argument stream

this was fixed in one of the subsequent commits.

Maran23 · 2024-09-15T13:36:51Z

...les/javafx.controls/src/test/java/test/javafx/scene/control/ToolBarHorizontalArrowsTest.java

-            {NodeOrientation.RIGHT_TO_LEFT}
-        });
+    private static Collection<NodeOrientation> parameters() {
+        return Arrays.asList(


Minor: Could also use List.of(..)

Maran23 · 2024-09-15T13:43:46Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/ArrayLinkedListTest.java

-        try {
+    @Test
+    public void testArrayLinkedList_Empty_GetResultsInArrayIndexOutOfBounds() {
+        assertThrows(IndexOutOfBoundsException.class, () -> {


Maran23 · 2024-09-15T13:46:14Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinCreationTest.java

    }

-    public LabelSkinCreationTest(
+    public record Parameter(


Minor: Can also imagine a name like LabelParameter or LabelConfig

rather, this record should be declared private

Also that is a good idea.

Maran23 · 2024-09-15T13:49:07Z

...vafx.controls/src/test/java/test/javafx/scene/control/skin/QueryAccessibleAttributeTest.java

-    public void setup() {
+    // @BeforeEach
+    // junit5 does not support parameterized class-level tests yet
+    public void setup(Class<Node> nodeClass) {


Minor: Since Control is used above, should also be used here and below as Generic.

hmm, the code is technically correct since Control extends Node.

yes, just a minor nitpick since relaxing the bounds is not really needed

Maran23 · 2024-09-15T13:52:41Z

...les/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinLabeledCleanupTest.java

-
-    public SkinLabeledCleanupTest(Class<Labeled> labeledClass) {
-        this.labeledClass = labeledClass;
+    private static Collection<Class> parameters() {


Minor: Class<?>

andy-goryachev-oracle · 2024-09-16T15:04:05Z

Thank you @Maran23 for taking a look!
I tried to minimize the changes, so purposely left the original code (empty tests, empty @AfterEach etc. intact.

I'll fix the small things you pointed out; I would rather avoid making more involved structural changes now.

Again, thanks!

…rols

andy-goryachev-oracle · 2024-09-17T22:46:39Z

@lukostyra @jaybhaskar please review

Maran23

Looks good to me.

arapte · 2024-09-18T03:50:17Z

modules/javafx.controls/src/test/java/test/javafx/scene/chart/NumberAxisTest.java

-    @Test(timeout = 1000)
+    @Test
    public void testCloseValues() {
-        axis.setForceZeroInRange(false);
-        axis.setSide(Side.LEFT);
-        double minValue = 1.0;
-        double maxValue = minValue + Math.ulp(minValue);
-        NumberAxisShim.autoRange(axis, minValue, maxValue, 500, 50);
+        assertTimeout(Duration.ofMillis(1000), () -> {


Could use the Timeout annotation as : @Timeout(1) or @Timeout(1000 ms)

Yes, please. I made a similar comment on Draft PR #1569

Agree, would be good to be consistent, especially now, where we anyway convert everything to JUnit 5 and have the chance to be consistent.

yep, for some reason I incorrectly decided that Timeout was not applicable. will change to Timeout.

…rols

lukostyra

Went through about 2/3rd of your changes, will continue tomorrow starting from javafx/scene/control/TreeViewKeyInputTest.java. In the meantime, a couple comments.

lukostyra · 2024-09-19T12:29:27Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlSkinTest.java

-import org.junit.Before;
-import org.junit.Ignore;
-import org.junit.Test;
+import org.junit.jupiter.api.Assumptions;


[Minor] Why not import static org.junit.jupiter.api.Assumptions.assumeTrue? We're directly importing Assertions this way anyway.

lukostyra · 2024-09-19T12:29:49Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlTooltipTest.java

+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertTimeout;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import org.junit.jupiter.api.Assumptions;


Why not import static org.junit.jupiter.api.Assumptions.assumeTrue? We're directly importing Assertions this way anyway.

a) I do prefer qualifiers, it makes it easier to see where things are coming from
b) but in this case, and in this PR in general, there is a mixture of static imports and class imports - for convenience and speed.

should be ok, it does not change the bytecode.

lukostyra · 2024-09-19T13:16:21Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextInputControlTest.java

        textInput.setText("The quick brown fox");
        textInput.selectRange(100, 180);
        assertEquals("", textInput.getSelectedText());
    }

-//    @Test public void selectedTextWorksWhenSelectionIsBound() {


This test was removed. Shouldn't we keep even commented tests around?

out of scope, for this PR

I'm not sure I understand. If removing old/commented out tests is out of scope of this PR then why is this one (and others) removed by this PR?

I agree with @lukostyra -- we should not be removing any commented out tests.

I thought that commented out tests have only historical value (or should be removed).
If not, they should be disabled.
If they are commented out, a comment must be added as to why.

in this case, will apply the change to commented out test.

lukostyra · 2024-09-19T13:17:30Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextInputControlTest.java

        textInput.setText("The quick brown fox");
        textInput.selectRange(44, 99);
        assertEquals(new IndexRange(19, 19), textInput.getSelection());
    }

-//    @Test public void selectionCanBeBound() {


As above, removed test that was commented out

lukostyra · 2024-09-19T13:17:41Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextInputControlTest.java

@@ -605,33 +750,10 @@ public void caretAndAnchorPositionAfterSettingText() {
        assertTrue(passed[0]);
    }

-//    @Test public void selectionChangeEventsHappenWhenBound() {


lukostyra · 2024-09-19T13:20:25Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/ToolbarTest.java

+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertTimeout;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import org.junit.jupiter.api.Assumptions;


Similar situation with Assumptions as earlier

lukostyra · 2024-09-19T13:27:08Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java

+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertTimeout;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import org.junit.jupiter.api.Assumptions;


Similar Assumptions question/situation

lukostyra · 2024-09-20T11:20:40Z

I reviewed the rest of your change, looks good outside of comments above. Will approve once we resolve those.

…rols

Maran23 · 2024-09-22T13:07:57Z

modules/javafx.controls/src/test/java/test/javafx/scene/chart/NumberAxisTest.java

@@ -257,72 +257,67 @@ public void checkTickUnitPropertyBind() {
    }

    @Test
+    @Timeout(value=1000, unit=TimeUnit.MILLISECONDS)


minor: I think the convention is to have spaces around the parameters,
e.g. @Timeout(value = 1000, unit = TimeUnit.MILLISECONDS)
Just scanned the JavaFX code and we seem to be inconsiostent on that, so I will leave that up to do.

I don't think we have a (different) format for annotation arguments, so I've used the function arguments format convention here, no spaces.

One point we could use later is to omit TimeUnit and express the value in seconds, but I kept milliseconds for sake of reviewers.

@Maran23 is correct about the convention when there is an annotation (or a function call, for that matter) with an embedded parameter assignment. For example, see various @Deprecated annotations in the JDK and JavaFX.

However, as he pointed out, we are not consistent on that, so I would leave it as is for this PR to avoid more churn.

lukostyra

LGTM

andy-goryachev-oracle · 2024-09-23T18:00:07Z

Thank you all for the time and effort reviewing it!
This PR still needs one "R"eviewer though.

kevinrushforth · 2024-09-23T18:15:38Z

@arapte plans to review this as the needed "R"eviewer.

arapte

LGTM

andy-goryachev-oracle · 2024-09-23T18:28:04Z

/integrate

openjdk · 2024-09-23T18:29:55Z

Going to push as commit 5171753.
Since your change was applied there have been 2 commits pushed to the master branch:

addf085: 8340405: JavaFX shutdown hook can hang preventing app from exiting
bc5adfa: 8340208: Additional WebKit 619.1 fixes from WebKitGTK 2.44.4

Your commit was automatically rebased without conflicts.

openjdk · 2024-09-23T18:30:02Z

@andy-goryachev-oracle Pushed as commit 5171753.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

part 1

e8647db

part 2

44381cf

andy-goryachev-oracle added 12 commits September 11, 2024 08:08

Merge remote-tracking branch 'origin/master' into 8338468.junit5.cont…

614d402

…rols

part 3

e6ed194

part 4

8d5de0c

part 5 charts

3300189

part 6

3266eb4

part 7

e0b92b6

part 8

a2d364c

part 9 cell

686f9f4

part 10

ef9bd6b

part 11, 9242 tests, 185 ignored

f1ccc53

Merge remote-tracking branch 'origin/master' into 8338468.junit5.cont…

3db80f1

…rols

part 12, 9274 - 185 = 9089

5e94247

andy-goryachev-oracle marked this pull request as ready for review September 13, 2024 18:23

openjdk bot added the rfr Ready for review label Sep 13, 2024

Maran23 approved these changes Sep 15, 2024

View reviewed changes

andy-goryachev-oracle added 2 commits September 16, 2024 08:16

Merge remote-tracking branch 'origin/master' into 8338468.junit5.cont…

a16ba62

…rols

review comments

b527028

Maran23 approved these changes Sep 17, 2024

View reviewed changes

arapte reviewed Sep 18, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into 8338468.junit5.cont…

06e9955

…rols

lukostyra mentioned this pull request Sep 18, 2024

8339512: [TestBug] Convert graphics tests to JUnit 5 #1566

Closed

3 tasks

review comments

55b33b2

lukostyra reviewed Sep 19, 2024

View reviewed changes

andy-goryachev-oracle added 2 commits September 20, 2024 13:19

Merge remote-tracking branch 'origin/master' into 8338468.junit5.cont…

3e8f93f

…rols

review comments

6c44cc6

Maran23 approved these changes Sep 22, 2024

View reviewed changes

lukostyra approved these changes Sep 23, 2024

View reviewed changes

arapte approved these changes Sep 23, 2024

View reviewed changes

openjdk bot added the ready Ready to be integrated label Sep 23, 2024

openjdk bot added the integrated Pull request has been integrated label Sep 23, 2024

openjdk bot closed this Sep 23, 2024

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Sep 23, 2024

8338468: [TestBug] Convert controls tests to JUnit 5 #1561

8338468: [TestBug] Convert controls tests to JUnit 5 #1561

Conversation

andy-goryachev-oracle commented Sep 10, 2024 • edited by openjdk bot Loading

JUnit5 Migration Notes

Parameterized Class-Level Tests

Migration Tricks

Acceptance Criteria

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 10, 2024

openjdk bot commented Sep 10, 2024 • edited Loading

Maran23 commented Sep 10, 2024

andy-goryachev-oracle commented Sep 10, 2024

Maran23 commented Sep 10, 2024 • edited Loading

andy-goryachev-oracle commented Sep 11, 2024

mlbridge bot commented Sep 13, 2024 • edited Loading

Webrevs

andy-goryachev-oracle commented Sep 13, 2024

Maran23 left a 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

andy-goryachev-oracle commented Sep 16, 2024

andy-goryachev-oracle commented Sep 17, 2024

Maran23 left a 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

lukostyra left a 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

lukostyra commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukostyra left a comment

Choose a reason for hiding this comment

andy-goryachev-oracle commented Sep 23, 2024

kevinrushforth commented Sep 23, 2024

arapte left a comment

Choose a reason for hiding this comment

andy-goryachev-oracle commented Sep 23, 2024

openjdk bot commented Sep 23, 2024

openjdk bot commented Sep 23, 2024

andy-goryachev-oracle commented Sep 10, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 10, 2024 •

edited

Loading

Maran23 commented Sep 10, 2024 •

edited

Loading

mlbridge bot commented Sep 13, 2024 •

edited

Loading