Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

lukostyra
Copy link
Contributor

@lukostyra lukostyra commented Sep 13, 2023

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.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8298500: Create test to initially show stage with various attributes (iconified, maximized, full screen) (Bug - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1240/head:pull/1240
$ git checkout pull/1240

Update a local copy of the PR:
$ git checkout pull/1240
$ git pull https://git.openjdk.org/jfx.git pull/1240/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1240

View PR using the GUI difftool:
$ git pr show -t 1240

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1240.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 13, 2023

👋 Welcome back lkostyra! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Ready for review label Sep 13, 2023
@mlbridge
Copy link

mlbridge bot commented Sep 13, 2023

Webrevs

Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested on macOS 13.5.1 (looks good), with some minor comments and suggestions.

topShownLatch.await(TIMEOUT, TimeUnit.MILLISECONDS));
}

public @Test void testIconifiedStage() throws InterruptedException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we use a more conventional formatting

@ Test
public void ...

import static junit.framework.Assert.assertTrue;
import test.robot.testharness.VisualTestBase;

import static test.util.Util.TIMEOUT;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the static import in this case. More to the point, this is what most other tests do.

import javafx.stage.StageStyle;
import javafx.scene.layout.Pane;
import javafx.scene.paint.Color;
import static junit.framework.Assert.assertFalse;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we use junit5 for new tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, import autocomplete got me there.


import static test.util.Util.TIMEOUT;

public class AttributesTest extends VisualTestBase {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more specific the class name, the better, I think (and also, specific names reduce collisions)

});

