RFR: 8338468: [TestBug] Convert controls tests to JUnit 5 [v3]

Thu Sep 19 14:09:49 UTC 2024

On Wed, 18 Sep 2024 15:36:53 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> 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:
>> 
>> 1. assertEquals() and similar methods: the message can be confused with the expected argument (junit5 moved the message to the last position)
>> 2. parameterized tests: junit5 allows for parameterizing individual tests
>> 3. parameterized `@BeforeEach` and `@AfterEach`: (see discussion below)
>> 4. charts: the test hierarchy for charts mixed parameterized and non-parameterized kinds, necessitating more changes
>> 5. overridden parameterized tests (must be annotated with ` @ParameterizedTest @MethodSource`
>> 
>> ### Parameterized Class-Level Tests
>> 
>> junit5 does not support parameterized class-level tests yet (see https://github.com/junit-team/junit5/issues/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:
>> 
>> 1. remove all the junit4 imports
>> 2. paste the following junit5 imports (below)
>> 3. fix the errors
>> 6. optimize imports via IDE (command-shift-O in Eclipse on macOS)
>> 7. 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.jupit...
>
> Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 18 additional commits since the last revision:
> 
>  - review comments
>  - Merge remote-tracking branch 'origin/master' into 8338468.junit5.controls
>  - review comments
>  - Merge remote-tracking branch 'origin/master' into 8338468.junit5.controls
>  - part 12, 9274 - 185 = 9089
>  - Merge remote-tracking branch 'origin/master' into 8338468.junit5.controls
>  - part 11, 9242 tests, 185 ignored
>  - part 10
>  - part 9 cell
>  - part 8
>  - ... and 8 more: https://git.openjdk.org/jfx/compare/de179048...55b33b2c

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

modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlSkinTest.java line 41:

> 39: import javafx.scene.control.Skin;
> 40: import javafx.scene.control.Tooltip;
> 41: 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.

modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlTooltipTest.java line 48:

> 46: import static org.junit.jupiter.api.Assertions.assertTimeout;
> 47: import static org.junit.jupiter.api.Assertions.assertNotEquals;
> 48: import org.junit.jupiter.api.Assumptions;

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

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextInputControlTest.java line 517:

> 515:     }
> 516: 
> 517: //    @Test public void selectedTextWorksWhenSelectionIsBound() {

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

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextInputControlTest.java line 591:

> 589:     }
> 590: 
> 591: //    @Test public void selectionCanBeBound() {

As above, removed test that was commented out

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextInputControlTest.java line 608:

> 606:     }
> 607: 
> 608: //    @Test public void selectionChangeEventsHappenWhenBound() {

As above

modules/javafx.controls/src/test/java/test/javafx/scene/control/ToolbarTest.java line 49:

> 47: import static org.junit.jupiter.api.Assertions.assertTimeout;
> 48: import static org.junit.jupiter.api.Assertions.assertNotEquals;
> 49: import org.junit.jupiter.api.Assumptions;

Similar situation with `Assumptions` as earlier

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 136:

> 134: import static org.junit.jupiter.api.Assertions.assertTimeout;
> 135: import static org.junit.jupiter.api.Assertions.assertNotEquals;
> 136: import org.junit.jupiter.api.Assumptions;

Similar `Assumptions` question/situation

-------------

PR Review: https://git.openjdk.org/jfx/pull/1561#pullrequestreview-2315390189
PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1766744540
PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1766745066
PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1766816642
PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1766818612
PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1766818902
PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1766823201
PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1766834349