On Mon, 16 Sep 2024 17:57:07 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
> Converting system tests to junit5. > > Please see migration notes: > https://github.com/andy-goryachev-oracle/Test/blob/main/doc/Tests/JUnit5Migration.md > > ### Notes: > > I see shutdown timeout on linux, this will be addressed by > [JDK-8340403](https://bugs.openjdk.org/browse/JDK-8340403) > > QPathTest > executionError FAILED > org.opentest4j.AssertionFailedError: Exceeded timeout limit of 10000 msec > at > app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:39) > at app//org.junit.jupiter.api.Assertions.fail(Assertions.java:134) > at app//test.util.Util.runAndWait(Util.java:156) > at app//test.util.Util.runAndWait(Util.java:127) > at app//test.util.Util.shutdown(Util.java:365) > at app//test.com.sun.marlin.QPathTest.teardownOnce(QPathTest.java:146) > > > This test might fail intermittently (?) on linux only: > > SetSceneScalingTest > testShowAndSetScene() FAILED > org.opentest4j.AssertionFailedError: expected: <true> but was: <false> > at > app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55) > at > app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40) > at > app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:35) > at > app//org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:179) > at > app//test.robot.javafx.stage.SetSceneScalingTest$TestShowSetSceneApp.test(SetSceneScalingTest.java:129) > at > app//test.robot.javafx.stage.SetSceneScalingTest$TestApp.runTest(SetSceneScalingTest.java:89) > at > app//test.robot.javafx.stage.SetSceneScalingTest.testShowAndSetScene(SetSceneScalingTest.java:193) The code changes look good with a few comments. I ran a full test including unstable tests. I see a few failures that you will want to look at (beyond what I would expect for the unstable Monocle tests). I'll add a comment with more detail. tests/system/src/test/java/test/javafx/scene/Snapshot1Test.java line 139: > 137: // Should throw IllegalStateException > 138: tmpScene.snapshot(p -> { > 139: throw new RuntimeException("Should never get here"); Suggestion: Use `fail`, rather than changing the Error into a RuntimeException. tests/system/src/test/java/test/javafx/scene/Snapshot1Test.java line 169: > 167: // Should throw IllegalStateException > 168: tmpNode.snapshot(p -> { > 169: throw new RuntimeException("Should never get here"); Use `fail` instead? tests/system/src/test/java/test/javafx/scene/Snapshot1Test.java line 265: > 263: // Should not get here > 264: latch.countDown(); > 265: throw new RuntimeException("Should never get here"); Use `fail` instead? tests/system/src/test/java/test/javafx/scene/Snapshot1Test.java line 387: > 385: // Should not get here > 386: latch.countDown(); > 387: throw new RuntimeException("Should never get here"); Use `fail` instead? tests/system/src/test/java/test/javafx/scene/Snapshot2Test.java line 182: > 180: public void testSnapshotEmptyNodeImmNoParams(boolean live, boolean > useImage) { > 181: setupEach(live, useImage); > 182: doTestSnapshotEmptyNodeDefer(live, useImage, null); Question about the pattern: did you consider having the setupEach method store the params in instance variables (the ones you removed)? Then you wouldn't have needed to change any of the other methods to pass them to those methods. Just a thought that occurred to me as I was looking at this. What you have is fine. tests/system/src/test/java/test/javafx/scene/UIRenderSnapToPixelTest.java line 76: > 74: Assertions.assertEquals(0, ((sp.snappedBottomInset() * > scale) + epsilon) % 1, 0.0001, "Bottom inset not snapped to pixel"); > 75: Assertions.assertEquals(0, ((sp.snappedLeftInset() * > scale) + epsilon) % 1, 0.0001, "Left inset not snapped to pixel"); > 76: Assertions.assertEquals(0, ((sp.snappedRightInset() * > scale) + epsilon) % 1, 0.0001, "Right inset not snapped to pixel"); Minor: these lines are geting a bit long. Maybe use static imports? tests/system/src/test/java/test/javafx/scene/shape/meshmanagercacheleaktest/MeshManagerCacheLeakTest.java line 44: > 42: * Unit test for verifying leak with cache of TriangleMesh in > PredefinedMeshManager. > 43: */ > 44: @Timeout(value=15000, unit=TimeUnit.MILLISECONDS) One of the tests had a 20 second timeout, so this could make that test less stable. I recommend changing this to 20 sec. tests/system/src/test/java/test/robot/javafx/embed/swing/NonFocusableJFXPanelTest.java line 51: > 49: import static org.junit.jupiter.api.Assertions.assertNotNull; > 50: import static org.junit.jupiter.api.Assertions.assertNull; > 51: import static org.junit.jupiter.api.Assertions.assertSame; I promised myself I wasn't going to look at the imports, but... Does Eclipse really mix static and non-static imports? And are any of the static imports actually used? I suspect not in both cases, so you might just need to have Eclipse fix up your imports. tests/system/src/test/java/test/robot/javafx/embed/swing/SwingNodePlatformExitCrashTest.java line 39: > 37: > 38: @Test > 39: @Disabled("JDK-8190329") Unrelated to your PR...well this is embarrassing! I just fixed the FX clone of JDK-8190329 a couple weeks ago, and forgot to look to see if there was an existing test. I'll file a follow-up bug for this. tests/system/src/test/java/test/robot/javafx/scene/SRGBTest.java line 61: > 59: import static org.junit.jupiter.api.Assertions.assertFalse; > 60: import static org.junit.jupiter.api.Assertions.assertNotNull; > 61: import static org.junit.jupiter.api.Assertions.assertNull; Unused imports? tests/system/src/test/java/test/robot/javafx/scene/SRGBTest.java line 249: > 247: // Timeout for potential hang on XWayland, see JDK-8335468. > 248: // the same timeout will apply to the rest of the tests > 249: // @Test(timeout = 15000) I would remove this commented out line, since it is a JUnit 4 holdover. Also, would it be better to apply the `@Timeout` annotation to just this method? Either is fine with me. tests/system/src/test/java/test/robot/javafx/web/TooltipFXTest.java line 50: > 48: import test.util.memory.JMemoryBuddy; > 49: > 50: @Timeout(value=15000, unit=TimeUnit.MILLISECONDS) This should be 20 seconds to match the test method you moved it from. ------------- PR Review: https://git.openjdk.org/jfx/pull/1569#pullrequestreview-2313831535 PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1765758123 PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1765760215 PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1765760327 PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1765760553 PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1765766941 PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1765770244 PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1765797326 PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1765861443 PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1765864042 PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1765866196 PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1765867552 PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1765869762
