RFR: 8298500: Create test to initially show stage with various attributes (iconified, maximized, full screen)

[Andy Goryachev]

On Wed, 13 Sep 2023 11:37:39 GMT, Lukasz Kostyra <lkostyra at 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