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

Convert Shortcut to os-specific shortcut key #320

Merged
merged 2 commits into from Dec 3, 2016

Conversation

Projects
None yet
2 participants
@JordanMartinez
Contributor

JordanMartinez commented Dec 2, 2016

Should fix #290, but I'm not exactly sure how it should be tested...

@JordanMartinez

This comment has been minimized.

Show comment
Hide comment
@JordanMartinez

JordanMartinez Dec 2, 2016

Contributor

Also, when I ran gradle test, this test failed on my system

org.testfx.cases.acceptance.ApplicationStartTest > should_click_on_button FAILED
    java.lang.AssertionError at ApplicationStartTest.java:71

Any idea why?

Linux Mint 18

java version "1.8.0_111"
Java(TM) SE Runtime Environment (build 1.8.0_111-b14)
Java HotSpot(TM) 64-Bit Server VM (build 25.111-b14, mixed mode)
Contributor

JordanMartinez commented Dec 2, 2016

Also, when I ran gradle test, this test failed on my system

org.testfx.cases.acceptance.ApplicationStartTest > should_click_on_button FAILED
    java.lang.AssertionError at ApplicationStartTest.java:71

Any idea why?

Linux Mint 18

java version "1.8.0_111"
Java(TM) SE Runtime Environment (build 1.8.0_111-b14)
Java HotSpot(TM) 64-Bit Server VM (build 25.111-b14, mixed mode)
@brcolow

This comment has been minimized.

Show comment
Hide comment
@brcolow

brcolow Dec 2, 2016

Collaborator

Does it fail consistently or is it flaky? As for how to test this, I will think about it and try and come up with something.

Collaborator

brcolow commented Dec 2, 2016

Does it fail consistently or is it flaky? As for how to test this, I will think about it and try and come up with something.

@JordanMartinez

This comment has been minimized.

Show comment
Hide comment
@JordanMartinez

JordanMartinez Dec 2, 2016

Contributor

It's failed all 3 times I've tried it.

Contributor

JordanMartinez commented Dec 2, 2016

It's failed all 3 times I've tried it.

@JordanMartinez

This comment has been minimized.

Show comment
Hide comment
@JordanMartinez

JordanMartinez Dec 2, 2016

Contributor

@brcolow any ideas for how I should test this? I could use a modification of the example you provided in #319 to change a TextField's text if the correct shortcut key is pressed. But if I do that, should the test still go in RobotImplTest.java? Or in the same package as KeysAndButtonsReleasedTest.java?

Contributor

JordanMartinez commented Dec 2, 2016

@brcolow any ideas for how I should test this? I could use a modification of the example you provided in #319 to change a TextField's text if the correct shortcut key is pressed. But if I do that, should the test still go in RobotImplTest.java? Or in the same package as KeysAndButtonsReleasedTest.java?

@brcolow

This comment has been minimized.

Show comment
Hide comment
@brcolow

brcolow Dec 2, 2016

Collaborator

I don't feel strongly as to which file it belongs in...I guess I would say RobotImplTest but again, I don't feel strongly on that. I think what you said is a good idea/plan.

Collaborator

brcolow commented Dec 2, 2016

I don't feel strongly as to which file it belongs in...I guess I would say RobotImplTest but again, I don't feel strongly on that. I think what you said is a good idea/plan.

@JordanMartinez

This comment has been minimized.

Show comment
Hide comment
@JordanMartinez

JordanMartinez Dec 2, 2016

Contributor

Personally, I think it should stay in its own package as it tests BaseRobotImpl, which is in a different package altogether. However, since I don't know this code base very well yet, it is easier to implement it the test using ApplicationTest.

Contributor

JordanMartinez commented Dec 2, 2016

Personally, I think it should stay in its own package as it tests BaseRobotImpl, which is in a different package altogether. However, since I don't know this code base very well yet, it is easier to implement it the test using ApplicationTest.

@brcolow

This comment has been minimized.

Show comment
Hide comment
@brcolow

brcolow Dec 2, 2016

Collaborator

In this case, I don't think it's necessary to force the method order to be ascending, as it is not testing the sequence of the two tests, is it? In other words, can the tests run in any order (A -> B, B ->A) and the outcome be the same? If so, we should remove the explicit ordering as ideally unit tests should be able to run in any order.

Collaborator

brcolow commented Dec 2, 2016

In this case, I don't think it's necessary to force the method order to be ascending, as it is not testing the sequence of the two tests, is it? In other words, can the tests run in any order (A -> B, B ->A) and the outcome be the same? If so, we should remove the explicit ordering as ideally unit tests should be able to run in any order.

