On Tue, 10 Sep 2024 18:25:40 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.jupiter.api.Test; > import org.junit.jupiter.api.Disabled; > import org.junit.jupiter.params.ParameterizedTest; > imp... Looks good overall. Could not spot any obvious error. Left some comments regarding code style to reduce warnings or use newer/better API. modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/LambdaMultipleObservableHandlerTest.java line 129: > 127: public void testUnregistersNotRegistered(boolean useChangeListener) { > 128: IntegerProperty p = new SimpleIntegerProperty(); > 129: assertTrue(unregisterListeners(useChangeListener, p) == null); This could be `assertNull` still? modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/LambdaMultipleObservableHandlerTest.java line 135: > 133: @MethodSource("data") > 134: public void testUnregistersNull(boolean useChangeListener) { > 135: assertTrue(unregisterListeners(useChangeListener, null) == null); This could be `assertNull` still? modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/LambdaMultipleObservableHandlerTest.java line 362: > 360: > 361: /** parameters */ > 362: private static Collection<Object[]> data() { Minor: This could also be just a Stream of Boolean Arguments modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/behavior/AccordionBehaviorTest.java line 39: > 37: > 38: @Test > 39: public void focusGainedIsCaughtByBehavior() { Wait, is this an empty test? Why? modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/infrastructure/MouseEventFirerTest.java line 203: > 201: // ------------- parameterized in not/alternative mouseEvent creation > 202: > 203: public static Collection<Object[]> data() { Minor: Could also be simplified with the argument stream modules/javafx.controls/src/test/java/test/javafx/scene/chart/XYNumberChartsTest.java line 99: > 97: @ParameterizedTest > 98: @MethodSource("parameters") > 99: public void testSeriesClearAnimated_rt_40632(Class chartClass, int > nodesPerSeries) { Minor: `Class<?>` modules/javafx.controls/src/test/java/test/javafx/scene/chart/XYNumberChartsTest.java line 106: > 104: @ParameterizedTest > 105: @MethodSource("parameters") > 106: public void testSeriesRemove(Class chartClass, int nodesPerSeries) { Minor: `Class<?>` modules/javafx.controls/src/test/java/test/javafx/scene/chart/XYNumberLineChartsTest.java line 63: > 61: } > 62: > 63: protected void createChart(Class chartClass, int seriesFadeOutTime, > int dataFadeOutTime) { Minor: `Class<?>` modules/javafx.controls/src/test/java/test/javafx/scene/chart/XYNumberLineChartsTest.java line 83: > 81: @ParameterizedTest > 82: @MethodSource("parameters") > 83: public void testSeriesClearAnimatedWithoutSymbols_rt_40632(Class > chartClass, int seriesFadeOutTime, int dataFadeOutTime) { Minor: `Class<?>` modules/javafx.controls/src/test/java/test/javafx/scene/chart/XYNumberLineChartsTest.java line 90: > 88: @ParameterizedTest > 89: @MethodSource("parameters") > 90: public void testSeriesRemoveWithoutSymbols(Class chartClass, int > seriesFadeOutTime, int dataFadeOutTime) { Minor: `Class<?>` modules/javafx.controls/src/test/java/test/javafx/scene/chart/XYNumberLineChartsTest.java line 98: > 96: @ParameterizedTest > 97: @MethodSource("parameters") > 98: public void testSeriesRemoveWithoutSymbolsAnimated_rt_22124(Class > chartClass, int seriesFadeOutTime, int dataFadeOutTime) { Minor: `Class<?>` modules/javafx.controls/src/test/java/test/javafx/scene/chart/XYNumberLineChartsTest.java line 119: > 117: @ParameterizedTest > 118: @MethodSource("parameters") > 119: public void testDataWithoutSymbolsAddWithAnimation_rt_39353(Class > chartClass, int seriesFadeOutTime, int dataFadeOutTime) { Minor: `Class<?>` modules/javafx.controls/src/test/java/test/javafx/scene/chart/XYNumberLineChartsTest.java line 131: > 129: @ParameterizedTest > 130: @MethodSource("parameters") > 131: public void testSeriesClearWithoutSymbolsAnimated_8150264(Class > chartClass, int seriesFadeOutTime, int dataFadeOutTime) { Minor: `Class<?>` modules/javafx.controls/src/test/java/test/javafx/scene/chart/XYNumberLineChartsTest.java line 151: > 149: @ParameterizedTest > 150: @MethodSource("parameters") > 151: public void testMinorTicksMatchMajorTicksAfterAnimation(Class > chartClass, int seriesFadeOutTime, int dataFadeOutTime) { Minor: `Class<?>` modules/javafx.controls/src/test/java/test/javafx/scene/control/AcceleratorParameterizedTest.java line 75: > 73: private KeyEventFirer keyboard; > 74: > 75: private static List<Class> parameters() { Minor: `Class<?>` modules/javafx.controls/src/test/java/test/javafx/scene/control/CellTest.java line 76: > 74: // @BeforeEach > 75: // junit5 does not support parameterized class-level tests yet > 76: private void setup(Class type) { Minor: `Class<?>` modules/javafx.controls/src/test/java/test/javafx/scene/control/EventAnyTest.java line 78: > 76: @ParameterizedTest > 77: @MethodSource("parameters") > 78: public void testEventDelivery(EventType type, Event event, Class > target, boolean matches) throws Exception { Minor: `Class<?>` modules/javafx.controls/src/test/java/test/javafx/scene/control/FireButtonBaseTest.java line 48: > 46: public class FireButtonBaseTest { > 47: public static Collection<Class> parameters() { > 48: return Arrays.asList( Minor: Could also use List.of(..) modules/javafx.controls/src/test/java/test/javafx/scene/control/FireButtonBaseTest.java line 63: > 61: //@BeforeEach > 62: // junit5 does not support parameterized class-level tests yet > 63: public void setup(Class type) { Minor: `Class<?>` modules/javafx.controls/src/test/java/test/javafx/scene/control/MultipleSelectionModelImplTest.java line 118: > 116: > 117: private static Collection<Class<? extends MultipleSelectionModel>> > parameters() { > 118: return Arrays.asList( Minor: Could also use List.of(..) modules/javafx.controls/src/test/java/test/javafx/scene/control/SelectionFocusModelMemoryTest.java line 259: > 257: private static Collection<Boolean> parameters() { > 258: // show the control before replacing the selectionModel > 259: return Arrays.asList(false, true); Minor: Could also use List.of(..) modules/javafx.controls/src/test/java/test/javafx/scene/control/SelectionModelImplTest.java line 117: > 115: > 116: private static Collection<Class<? extends SelectionModel>> > parameters() { > 117: return Arrays.asList( Minor: Could also use List.of(..) modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewHorizontalArrowsTest.java line 72: > 70: public class TableViewHorizontalArrowsTest { > 71: public static Collection<NodeOrientation> parameters() { > 72: return Arrays.asList( Minor: Could also use List.of(..) modules/javafx.controls/src/test/java/test/javafx/scene/control/TextInputControlTest.java line 64: > 62: public class TextInputControlTest { > 63: private static Collection<Class> parameters() { > 64: return Arrays.asList( Minor: Could also use List.of(..) modules/javafx.controls/src/test/java/test/javafx/scene/control/ToolBarHorizontalArrowsTest.java line 72: > 70: public class ToolBarHorizontalArrowsTest { > 71: private static Collection<NodeOrientation> parameters() { > 72: return Arrays.asList( Minor: Could also use List.of(..) modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/ArrayLinkedListTest.java line 90: > 88: @Test > 89: public void > testArrayLinkedList_Empty_GetResultsInArrayIndexOutOfBounds() { > 90: assertThrows(IndexOutOfBoundsException.class, () -> { good catch modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinCreationTest.java line 98: > 96: } > 97: > 98: public record Parameter( Minor: Can also imagine a name like `LabelParameter` or `LabelConfig` modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/QueryAccessibleAttributeTest.java line 50: > 48: // @BeforeEach > 49: // junit5 does not support parameterized class-level tests yet > 50: public void setup(Class<Node> nodeClass) { Minor: Since `Control` is used above, should also be used here and below as Generic. modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinLabeledCleanupTest.java line 86: > 84: //----------- parameterized > 85: > 86: private static Collection<Class> parameters() { Minor: `Class<?>` ------------- Marked as reviewed by mhanl (Committer). PR Review: https://git.openjdk.org/jfx/pull/1561#pullrequestreview-2305357236 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760043185 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760043199 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760043466 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760043562 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760044016 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760044770 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760044814 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760044835 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760044967 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760044988 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760045000 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760045045 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760045057 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760045069 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760045091 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760045314 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760046599 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760048634 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760046666 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760048539 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760048473 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760048405 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760048203 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760049380 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760049638 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760051126 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760051466 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760052040 PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1760052590
