RFR: 8339510: [TestBug] Convert system tests to JUnit 5
Kevin Rushforth
kcr at openjdk.org
Wed Sep 18 23:31:47 UTC 2024
On Mon, 16 Sep 2024 17:57:07 GMT, Andy Goryachev <angorya at 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
More information about the openjfx-dev
mailing list