@JordanMartinez

This comment has been minimized.

Show comment
Hide comment
@JordanMartinez

JordanMartinez Dec 2, 2016

Contributor

True. I forced the method order because I was going to use press(SHORTCUT) in the second one before later changing it to the OS-specific one. Give me a sec.

Contributor

JordanMartinez commented Dec 2, 2016

True. I forced the method order because I was going to use press(SHORTCUT) in the second one before later changing it to the OS-specific one. Give me a sec.

@JordanMartinez

This comment has been minimized.

Show comment
Hide comment
@JordanMartinez

JordanMartinez Dec 2, 2016

Contributor

The 2nd test fails on my side now.

Contributor

JordanMartinez commented Dec 2, 2016

The 2nd test fails on my side now.

@brcolow

This comment has been minimized.

Show comment
Hide comment
@brcolow

brcolow Dec 2, 2016

Collaborator

I see two lint failures regarding unused imports...but there are also test failures on your end?

Collaborator

brcolow commented Dec 2, 2016

I see two lint failures regarding unused imports...but there are also test failures on your end?

@JordanMartinez

This comment has been minimized.

Show comment
Hide comment
@JordanMartinez

JordanMartinez Dec 3, 2016

Contributor

Yup:

org.testfx.framework.junit.ShortcutKeyTest > shortcut_keyCode_converts_to_OS_specific_keyCode_when_released FAILED
    java.lang.AssertionError: 
    Expected: "keyReleased"
         but: was "keyPressed"
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
        at org.testfx.framework.junit.ShortcutKeyTest.shortcut_keyCode_converts_to_OS_specific_keyCode_when_released(ShortcutKeyTest.java:83)
Contributor

JordanMartinez commented Dec 3, 2016

Yup:

org.testfx.framework.junit.ShortcutKeyTest > shortcut_keyCode_converts_to_OS_specific_keyCode_when_released FAILED
    java.lang.AssertionError: 
    Expected: "keyReleased"
         but: was "keyPressed"
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
        at org.testfx.framework.junit.ShortcutKeyTest.shortcut_keyCode_converts_to_OS_specific_keyCode_when_released(ShortcutKeyTest.java:83)
@brcolow

This comment has been minimized.

Show comment
Hide comment
@brcolow

brcolow Dec 3, 2016

Collaborator

What if you add a:

@Before
public void clearTextField() {
    field.clear();
}

Also, instead of doing:

field.addEventHandler(KeyEvent.KEY_PRESSED, and field.addEventHandler(KeyEvent.KEY_RELEASED
it might be more readble to do:

field.setOnKeyPressed(...) and field.setOnKeyReleased(..)

Collaborator

brcolow commented Dec 3, 2016

What if you add a:

@Before
public void clearTextField() {
    field.clear();
}

Also, instead of doing:

field.addEventHandler(KeyEvent.KEY_PRESSED, and field.addEventHandler(KeyEvent.KEY_RELEASED
it might be more readble to do:

field.setOnKeyPressed(...) and field.setOnKeyReleased(..)

@JordanMartinez

This comment has been minimized.

Show comment
Hide comment
@JordanMartinez

JordanMartinez Dec 3, 2016

Contributor

Ah... I see what's the issue... KeyboardRobotImpl holds the keysPressed, right? So when I call press(osSpecificKey), it stores the specific key. When I call release(SHORTCUT), it doesn't contain the shortcut keycode, so it does nothing.

Contributor

JordanMartinez commented Dec 3, 2016

Ah... I see what's the issue... KeyboardRobotImpl holds the keysPressed, right? So when I call press(osSpecificKey), it stores the specific key. When I call release(SHORTCUT), it doesn't contain the shortcut keycode, so it does nothing.

@brcolow

This comment has been minimized.

Show comment
Hide comment
@brcolow

brcolow Dec 3, 2016

Collaborator

Ahhh, yes, good catch! When release(SHORTCUT) is pressed, maybe we should check if keysPressed contains the OS-specific key (either command or control) instead?

Collaborator

brcolow commented Dec 3, 2016

Ahhh, yes, good catch! When release(SHORTCUT) is pressed, maybe we should check if keysPressed contains the OS-specific key (either command or control) instead?

@JordanMartinez

This comment has been minimized.

Show comment
Hide comment
@JordanMartinez

JordanMartinez Dec 3, 2016

Contributor

Yes, I think we should. Otherwise, someone might accidentally do what I just did.

Contributor

JordanMartinez commented Dec 3, 2016

Yes, I think we should. Otherwise, someone might accidentally do what I just did.

@JordanMartinez

This comment has been minimized.

Show comment
Hide comment
@JordanMartinez

JordanMartinez Dec 3, 2016

Contributor

These check style tests.... Sheez!

Ok. It built successfully on my side.

Contributor

JordanMartinez commented Dec 3, 2016

These check style tests.... Sheez!

Ok. It built successfully on my side.

@JordanMartinez

This comment has been minimized.

Show comment
Hide comment
@JordanMartinez

JordanMartinez Dec 3, 2016

Contributor

I also moved the conversion step from BaseRobotImpl to KeyboardRobotImpl.

Contributor

JordanMartinez commented Dec 3, 2016

I also moved the conversion step from BaseRobotImpl to KeyboardRobotImpl.

@brcolow brcolow merged commit bbd221f into TestFX:master Dec 3, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@brcolow

This comment has been minimized.

Show comment
Hide comment
@brcolow

brcolow Dec 3, 2016

Collaborator

Awesome. Looks good. I know the Checkstyle checks are a bit excessive, but having a thorough and enforced style guide is worth the trade-off, at least, I think so :).

Thanks for your patience and diligence! Merged :).

Collaborator

brcolow commented Dec 3, 2016

Awesome. Looks good. I know the Checkstyle checks are a bit excessive, but having a thorough and enforced style guide is worth the trade-off, at least, I think so :).

Thanks for your patience and diligence! Merged :).

