Update getEventKey tests to use public API (#11299)

[mwilc0x]

Hi!

This PR is my attempt at updating the tests for getEventKey.js to use the public API. I browsed other test examples that were provided in #11299. After reading those and getEventKey.js I came up with an approach that uses Simulate and SimulateNative on the ref node and captures the synthetic event in the onKeyPressCapture and onKeyPressCapture handlers. It then tests properties on the event that was passed in.

I had some questions along the way:

• Is this the right approach?
• Am I using Simulate/SimulateNative correctly?
• Am I generating the component/ref node generated correctly?

Thank you for the contribution opportunity!

Cheers

[gaearon]

It seems like the coverage is not as complete as it used to be. For example I can completely comment out this block and the test still passes, but it shouldn’t.

[mwilc0x]

Thanks for the feedback @gaearon.

I will investigate the coverage.

I noticed that removing the require('getEventKey') removes the watch of this file when running test --watch. Wondering if it should be kept in for watching during testing?

[mwilc0x]

I'm seeing that changing the SimulateNative.keyPress here https://github.com/facebook/react/pull/11317/files#diff-472869954df6df07cd9e070a62ecc39fR39

to SimulateNative.keyDown picks up the nativeEvent.key and goes into the if (nativeEvent.key) branch. Otherwise, key is undefined on the nativeEvent for SimulateNative.keyPress when it reaches getEventKey.

But now I'm not sure how this test was working correctly before. Actually, what I am seeing is that when using SimulateNative.keyPress, the handler for onKeyPressCapture is never actually called so the test was never actually run for this spec.

[mwilc0x]

Hi @sebmarkbage, I saw your comment about not using ReactTestutils. I'm having some doubts when switching them out. I'm not sure I'm doing this correctly and would appreciate any feedback.

I can get something like this to work for the first test:

class Comp extends React.Component {
  cb = (e) => {
    this.key = e.key;
  }

  render() {
    return <input ref={n => this.a = n} onKeyDown={this.cb} />;
  }
}

var container = document.createElement('div');
var instance = ReactDOM.render(<Comp />, container);

document.body.appendChild(container);

var node = ReactDOM.findDOMNode(instance.a);

var event = document.createEvent('Event');
event.key = 'Del';
event.initEvent('keydown', true, true);

node.dispatchEvent(event);

expect(instance.key).toBe('Delete');

document.body.removeChild(container);

Does this look like the right approach?

One thing I'm seeing is that it doesn't capture in the onKeyPress handler when swapping the type for keypress though. I was trying a couple other things with CustomEvent and KeyboardEvent too but couldn't get those to work either. Something like:

var node = ReactDOM.findDOMNode(instance.a);
var nativeEvent = new KeyboardEvent('keydown', { key: 'Del' });
node.dispatchEvent(nativeEvent);

but it looks like it never calls the onKeyDown handler.

If I add an an event listener to the node, I see it's being called:

componentDidMount() {
  ReactDOM.findDOMNode(this.a).addEventListener('keydown', e => console.log(e.key));
}

[gaearon]

Does this look like the right approach?

Yes. Please change tests to use this approach (with dispatching native events) instead. You can use #11299 as inspiration.

One thing I'm seeing is that it doesn't capture in the onKeyPress handler when swapping the type for keypress though.

This might be related to this check. And it is there to normalize browser behavior which is usually to only fire keypress for printable characters (unlike Delete button).

I don't think it's super important to test keypress specifically. At least that Delete case. It's not supposed to fire in browsers for Delete.

It's nice to have at least a single test with it though. Maybe you can just test that {key: 'Whatever'} turns into Unidentified (which proves this code ran). For Delete tests please use keydown instead.

[mwilc0x]

Thanks @gaearon!

There is a test for returning Unidentified in this condition. (it's only for keydown or keyup)

Another thing, on MDN the docs say that initEvent is being deprecated. I switched over to using the Event constructor. The tests now look like this:

class Comp extends React.Component {
  cb = (e) => {
    this.key = e.key;
  }

  render() {
    return <input ref={n => this.a = n} onKeyDown={this.cb} />;
  }
}

var container = document.createElement('div');
var instance = ReactDOM.render(<Comp />, container);

document.body.appendChild(container);

var node = ReactDOM.findDOMNode(instance.a);

var event = new Event('keydown', {bubbles: true, cancelable: true});
event.key = 'Del';

node.dispatchEvent(event);

expect(instance.key).toBe('Delete');

document.body.removeChild(container);

If this looks ok, I have all of the tests switched over and working except for the last one as I'm not sure how to test it. This last test is testing unknown event types. The way these tests are now is testing event properties through the callback handlers, i.e. onKeyDown, onKeyPress.

Specifically the last test is testing the last return state when the returned key is an empty string for an unknown event type. What I am not sure about is that React doesn't handle unknown event types on components, so how to test that this condition was met?

Also, I'd like to test code coverage to make sure it's not regressed. Should the test script be run with --coverage?

[gaearon]

I switched over to using the Event constructor

This sounds fine.

Specifically the last test is testing the last return state when the returned key is an empty string for an unknown event type.

I think it's fine to remove that test if you add Flow typing to the file, with a string return type.

[gaearon]

Also, I'd like to test code coverage to make sure it's not regressed. Should the test script be run with --coverage?

Maybe. I don't know if that infrastructure works properly. Can you try and let me know?

[gaearon]

Please:

• Remove usage of Simulate and SimulateNative in favor of dispatching native events. You can use Use only public API for EnterLeaveEventPlugin Tests #11316 and Rewrite ReactDOMInput-test depending on internal API (#11299) #11309 as a reference.

• Delete the last test.

• Instead, add @flow annotation to getEventKey that verifies it always returns a string.

[mwilc0x]

Hi @gaearon, thank you for your feedback.

Here is the latest update for this PR:

• Update tests to use native events, removing Simulate and SimulateNative
• Remove last test
• Add flow annotation to getEventKey signature

One thing I ran into when adding @flow was this issue. It looks like it's been a long standing issue, I added this workaround as mentioned on the thread.

I did not get a chance to look into coverage in more detail yet.

[gaearon]

Can we just use string keys?

[mwilc0x]

Yeah, string keys work. Updated with a2adbdf.

[gaearon]

This is great. Thank you!

[mwilc0x]

Thank you @gaearon!