On Wed, 18 Sep 2024 15:36:53 GMT, Andy Goryachev <ango...@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
