On Wed, 13 Sep 2023 11:37:39 GMT, Lukasz Kostyra <lkost...@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.
tested on macOS 13.5.1 (looks good), with some minor comments and suggestions.
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?
tests/system/src/test/java/test/robot/javafx/stage/AttributesTest.java line 46:
> 44: import test.robot.testharness.VisualTestBase;
> 45:
> 46: import static test.util.Util.TIMEOUT;
this might be my personal preference - I think it's easier to read Util.TIMEOUT
in the code rather to use a static import (especially since it's used only
twice)
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.
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?
tests/system/src/test/java/test/robot/javafx/stage/AttributesTest.java line 103:
> 101: }
> 102:
> 103: public @Test void testIconifiedStage() throws InterruptedException {
could we use a more conventional formatting
@ Test
public void ...
tests/system/src/test/java/test/robot/javafx/stage/AttributesTest.java line 134:
> 132:
> 133: // wait a bit to let window system animate the change
> 134: sleep(1000);
could we use Util.waitForIdle() here instead of the fixed timeout?
tests/system/src/test/java/test/robot/javafx/stage/AttributesTest.java line 146:
> 144: // wait a little bit between getColor() calls - on macOS the
> below one
> 145: // would fail without this wait
> 146: sleep(100);
same - waitForIdle?
-------------
PR Review: https://git.openjdk.org/jfx/pull/1240#pullrequestreview-1624765793
PR Review Comment: https://git.openjdk.org/jfx/pull/1240#discussion_r1324657804
PR Review Comment: https://git.openjdk.org/jfx/pull/1240#discussion_r1324656880
PR Review Comment: https://git.openjdk.org/jfx/pull/1240#discussion_r1324660804
PR Review Comment: https://git.openjdk.org/jfx/pull/1240#discussion_r1324671847
PR Review Comment: https://git.openjdk.org/jfx/pull/1240#discussion_r1324646642
PR Review Comment: https://git.openjdk.org/jfx/pull/1240#discussion_r1324668822
PR Review Comment: https://git.openjdk.org/jfx/pull/1240#discussion_r1324669434
