On Wed, 18 Sep 2024 21:42:08 GMT, Kevin Rushforth <k...@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
