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

8239822: Intermittent unit test failures in RegionCSSTest #124

Closed

Conversation

@kevinrushforth
Copy link
Member

kevinrushforth commented Feb 22, 2020

This is a fix for an intermittent test failure affecting test.javafx.scene.layout.RegionCSSTest. This turns out to be a test bug in test.javafx.scene.CssStyleHelperTest, which was added as part of JDK-8237469. RegionCSSTest is a victim of this bug.

Except for the systemTests project, all unit tests are run in the same JVM, so each test class must ensure that any modified global state is restored after the tests are run. The CssStyleHelperTest leaves a userAgentStyleSheet set, which is a global (per Application) attribute, so any subsequent tests will be run with that stylesheet set. If the last test that is run in CssStyleHelperTest sets a style sheet with "-fx-background" then it will cause assertion failures in 78 of the tests in RegionCSSTest.

The fix is to cleanup the global userAgentStyleSheet, along with any other state that is set, in an @AfterClass cleanup method.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8239822: Intermittent unit test failures in RegionCSSTest

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jfx pull/124/head:pull/124
$ git checkout pull/124

@kevinrushforth kevinrushforth self-assigned this Feb 22, 2020
@bridgekeeper
Copy link

bridgekeeper bot commented Feb 22, 2020

👋 Welcome back kcr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label Feb 22, 2020
@kevinrushforth
Copy link
Member Author

kevinrushforth commented Feb 22, 2020

@aghaisas Can you review this?

@DeanWookey You are also welcome to review it if you like.

@mlbridge
Copy link

mlbridge bot commented Feb 22, 2020

Webrevs

@DeanWookey
Copy link
Collaborator

DeanWookey commented Feb 22, 2020

test.javafx.css.Node_cssStateTransition_Test may also need the same treatment.

@kevinrushforth
Copy link
Member Author

kevinrushforth commented Feb 24, 2020

Thanks for pointing that out. In addition to test.javafx.css.Node_cssStateTransition_Test, the following test has a similar pattern:

test.com.sun.javafx.css.StyleManagerTest

I'll update the PR to fix the above two.

I looked further, and see that the following tests call StyleManager::setDefaultUserAgentStylesheet:

test/javafx/css/CssMetaDataTest
test/javafx/css/HonorDeveloperSettingsTest
test/javafx/css/Node_cssStateTransition_Test
test/javafx/css/Node_cssStyleMap_Test
test/javafx/scene/Node_effectiveOrientation_Css_Test

This is a slightly different case, and none of these are causing problems today, so I will file a follow-up test bug to look at those and see if they need cleanup.

@kevinrushforth
Copy link
Member Author

kevinrushforth commented Feb 24, 2020

I filed JDK-8239880 to track fixing the above 5 test classes.

Copy link
Collaborator

DeanWookey left a comment

Looks good to me. Just the dates in the copyright headers need to be updated.

@openjdk
Copy link

openjdk bot commented Feb 25, 2020

@kevinrushforth This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8239822: Intermittent unit test failures in RegionCSSTest

Reviewed-by: aghaisas
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 2 commits pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 31505620ad5fc49580ae7e7241a1edcd6d1f2e30.

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

@openjdk openjdk bot added the ready label Feb 25, 2020
@kevinrushforth
Copy link
Member Author

kevinrushforth commented Feb 25, 2020

Looks good to me. Just the dates in the copyright headers need to be updated.

They will get updated when I update copyrights of files modified this year to 2020 (with automated scripts) with JDK-8237504.

@kevinrushforth
Copy link
Member Author

kevinrushforth commented Feb 25, 2020

/integrate

@openjdk openjdk bot closed this Feb 25, 2020
@openjdk openjdk bot added integrated and removed ready labels Feb 25, 2020
@openjdk
Copy link

openjdk bot commented Feb 25, 2020

@kevinrushforth The following commits have been pushed to master since your change was applied:

  • 3150562: 8235772: Remove use of deprecated finalize method from PiscesRenderer and AbstractSurface
  • 4eaff0d: 8239109: Update SQLite to version 3.31.1

Your commit was automatically rebased without conflicts.

Pushed as commit c3ee1a3.

@openjdk openjdk bot removed the rfr label Feb 25, 2020
@mlbridge
Copy link

mlbridge bot commented Feb 25, 2020

Mailing list message from Kevin Rushforth on openjfx-dev:

Changeset: c3ee1a3
Author: Kevin Rushforth <kcr at openjdk.org>
Date: 2020-02-25 14:01:57 +0000
URL: https://git.openjdk.java.net/jfx/commit/c3ee1a30

8239822: Intermittent unit test failures in RegionCSSTest

Reviewed-by: aghaisas

! modules/javafx.graphics/src/test/java/test/com/sun/javafx/css/StyleManagerTest.java
! modules/javafx.graphics/src/test/java/test/javafx/css/Node_cssStateTransition_Test.java
! modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java

@kevinrushforth kevinrushforth deleted the kevinrushforth:8239822-css-testbug branch Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.