@JordanMartinez

This comment has been minimized.

Show comment
Hide comment
@JordanMartinez

JordanMartinez Dec 3, 2016

Contributor

Excessive? YES! 😆

I think the real issue here was not knowing about those tests and not realizing how that should affect my work before I push it to this PR as I had to amend the commit multiple times.
Having that expectation now helps put up with the excess tests because I agree, it does help with reading.

Contributor

JordanMartinez commented Dec 3, 2016

Excessive? YES! 😆

I think the real issue here was not knowing about those tests and not realizing how that should affect my work before I push it to this PR as I had to amend the commit multiple times.
Having that expectation now helps put up with the excess tests because I agree, it does help with reading.

@JordanMartinez JordanMartinez deleted the JordanMartinez:shortcutKey branch Dec 3, 2016

@brcolow

This comment has been minimized.

Show comment
Hide comment
@brcolow

brcolow Dec 3, 2016

Collaborator

Some of the annoyance can be mitigated by (if you use IntelliJ IDEA IDE) using the checkstyle-idea plugin. It allows you to setup a profile of rules (which is done by selecting the checkstyle rules file TestFX/gradle/checkstyle.xml. Then you can run Checkstyle against the current file, or even the entire project.

Reminder to myself to add the above suggestion to https://github.com/TestFX/TestFX/wiki/How-to-Contribute

Collaborator

brcolow commented Dec 3, 2016

Some of the annoyance can be mitigated by (if you use IntelliJ IDEA IDE) using the checkstyle-idea plugin. It allows you to setup a profile of rules (which is done by selecting the checkstyle rules file TestFX/gradle/checkstyle.xml. Then you can run Checkstyle against the current file, or even the entire project.

Reminder to myself to add the above suggestion to https://github.com/TestFX/TestFX/wiki/How-to-Contribute

@JordanMartinez

This comment has been minimized.

Show comment
Hide comment
@JordanMartinez

JordanMartinez Dec 3, 2016

Contributor

Gasp! Lifesaver!

Hmm... Should have read through that guide so my commit messages better adhere to that style.

Contributor

JordanMartinez commented Dec 3, 2016

Gasp! Lifesaver!

Hmm... Should have read through that guide so my commit messages better adhere to that style.

@JordanMartinez

This comment has been minimized.

Show comment
Hide comment
@JordanMartinez

JordanMartinez Dec 3, 2016

Contributor

Reminder to myself to add the above suggestion to https://github.com/TestFX/TestFX/wiki/How-to-Contribute

I added a small tip in the check style section. It's at least something now that you can modify/improve later.

Contributor

JordanMartinez commented Dec 3, 2016

Reminder to myself to add the above suggestion to https://github.com/TestFX/TestFX/wiki/How-to-Contribute

I added a small tip in the check style section. It's at least something now that you can modify/improve later.

@brcolow

This comment has been minimized.

Show comment
Hide comment
@brcolow

brcolow Dec 3, 2016

Collaborator

Awesome, I re-worked it a bit. Thanks again.

Collaborator

brcolow commented Dec 3, 2016

Awesome, I re-worked it a bit. Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment