On Thu, 19 Sep 2024 21:24:59 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> 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.
In that case, `throw new AssertionError` seems the most equivalent change, in
that it preserves the behavior of throwing an `Error` rather than a
`RuntimeException` -- typically an Error is better for a case of "can't get
here". I'll admit that it doesn't matter in this particular test, so it's just
a suggestion. Feel free to ignore it.
>> 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?
>
> still too long, but I do prefer having the class qualifier over static import
> (though static imports make more sense in the tests).
>
> I think tests is one area where we can relax the maximum line length rule (we
> already violate it in many places anyway)
Yeah, don't worry about the line length, that was just an observation.
> I do prefer having the class qualifier over static import (though static
> imports make more sense in the tests).
So do I in most cases. For tests, I prefer static imports of the methods in
`Assertions` and `Assumptions`, which is also the pattern recommended by the
JUnit docs.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1768765939
PR Review Comment: https://git.openjdk.org/jfx/pull/1569#discussion_r1768770602
