Refactor SyntheticKeyboardEvent tests to only use the public API #11631
Conversation
| cancelable: true, | ||
| }); | ||
| container.firstChild.dispatchEvent(nativeEvent); | ||
| expect(charCode).toBe(13); |
There was a problem hiding this comment.
I expect charCode at this point to be 13 but get this message.
I'm wondering if there is a difference between the keydown and keypress event that I am missing.
| expect(syntheticEvent.target).toBe(target); | ||
| expect(syntheticEvent.type).toBe(undefined); | ||
| event = document.createEvent('Event'); | ||
| event.initEvent('keypress', true, true); |
There was a problem hiding this comment.
The EventInterface, the keypress event also seems to be not firing in the same manner as keydown and keyup.
|
|
||
| beforeEach(() => { | ||
| // Mock getEventCharCode for proper unit testing | ||
| jest.mock('../getEventCharCode'); | ||
| getEventCharCode = require('../getEventCharCode').default; |
There was a problem hiding this comment.
This is also problematic. It's importing an internal module—which is exactly the situation we're trying to avoid.
There was a problem hiding this comment.
In fact I think the tests from getEventCharCode should just be "rolled" into this test case. Since they're covering the same thing.
| container = null; | ||
| }); | ||
|
|
||
|
|
||
| describe('KeyboardEvent interface', () => { | ||
| describe('charCode', () => { | ||
| describe('when event is `keypress`', () => { | ||
| it('returns whatever getEventCharCode returns', () => { |
There was a problem hiding this comment.
A lot of these tests were written in a very unit testing centric way. You need to try to liberate from that mindset. Think what's actually valuable to test for from perspective of library user. Not all the possible inputs to internal functions that never get called with those inputs in the first place.
|
I did the changes. Hopefully this passes CI! |
|
Changes look awesome. Apologies for not being able to act faster - was busy with Thanksgiving/family obligations the past 2 days. Regardless, thank you for the feedback and the opportunity, @gaearon! |
…ebook#11631) * KeyboardEvent interface-keypress * Pass first 6 tests * Roll getEventCharCode-test into SyntheticKeyboardEvent-test * Run SyntheticKeyboardEvent-test on bundles * Remove unused code
…ebook#11631) * KeyboardEvent interface-keypress * Pass first 6 tests * Roll getEventCharCode-test into SyntheticKeyboardEvent-test * Run SyntheticKeyboardEvent-test on bundles * Remove unused code
…ebook#11631) * KeyboardEvent interface-keypress * Pass first 6 tests * Roll getEventCharCode-test into SyntheticKeyboardEvent-test * Run SyntheticKeyboardEvent-test on bundles * Remove unused code
I've been working on SyntheticKeyboardEvent test and have gotten a significant amount of tests to pass but wanted to see if the community had feedback on my work so far and advice on how to complete the few remaining tests where I am stuck.
Problem: I am unable to fire events and pass tests that require a 'keypress' event. I've looked thru other examples, specifically mdn docs and PR #11365 to fire the keypress event, specifically on line 41.
Via MDN and other PR's, I've found no significant difference on why a 'keypress' event and and the 'keydown'/'keyup' which fire as expected. @gaearon any feedback or tips would be on my approach/code appreciated 😃