// wait a bit to let window system animate the change
sleep(1000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we use Util.waitForIdle() here instead of the fixed timeout?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, no. This isn't the intended use case for waitForIdle. That method should only be used when waiting for scene graph changes to propagate, not for the platform to finish showing or animating a window.


// wait a little bit between getColor() calls - on macOS the below one
// would fail without this wait
sleep(100);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same - waitForIdle?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. This is a small sleep that isn't intended to wait for idle.

private static final Color BOTTOM_COLOR = Color.LIME;
private static final Color TOP_COLOR = Color.RED;

private static final double TOLERANCE = 0.07;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious: how did we arrive at this value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I borrowed it from IconifyTest.java which most of this file is based on. Hard for me to say why is it exactly that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This constant, with the same value of 0.07, is repeated in most of our robot screen capture tests. It seems a good candidate for moving it to the VisualTestBase parent class, as a future test enhancement.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few inline comment. I still need to test it, but I'll wait until you address my main feedback about the order of operations before I do too much testing.

private static final Color BOTTOM_COLOR = Color.LIME;
private static final Color TOP_COLOR = Color.RED;

private static final double TOLERANCE = 0.07;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This constant, with the same value of 0.07, is repeated in most of our robot screen capture tests. It seems a good candidate for moving it to the VisualTestBase parent class, as a future test enhancement.

import static junit.framework.Assert.assertTrue;
import test.robot.testharness.VisualTestBase;

import static test.util.Util.TIMEOUT;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the static import in this case. More to the point, this is what most other tests do.

Comment on lines 104 to 111
setupStages(true);

runAndWait(() -> {
Color color = getColor(200, 200);
assertColorEquals(TOP_COLOR, color, TOLERANCE);

topStage.setIconified(true);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will show the stage before setting it as iconified. Testing correct behavior for a stage that is initially iconified/maximized/fullScreen prior to showing the stage is the main purpose of this test enhancement, so you will need to do something like split the creation and showing of the stages into separate methods or pass flags for the initial state of those three attributes as arguments to the setupStages method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like testing both pre- and post-show behaviors would be the best option. I'll expand this to test both paths.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The post-show behavior is already covered by other tests (e.g., IconifyTest). It wouldn't hurt to provide additional tests if easy, but it's not really necessary.


// wait a little bit between getColor() calls - on macOS the below one
// would fail without this wait
sleep(100);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. This is a small sleep that isn't intended to wait for idle.

Color color = getColor(200, 200);
assertColorEquals(BOTTOM_COLOR, color, TOLERANCE);

topStage.setFullScreen(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about needing to set fullScreen before showing topStage.

Color color = getColor(200, 200);
assertColorEquals(BOTTOM_COLOR, color, TOLERANCE);

topStage.setMaximized(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about needing to set maximized before showing topStage.

@lukostyra
Copy link
Contributor Author

I addressed review remarks. I noticed following behaviors when testing the change:

  • On Windows all tests pass
  • On macOS testMaximizedStageBeforeShow and testIconifiedStageBeforeShow fail because stages are not shown already iconified/maximized. Rest of the tests work fine - I assume a bug in functionality that needs to be fixed.
  • On Linux all tests fail, on my VM (Fedora 38 with X11) the stages are created without taking into account WIDTH/HEIGHT parameters provided to the Scene. They are created smaller than expected (I guess 100x100), so getColor() calls return background colors (terminal background/letters in my case). This could be cured in-test of course, but it also seems like a bug that should be ironed out (we do expect stages to be larger after all). After forcing proper width/height, testMaximizedStageBeforeShow/testFullScreenStageBeforeShow both fail - I noticed top stage is created maximized/fullscreen and immediately brought back to default dimensions, so I think it's another to-be-fixed bug.

@kevinrushforth
Copy link
Member

  • On macOS testMaximizedStageBeforeShow and testIconifiedStageBeforeShow fail because stages are not shown already iconified/maximized. Rest of the tests work fine - I assume a bug in functionality that needs to be fixed.

JDK-8305675 is already tracking the failure of an initially iconified window. You can use that bug ID when adding the "assumeFalse" to exclude the test on macOS.

I don't see a bug tracking the failure of a maximized window. JDK-8253997 describes a problem where the window is shown normal size and then animated to maximized. Perhaps the behavior has changed on recent macOS systems. Can you check by running that test program attached to that bug? If it behaves the same way as your new test, then we can repurpose that bug; otherwise, please file a new bug.

  • On Linux all tests fail, on my VM (Fedora 38 with X11) the stages are created without taking into account WIDTH/HEIGHT parameters provided to the Scene. They are created smaller than expected (I guess 100x100), so getColor() calls return background colors (terminal background/letters in my case). This could be cured in-test of course, but it also seems like a bug that should be ironed out (we do expect stages to be larger after all). After forcing proper width/height, testMaximizedStageBeforeShow/testFullScreenStageBeforeShow both fail - I noticed top stage is created maximized/fullscreen and immediately brought back to default dimensions, so I think it's another to-be-fixed bug.

Yes, it does seem like there are two or three product bugs on Linux. Can you file new bugs, and then use those bug IDs to exclude the failing tests on Linux? I'll test this on Ubuntu and see if that behaves differently than Fedora.

@lukostyra
Copy link
Contributor Author

I don't see a bug tracking the failure of a maximized window. JDK-8253997 describes a problem where the window is shown normal size and then animated to maximized

I found out this is a problem when trying to call setMaximized(true) after establishing Stage position with setX() and setY(). Seems like a separate issue, I will file a bug with the test program I just made (also it is macOS exclusive, Windows handles this just fine).

Yes, it does seem like there are two or three product bugs on Linux. Can you file new bugs, and then use those bug IDs to exclude the failing tests on Linux? I'll test this on Ubuntu and see if that behaves differently than Fedora.

Will do. I'll also attach test programs for these.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Windows 10 system, several tests fail for me:

StageAttributesTest > testFullScreenStageBeforeShow FAILED
    junit.framework.AssertionFailedError: expected:rgba(0,255,0,255) but was:rgba(42,244,71,255)
        at test.robot.testharness.VisualTestBase.assertColorEquals(VisualTestBase.java:173)
        at test.robot.javafx.stage.StageAttributesTest.lambda$testFullScreenStageBeforeShow$17(StageAttributesTest.java:298)

StageAttributesTest > testIconifiedStageBeforeShow FAILED
    junit.framework.AssertionFailedError: expected:rgba(0,255,0,255) but was:rgba(28,248,48,255)
        at test.robot.testharness.VisualTestBase.assertColorEquals(VisualTestBase.java:173)
        at test.robot.javafx.stage.StageAttributesTest.lambda$testIconifiedStageBeforeShow$12(StageAttributesTest.java:226)

StageAttributesTest > testMaximizedStage FAILED
    junit.framework.AssertionFailedError: expected:rgba(0,255,0,255) but was:rgba(23,249,38,255)
        at test.robot.testharness.VisualTestBase.assertColorEquals(VisualTestBase.java:173)
        at test.robot.javafx.stage.StageAttributesTest.lambda$testMaximizedStage$6(StageAttributesTest.java:147)

StageAttributesTest > testFullScreenStage FAILED
    junit.framework.AssertionFailedError: expected:rgba(0,255,0,255) but was:rgba(28,248,48,255)
        at test.robot.testharness.VisualTestBase.assertColorEquals(VisualTestBase.java:173)
        at test.robot.javafx.stage.StageAttributesTest.lambda$testFullScreenStage$9(StageAttributesTest.java:185)

StageAttributesTest > testIconifiedStage FAILED
    junit.framework.AssertionFailedError: expected:rgba(255,0,0,255) but was:rgba(177,84,13,255)
        at test.robot.testharness.VisualTestBase.assertColorEquals(VisualTestBase.java:173)
        at test.robot.javafx.stage.StageAttributesTest.lambda$testIconifiedStage$4(StageAttributesTest.java:122)

StageAttributesTest > testMaximizedStageBeforeShow FAILED
    junit.framework.AssertionFailedError: expected:rgba(0,255,0,255) but was:rgba(23,249,39,255)
        at test.robot.testharness.VisualTestBase.assertColorEquals(VisualTestBase.java:173)
        at test.robot.javafx.stage.StageAttributesTest.lambda$testMaximizedStageBeforeShow$14(StageAttributesTest.java:258)

I tracked this down to a missing sleep in setupStages (see inline comments).

if (topShown) {
assertTrue("Timeout waiting for top stage to be shown",
topShownLatch.await(TIMEOUT, TimeUnit.MILLISECONDS));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several tests fail for me without an additional sleep. I recommend adding sleep(1000) here.

Comment on lines 127 to 128
// wait a bit to let window system animate the change
Util.waitForIdle(topScene);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In looking at this more closely, this change should be reverted back to using sleep(1000). Util.waitForIdle is intended to wait for scene graph changes to be propagated, not for platform windowing operations to settle down, so using it could lead to intermittent failures.

@lukostyra
Copy link
Contributor Author

Added both the sleep(1000) at the end of setupStages and replaced waitForIdle back to sleeps.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@kevinrushforth
Copy link
Member

@andy-goryachev-oracle Did you want a change to review this?

@openjdk
Copy link

openjdk bot commented Sep 20, 2023

@lukostyra This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: angorya, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 6 new commits pushed to the master branch:

  • fa73e0d: 8316135: Create release notes for JavaFX 21
  • 2ae8c27: 8255079: RobotTest::testPixelCaptureAverage fails intermittently on Windows with HiDPI scaling
  • 5e145cc: 8199216: Quadratic layout time with nested nodes and pseudo-class in style sheet
  • f185974: 8251240: Menus inaccessible on Linux with i3 wm
  • f7b21e5: 8315074: Possible null pointer access in native glass
  • 7c8dd1e: 8315958: Missing range checks in GlassPasteboard

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Sep 20, 2023
Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double checked on macOS, looks good.
thanks!

@lukostyra
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Sep 21, 2023

Going to push as commit 658c833.
Since your change was applied there have been 6 commits pushed to the master branch:

  • fa73e0d: 8316135: Create release notes for JavaFX 21
  • 2ae8c27: 8255079: RobotTest::testPixelCaptureAverage fails intermittently on Windows with HiDPI scaling
  • 5e145cc: 8199216: Quadratic layout time with nested nodes and pseudo-class in style sheet
  • f185974: 8251240: Menus inaccessible on Linux with i3 wm
  • f7b21e5: 8315074: Possible null pointer access in native glass
  • 7c8dd1e: 8315958: Missing range checks in GlassPasteboard

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 21, 2023
@openjdk openjdk bot closed this Sep 21, 2023
@openjdk openjdk bot removed the ready Ready to be integrated label Sep 21, 2023
@openjdk openjdk bot removed the rfr Ready for review label Sep 21, 2023
@openjdk
Copy link

openjdk bot commented Sep 21, 2023

@lukostyra Pushed as commit 658c833.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@lukostyra lukostyra deleted the stage_attributes_test-8298500 branch November 28, 2023 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
3 participants