Rewrite SyntheticWheelEvent-test depending on internal API (#11299) #11367
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
Can you please rebase on master, run |
|
Hmm, something got messed up. Might be easier to start a new branch off master and cherry-pick your commits on it? |
|
@gaearon done, can you take a look if everything is how expected? |
|
^^ see my reply above, we commented at the same time |
|
@gaearon sorry about the late answer, I was busy with a few jobs. I've merged the current master into my branch and I've run the prettier script, you think this works as you expected? |
| var target = document.createElement('div'); | ||
| var syntheticEvent = createEvent({srcElement: target}); | ||
| const container = document.createElement('div'); | ||
| const event = createMouseEvent('', {srcElement: container}); |
There was a problem hiding this comment.
I think this doesn't test that srcElement is used when event.target is not supported. The code just falls back to event.target because it exists. You need to do extra work to actually "break" event.target and cause the polyfill path to execute.
| which: 2, | ||
| button: 1, | ||
| }); | ||
| expect(event.button).toBe(1); |
There was a problem hiding this comment.
This doesn't test the React's synthetic event at all. It just creates a jsdom event object and then asserts that object works. This is not a useful test for us because we already know jsdom works (it has its own test suite).
We should instead test that the synthetic event object produced by React has the right fields.
| const node = ReactDOM.findDOMNode(component); | ||
|
|
||
| node.dispatchEvent(createMouseEvent('wheel', {deltaX: 10, deltaY: -50})); | ||
| expect(events[0].deltaX).toBe(10); |
There was a problem hiding this comment.
It is better to first assert the number of expected events. Then, if some are missing, the errors are clearer.
| const component = ReactDOM.render(<div onWheel={onWheel} />, container); | ||
| document.body.appendChild(container); | ||
|
|
||
| const node = ReactDOM.findDOMNode(component); |
There was a problem hiding this comment.
ReactDOM.render already returns a DOM node. There is no need to call findDOMNode on a DOM node.
|
|
||
| expect(syntheticEvent.target).toBe(target); | ||
| expect(syntheticEvent.type).toBe(undefined); |
There was a problem hiding this comment.
It wouldn't be undefined in practice. The old test was odd, but we shouldn't replicate that odd behavior in the new test :-)
|
|
||
| expect(syntheticEvent.target).toBe(target); | ||
| expect(syntheticEvent.type).toBe(undefined); | ||
| expect(event.target).toBe(container); |
There was a problem hiding this comment.
This does not test the React synthetic event system. Like below, the problem is this is testing jsdom itself. If React were to break the synthetic events completely, this test would still be happily passing.
|
I pushed changes to address my review comments. I hope they are helpful for future reference! |
…11299) (facebook#11367) * Update SyntheticWheelEvent tests to use public API * Replace: ReactTestUtils.SimulateNative to native Events() * Update: Replaced WheelEvent() interface to document.createEvent * Fix: Lint SyntheticWheelEvent file * Update: Custom WheelEvent function to a generic MouseEvent function * Update: Prettier SyntheticWheelEvent-test.js * Verify the `button` property is set on synthetic event * Use MouseEvent constructor over custom helper * Rewrite to test React rather than jsdom * Force the .srcElement code path to execute * Style tweaks and slight code reorganization
Rewrite SyntheticWheelEvent-test to remove internal API dependencies based in issue #11299