Rewrite SelectEventPlugin-test to test behavior using Public API (#11299) #11676
Conversation
| ); | ||
| var node = ReactDOM.findDOMNode(rendered); | ||
| document.body.appendChild(childContainer); |
There was a problem hiding this comment.
Do you mind doing this in beforeEach and then cleaning up in afterEach? Like here.
|
|
||
| node.selectionStart = 0; | ||
| node.selectionEnd = 0; | ||
| var node = ReactDOM.findDOMNode(childNode); |
There was a problem hiding this comment.
There's no need to findDOMNode. render() already gave you the right node. Old test just wasn't updated to reflect this.
|
|
||
| var focus = extract(node, 'topFocus'); |
There was a problem hiding this comment.
This seems to be testing that a focus DOM event would have been ignored at this point. Shouldn't the new test also verify this?
|
|
||
| var focus = extract(node, 'topFocus'); | ||
| expect(focus).toBe(null); | ||
| var nativeEvent = document.createEvent('Event'); |
There was a problem hiding this comment.
Let's use the newer API for this? e.g. new MouseEvent('mousedown', {bubbles: true, cancelable: true})
Use newer API when creating events
|
Is something blocking you from changing the second test? |
|
@gaearon Thank you for taking the time to review the changes. Like I mentioned on the description, the other test that has not been changed is supposed to "skip extraction if no listeners are present". This means that in order to test it we should not add any React event listeners on the element and make sure that the events were not emitted. Since React though internally does not emit the event if the listener is not present how can we verify the behavior without accessing its internals? |
|
The test was originally written in 47b147f (found it via blame). The commit description describes the issue that it was trying to solve:
Perhaps you can manage to reproduce this exact problem in a new test? You can then "undo" some parts of the fix in 47b147f to verify your code still tests it. |
|
Thank you again. I will try to reproduce it with a different test and let you know. |
|
Please let me know if you get stuck! |
|
Sorry for the late response, I rewrote the other test as well. It may look weird and I could be way wrong here, but l'm looking at the code path and it seems to me that is exactly the same. |
|
If I comment out this line on master, the test fails. But it doesn't fail in this branch. Doesn't this mean we're no longer verifying that behavior? |
|
I think what #3639 says is not that |
| }); | ||
| node.dispatchEvent(nativeEvent); | ||
|
|
||
| nativeEvent = new MouseEvent('keyup', {bubbles: true, cancelable: true}); |
There was a problem hiding this comment.
Note: keyup is not a mouse event.
|
I think the correct test would be: // See https://github.com/facebook/react/pull/3639 for details.
it('does not get confused when dependent events are registered independently', () => {
var select = jest.fn();
var onSelect = event => select(event.currentTarget);
// Pass `onMouseDown` so React registers a top-level listener.
var node = ReactDOM.render(
<input type="text" onMouseDown={function() {}} />,
container,
);
node.focus();
// Trigger `mousedown` and `mouseup`. Note that
// React is not currently listening to `mouseup`.
node.dispatchEvent(
new MouseEvent('mousedown', {
bubbles: true,
cancelable: true,
}),
);
node.dispatchEvent(
new MouseEvent('mouseup', {
bubbles: true,
cancelable: true,
}),
);
// Now subscribe to `onSelect`.
ReactDOM.render(<input type="text" onSelect={select} />, container);
node.focus();
// This triggers a `select` event in our polyfill.
node.dispatchEvent(
new KeyboardEvent('keydown', {bubbles: true, cancelable: true}),
);
node.dispatchEvent(
new KeyboardEvent('keyup', {bubbles: true, cancelable: true}),
);
// Verify that it doesn't get "stuck" waiting for
// a `mouseup` event that it has "missed" because
// a top-level listener didn't exist yet.
expect(select.mock.calls.length).toBe(1);
}); |
|
Thanks for editing. I believe this
made it a lot more clear to me. |
|
Yea, took me a bit of time to figure out myself. Reading what |
…ebook#11299) (facebook#11676) * Rewrite SelectEventPlugin-test to test behavior using Public API * Minor refactor * Make sure that we test that "focus" event is ignored Use newer API when creating events * Rewrote the other test to use Public API as well * Tweak the test * Remove -internal suffix * Oops
I rewrote one of the two tests to use Public API, but I'm not sure about the other.
The test is supposed to verify that in case we have no event listeners attached, the events are not extracted. Isn't this decision taken internally by react though? And since they are not extracted in the first place how can you verify it without tapping into React internals? Any ideas or feedback are welcome.