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) I spot checked many of the classes, and most of the changes look reasonable. One question, though: Why did you replace the per-test timeout with an in-method annotation in many places (especially in the early classes I looked at)? There is a direct replacement in JUnit 5 for the per-test timeout, which seems a more straightforward change. I also think an explicit timeout on the test (or class, if all `@Test` methods have the same timeout) will work more consistently with my in-progress fix to add a global timeout. Testing recommendation: do a test run with `gradle -PUNSTABLE_TEST=true` so that all the tests that aren't disabled are run. tests/system/src/test/java/test/com/sun/javafx/application/ConcurrentStartupTest.java line 46: > 44: @Test > 45: public void testStartupReturnBeforeRunnableComplete() { > 46: assertTimeout(Duration.ofMillis(15000), () -> { Why not use an `@Timeout` annotation on the method? That would have been the natural change. tests/system/src/test/java/test/com/sun/javafx/application/InitializeJavaFXLaunch1Test.java line 42: > 40: @Test > 41: public void testLaunchThenLaunchInFX() throws Exception { > 42: assertTimeout(Duration.ofMillis(15000), () -> { Same comment as previous. Why not add an `@Timeout` annotation? tests/system/src/test/java/test/util/Util.java line 68: > 66: public static void throwError(Throwable e) { > 67: fail(e); > 68: } This is not an equivalent change. In the former code, if `testError` was a RuntimeException it was thrown directly and not wrapped. I recommend reverting this change, and then replacing only the two blocks that wrap the throwable in an AssertionError with `fail(testError)`. ------------- PR Review: https://git.openjdk.org/jfx/pull/1569#pullrequestreview-2311357616 PR Comment: https://git.openjdk.org/jfx/pull/1569#issuecomment-2357243613 PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1764218765 PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1764227387 PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1764234743
