RFR: 8339510: [TestBug] Convert system tests to JUnit 5

[Kevin Rushforth]

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)

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