RFR: 8338468: [TestBug] Convert controls tests to JUnit 5
Marius Hanl
mhanl at openjdk.org
Sun Sep 15 13:59:19 UTC 2024
On Tue, 10 Sep 2024 18:25:40 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.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
More information about the openjfx-dev
mailing list