On Wed, 13 Sep 2023 14:58:21 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> PR adds tests mentioned in the title - a new `AttributesTest` class is added
>> testing iconification, maximization and full-screen-ification of a Stage.
>>
>> All variants are tested with decorated stage style.
>>
>> Iconification is tested via overlaying two stages on top of one another, and
>> then iconifying the top one - this is similar to already existing
>> `IconifyTest.java` but it tests just the iconfication process and nothing
>> more.
>>
>> Maximization and FullScreen are both tested by creating two stages _not_
>> overlapping each other. After maximization/fullscreen top stage (being
>> always on top as well) should cover the bottom stage. Moreover, FullScreen
>> and Maximize are differentiated by checking if window decoration exists -
>> maximized Stage will have its decoration taking space on top of the screen,
>> whereas FullScreen one will not.
>>
>> **NOTE:** on macOS I had issues with `getColor()` returning a valid color
>> when called a second time. This only happened on macOS and with FullScreen
>> test (others worked fine). Unfortunately I couldn't figure out why it
>> returned (0, 0, 0, 255) or (255, 255, 255, 255). To mitigate that I moved
>> color checks into separate `runAndWait()`-s with a small sleep between them,
>> which seemed to help `getColor()` return proper values.
>>
>> Verified to work on Windows 11, macOS and Linux.
>
> tests/system/src/test/java/test/robot/javafx/stage/AttributesTest.java line
> 42:
>
>> 40: import javafx.scene.layout.Pane;
>> 41: import javafx.scene.paint.Color;
>> 42: import static junit.framework.Assert.assertFalse;
>
> should we use junit5 for new tests?
Fair point, import autocomplete got me there.
> tests/system/src/test/java/test/robot/javafx/stage/AttributesTest.java line
> 48:
>
>> 46: import static test.util.Util.TIMEOUT;
>> 47:
>> 48: public class AttributesTest extends VisualTestBase {
>
> a brief javadoc explaining what this test does would be nice.
> also, perhaps the class name could be more descriptive, i.e.
> StageAttributesTest or something like that.
I'll add the javadoc.
I skipped the `Stage` prefix since this test is in a directory of
`Stage`-related tests. Should I still add it despite that?
> tests/system/src/test/java/test/robot/javafx/stage/AttributesTest.java line
> 56:
>
>> 54: private static final Color TOP_COLOR = Color.RED;
>> 55:
>> 56: private static final double TOLERANCE = 0.07;
>
> just curious: how did we arrive at this value?
I borrowed it from `IconifyTest.java` which most of this file is based on. Hard
for me to say why is it exactly that.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1240#discussion_r1324707762
PR Review Comment: https://git.openjdk.org/jfx/pull/1240#discussion_r1324707030
PR Review Comment: https://git.openjdk.org/jfx/pull/1240#discussion_r1324705760
