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

[Andy Goryachev]

On Wed, 18 Sep 2024 21:42:08 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   unused imports
>
> 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.

won't compile with `fail()`.  any exception should be fine.

> 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?

won't compile.

> 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?

won't compile here either

> 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?

ditto

> 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.

yeah, I realized the same thing, a bit too late.
some of the later tests do use the instance variables.
thank you for suggestion though!
cc: @lukostyra

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1767614057
PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1767616578
PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1767617021
PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1767617531
PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1767618872