[Rev 01] RFR: 8231692: Test Infrastructure: enhance KeyEventFirer to inject keyEvents into scene

[Jeanette Winzenburg]

On Thu, 7 Nov 2019 07:03:47 GMT, Ajit Ghaisas <aghaisas at openjdk.org> wrote:

> On Tue, 5 Nov 2019 15:43:03 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:
> 
>> The pull request has been updated with additional changes.
>> 
>> ----------------
>> 
>> Added commits:
>>  - 0366a0a5: added copyright header
>> 
>> Changes:
>>   - all: https://git.openjdk.java.net/jfx/pull/20/files
>>   - new: https://git.openjdk.java.net/jfx/pull/20/files/aabea139..0366a0a5
>> 
>> Webrevs:
>>  - full: https://webrevs.openjdk.java.net/jfx/20/webrev.01
>>  - incr: https://webrevs.openjdk.java.net/jfx/20/webrev.00-01
>> 
>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8231692
>>   Stats: 22 lines in 1 file changed: 21 ins; 0 del; 1 mod
>>   Patch: https://git.openjdk.java.net/jfx/pull/20.diff
>>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/20/head:pull/20
> 
> modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/infrastructure/KeyEventFirerTest.java line 204:
> 
>> 203:     public void setup() {
>> 204:         // This step is not needed (Just to make sure StubToolkit is loaded into VM)
>> 205:         // @SuppressWarnings("unused")
> 
> Remove this commented code.
> 
> modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/infrastructure/KeyEventFirerTest.java line 55:
> 
>> 54:  * Most of these tests are meant to document how to use the KeyEventFirer and what
>> 55:  * happens if used incorrectly. The latter are ignored, because the build should pass.
>> 56:  *
> 
> I see ignored tests - for false greens.
> As these tests are written for new code in KeyEventFirer test infrastructure class, I suggest not to ignore these tests. Rather adjust asserts to pass them. A comment explaining the expected result should be added.
> 
> modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/infrastructure/KeyEventFirer.java line 85:
> 
>> 84: 
>> 85:     public void doUpArrowPress(KeyModifier... modifiers) {
>> 86:         doKeyPress(KeyCode.UP, modifiers);
> 
> Enclose in braces {}

yeah, suspected that ignoring a test is not the right thingy to do (though there are precedences in the code base, that you are currently cleaning up ;) 

But couldn't come up with a passing test that demonstrates the failure of a false-green. Any ideas? Or alternatively: the basic idea in these ignored tests was to document the correct vs. incorrect useage - is there any place/internal documentation to reach that goal, other than a failing test? Maybe it's only me, but it took me quite a while to find out why that darn test (on comboBox) was _not_ failing, that is was a real false-green - which I would like to spare future contributors.

PR: https://git.openjdk.java.net/jfx/pull/20