8255337: [TestBug] Controls unit tests - ButtonTest and ComboBoxTest - log ClassCastException #337

aghaisas · 2020-10-23T11:35:50Z

This is a test fix.

Root cause:

For ButtonTest - Incorrect use of MouseEventFirer - which puts Button on to the stage and shows it before firing mouse event
For ComboBoxTest - adding ComboBox to a Stage, not showing the Stage but trying to show the ComboBox

Fix :

For ButtonTest - Moved MouseEventFirer usage from test class to the 2 tests that need it
For ComboBoxTest - Started using StageLoader (which adds comboBox to the stage and shows it) instead of separate Scene and Stage creation.

I have attached the logs captured before and after this fix to the JBS.

Progress

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

Issue

JDK-8255337: [TestBug] Controls unit tests - ButtonTest and ComboBoxTest - log ClassCastException

Reviewers

Jeanette Winzenburg (@kleopatra - Committer)
Kevin Rushforth (@kevinrushforth - Reviewer)

Download

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

bridgekeeper · 2020-10-23T11:36:27Z

👋 Welcome back aghaisas! 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.

mlbridge · 2020-10-23T11:41:44Z

Webrevs

01: Full - Incremental (4826d81)
00: Full (8a6fc1c)

kleopatra · 2020-10-23T15:44:43Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/ButtonTest.java

+
+        mouse.dispose();
    }


just curious: do we have to dispose the mouseFirer? If so, that pattern isn't safe because it will not happen if the test fails.

Hmm.. very good point. I believe, we should use dispose mechanism whenever available for a readable cleanup. Both classes MouseEventFirer & StageLoader provide dispose method and the user of these classes should use it.
Now, this can be handled in two ways -

Keep a reference at test class level - it will be null by default - individual tests will create the object and update the class reference - we can dispose in cleanup method after the test

Use Try blocks in tests that use MouseEventFirerer or StageLoader objects and dispose them in finally{} blocks.

We can use (1) if many tests in a test class use MouseEventFirerer or StageLoader objects - for example ComboBoxTest and use approach (2) if very few tests use these classes - for example ButtonTest.

makes sense :)

just looked at MouseEventFirer (a bit late, sry ;): all it does is to dispose the Stageloader it created (if it received a bare node) - so if we add a node to a stage and/or wrap it into our own loader, we don't really need to dispose the firer (though it doesn't hurt except a very minimal code clutter)

If we use our own loader - we would need to dispose that as well :)

Also, I found approach (1) of using reference at test class level and cleaning it up in cleanup method after the test is more suitable as in ButtonTest, the assertion happens in event handler. Wrapping it in try..finally will need additional code.
I will push the changes in a moment.

nit-picking: we already do dispose our own loader (or cleanup our stage), don't we :)

the change looks good

Can you please mark it as Approved?

kleopatra · 2020-10-23T15:47:13Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java

-        Stage stage = new Stage();
-        Scene scene = new Scene(comboBox);
-        stage.setScene(scene);
+        StageLoader sl = new StageLoader(comboBox);
+



good catch :) But wouldn't show the stage have the same effect? (It's just me having a personal dislike of stageloader :)

stage.show() has the same effect of fixing the bug. In fact, I fixed it initially with exact this fix.

When I looked at the other tests in ComboBoxTest class, I saw that there is a pattern of creating a Stage, creating a Scene with ComboBox and then adding Scene to the Stage. What was missed was stage.show(). The StageLoader does exactly these steps (including the missed stage.show()).
StageLoader might be a misfit at some of the other places in our tests (I believe that is the reason for your dislike), but I feel using StageLoader is a better fit in ComboBoxTest.

thanks!

Wondering as to when/why Stageloader might be a misfit? I completely gave up on using it a while ago, so don't really remember *cough

It is not that StageLoader is bad as such. By misfit - I meant - error prone to be used in tests that already use their own Scene and Stage creation.

ahh .. I see, thanks :)

kleopatra · 2020-10-23T15:47:53Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java

+        sl.dispose();
    }


just for completeness: same as mouse.dispose above :)

kleopatra

fix looks good to me - approving, but probably will need a "real" reviewer :)

kevinrushforth

Looks good.

openjdk · 2020-10-27T21:59:02Z

@aghaisas 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:

8255337: [TestBug] Controls unit tests - ButtonTest and ComboBoxTest - log ClassCastException

Reviewed-by: fastegal, 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 5 new commits pushed to the master branch:

2c67555: 8213573: MouseLocationOnScreenTest fails intermittently
243b1a5: 8255241: [TestBug] Re-enable few ignored tests in javafx.controls module that pass with latest code
4e5f0e6: 8252596: [TESTBUG] WebPageShim::paint is not thread-safe
a5a71d1: 8247494: Test failure in ImageRaceTest on some systems

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.

aghaisas · 2020-10-28T02:19:45Z

/integrate

openjdk · 2020-10-28T02:22:18Z

@aghaisas Since your change was applied there have been 5 commits pushed to the master branch:

2c67555: 8213573: MouseLocationOnScreenTest fails intermittently
243b1a5: 8255241: [TestBug] Re-enable few ignored tests in javafx.controls module that pass with latest code
4e5f0e6: 8252596: [TESTBUG] WebPageShim::paint is not thread-safe
a5a71d1: 8247494: Test failure in ImageRaceTest on some systems

Your commit was automatically rebased without conflicts.

Pushed as commit 690d266.

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

8255337-fix-test-log-exceptions

8a6fc1c

openjdk bot added the rfr Ready for review label Oct 23, 2020

kleopatra reviewed Oct 23, 2020

View reviewed changes

review_fixes

4826d81

kleopatra approved these changes Oct 27, 2020

View reviewed changes

kevinrushforth approved these changes Oct 27, 2020

View reviewed changes

openjdk bot added the ready Ready to be integrated label Oct 27, 2020

openjdk bot closed this Oct 28, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Oct 28, 2020

aghaisas deleted the 8255337-ButtonTest-ComboBoxTest_log_fix branch July 19, 2021 05:20

8255337: [TestBug] Controls unit tests - ButtonTest and ComboBoxTest - log ClassCastException #337

8255337: [TestBug] Controls unit tests - ButtonTest and ComboBoxTest - log ClassCastException #337

Uh oh!

Conversation

aghaisas commented Oct 23, 2020 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Download

Uh oh!

bridgekeeper bot commented Oct 23, 2020

Uh oh!

mlbridge bot commented Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kleopatra left a comment

Choose a reason for hiding this comment

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aghaisas commented Oct 28, 2020

Uh oh!

openjdk bot commented Oct 28, 2020

Uh oh!

Uh oh!

aghaisas commented Oct 23, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 23, 2020 •

edited

Loading

openjdk bot commented Oct 27, 2020 •

edited

Loading