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

8231692: Test Infrastructure: enhance KeyEventFirer to inject keyEvents into scene #20

Closed
wants to merge 4 commits into from

Conversation

@kleopatra
Copy link
Contributor

kleopatra commented Oct 23, 2019

The issue is that firing keyEvents on a node that is not focusOwner might produce false green tests, please see the issue for details.

fix for https://bugs.openjdk.java.net/browse/JDK-8231692

  • added contructor taking the scene
  • changed event firing to use either the target directly or inject into
    scene

Progress

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

Issue

JDK-8231692: Test Infrastructure: enhance KeyEventFirer to inject keyEvents into scene

Approvers

scene

fix for https://bugs.openjdk.java.net/browse/JDK-8231692
- added contructor taking the scene
- changed event firing to use either the target directly or inject into
scene
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 23, 2019

👋 Welcome back fastegal! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

@openjdk openjdk bot added the rfr label Oct 23, 2019
@mlbridge
Copy link

mlbridge bot commented Oct 23, 2019

@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 31, 2019

@aghaisas can you review this? A single reviewer will be sufficient.

- removed ignored tests
- code cleanup
@kleopatra
Copy link
Contributor Author

kleopatra commented Nov 7, 2019

done as requested :)

As to the ignored tests, I removed most and re-formulated one to demonstrate the difference between firing onto a not-focusOwner and firing via scene

// firing on the scene makes a difference
KeyEventFirer correctFirer = new KeyEventFirer(null, scene);
correctFirer.doKeyPress(A);
assertNotEquals(falseTextFieldNotification, textFieldEvents.size());

This comment has been minimized.

Copy link
@aghaisas

aghaisas Nov 8, 2019

assertNotEquals does not compile for me.

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Nov 8, 2019

Member

It looks like assertNotEquals was added in a later version of junit than the one we use, which is still junit 4.8.2. We should upgrade to a newer version at some point, but until then we need to avoid using anything from newer versions of junit.

This comment has been minimized.

Copy link
@kleopatra

kleopatra Nov 8, 2019

Author Contributor

*I-must-not-push-without-commandline-build, *I-must-not-push-without-commandline-build, *I-must-...

sry, guys, for wasting your time - changed

@openjdk openjdk bot removed the rfr label Nov 11, 2019
@openjdk
Copy link

openjdk bot commented Nov 11, 2019

@kleopatra This change can now be integrated. The commit message will be:

8231692: Test Infrastructure: enhance KeyEventFirer to inject keyEvents into scene

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

Since the source branch of this PR was last updated there have been 6 commits pushed to the master branch:

  • 286d1b5: 8230492: font-family not set in HTMLEditor if font name has a number in it
  • f74f3af: 8233040: ComboBoxPopupControl: remove eventFilter for F4
  • a1cc4ab: 8232210: Update Mesa 3-D Headers to version 19.2.1
  • dca8df4: 8232943: Gesture support is not initialized on iOS
  • 5a70b0c: 8189092: ArrayIndexOutOfBoundsException on Linux in getCachedGlyph
  • ac71396: 8232929: Duplicate symbols when building static libraries

Since there are no conflicts, your changes will automatically be rebased on top of the above commits when integrating. If you prefer to do this manually, please merge master into your branch first.

As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@aghaisas) but any other Committer may sponsor as well.

  • To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).
@openjdk openjdk bot added the ready label Nov 11, 2019
@kleopatra
Copy link
Contributor Author

kleopatra commented Nov 11, 2019

/integrate

@openjdk openjdk bot added the sponsor label Nov 11, 2019
@openjdk
Copy link

openjdk bot commented Nov 11, 2019

@kleopatra
Your change (at version fbdea3d) is now ready to be sponsored by a Committer.

@aghaisas
Copy link

aghaisas commented Nov 12, 2019

/sponsor

@openjdk openjdk bot closed this Nov 12, 2019
@openjdk openjdk bot added integrated and removed sponsor ready labels Nov 12, 2019
@openjdk
Copy link

openjdk bot commented Nov 12, 2019

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

  • 286d1b5: 8230492: font-family not set in HTMLEditor if font name has a number in it
  • f74f3af: 8233040: ComboBoxPopupControl: remove eventFilter for F4
  • a1cc4ab: 8232210: Update Mesa 3-D Headers to version 19.2.1
  • dca8df4: 8232943: Gesture support is not initialized on iOS
  • 5a70b0c: 8189092: ArrayIndexOutOfBoundsException on Linux in getCachedGlyph
  • ac71396: 8232929: Duplicate symbols when building static libraries

Your commit was automatically rebased without conflicts.

Pushed as commit 94bcf3f.

@mlbridge
Copy link

mlbridge bot commented Nov 12, 2019

Mailing list message from Ajit Ghaisas on openjfx-dev:

Changeset: 94bcf3f
Author: Jeanette Winzenburg
Committer: Ajit Ghaisas
Date: 2019-11-12 04:21:17 +0000
URL: https://git.openjdk.java.net/jfx/commit/94bcf3fc

8231692: Test Infrastructure: enhance KeyEventFirer to inject keyEvents into scene

Reviewed-by: aghaisas

! modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/infrastructure/KeyEventFirer.java

  • modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/infrastructure/KeyEventFirerTest.java
@kleopatra kleopatra deleted the kleopatra:bug-fix-JDK-8231692 branch Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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