-
Notifications
You must be signed in to change notification settings - Fork 46.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use native event dispatching instead of Simulate or SimulateNative #13023
Conversation
In facebook#12629 @gaearon suggested that it would be better to drop usage of `ReactTestUtils.Simulate` and `ReactTestUtils.SimulateNative`. In this PR I’m attempting at removing it from a lot of places with only a few leftovers. Those leftovers can be categorized into three groups: 1. Anything that tests that `SimulateNative` throws. This is a property that native event dispatching doesn’t have so I can’t convert that easily. Affected test suites: `EventPluginHub-test`, `ReactBrowserEventEmitter-test`. 2. Anything that tests `ReactTestUtils` directly. Affected test suites: `ReactBrowserEventEmitter-test` (this file has one test that reads "should have mouse enter simulated by test utils"), `ReactTestUtils-test`. 3. Anything that dispatches a `change` event. The reason here goes a bit deeper and is rooted in the way we shim onChange. Usually when using native event dispatching, you would set the node’s `.value` and then dispatch the event. However inside [`inputValueTracking.js`][] we install a setter on the node’s `.value` that will ignore the next `change` event (I found [this][near-perfect-oninput-shim] article from Sophie that explains that this is to avoid onChange when updating the value via JavaScript). All remaining usages of `Simulate` or `SimulateNative` can be avoided by mounting the containers inside the `document` and dispatching native events. Here some remarks: 1. I’m using `Element#click()` instead of `dispatchEvent`. In the jsdom changelog I read that `click()` now properly sets the correct values (you can also verify it does the same thing by looking at the [source][jsdom-source]). 2. I had to update jsdom in order to get `TouchEvent` constructors working (and while doing so also updated jest). There was one unexpected surprise: `ReactScheduler-test` was relying on not having `window.performance` available. I’ve recreated the previous environment by deleting this property from the global object. 3. I was a bit confused that `ReactTestUtils.renderIntoDocument()` does not render into the document 🤷 [`inputValueTracking.js`]: https://github.com/facebook/react/blob/392530104c00c25074ce38e1f7e1dd363018c7ce/packages/react-dom/src/client/inputValueTracking.js#L79 [near-perfect-oninput-shim]: https://sophiebits.com/2013/06/18/a-near-perfect-oninput-shim-for-ie-8-and-9.html [jsdom-source]: https://github.com/jsdom/jsdom/blob/45b77f5d21cef74cad278d089937d8462c29acce/lib/jsdom/living/nodes/HTMLElement-impl.js#L43-L76
/** | ||
* Render a TestRefsComponent and ensure that the main refs are wired up. | ||
*/ | ||
const renderTestRefsComponent = function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I move this function (which is only used for one test) inside this describe()
closure so it's easier to clean up the container
.
You're not alone lol |
expect(divRef.current.textContent).toBe('remote:2, local:2'); | ||
|
||
document.body.removeChild(container); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to be careful with stuff like this because it won't clean up if the test fails. I tend to either use try/finally (annoying) or put it in beforeEach/afterEach.
function assignEvent(e) { | ||
event = e; | ||
} | ||
const node = ReactDOM.render(<div onClick={assignEvent} />, element); | ||
ReactTestUtils.Simulate.click(ReactDOM.findDOMNode(node)); | ||
ReactDOM.findDOMNode(node).click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove findDOMNode here too. Actually we should remove it in a lot of places in the tests, it's often completely unnecessary and is confusing
This behavior seems to be around for 5 years already 🙈 Do we ever want to change that? It doesn't have to confuse people for 5 more years 🙂 (Related issue: #3789) |
@gaearon I made sure all attached elements are also cleaned up even if the test fails and also removed all |
I've ended up writing
I feel like abstractions around testing are brutally hard though. |
@nhunzaker Yea it's hard to strike the balance between tests that are easy to understand/explicit and avoiding lots of boilerplate code. I think using let container
beforeEach(() => {
container = document.createElement('div');
document.body.appendChild(container);
});
afterEach(() => {
document.body.removeChild(container);
container = null;
}); The |
We do have a way to simulate onChange AFAIK. It’s just a bit odd. You have to grab a value setter before it gets shimmed and then use that directly before dispatching an event. It’s how ChangeEventPlugin tests themselves work. It would be good to convert change callsites too. |
Thanks! If you could follow up for change events that would be great. |
In #12629 @gaearon suggested that it would be better to drop usage of
ReactTestUtils.Simulate
andReactTestUtils.SimulateNative
. In this PR I’m attempting at removing it from a lot of places with only a few leftovers.Those leftovers can be categorized into three groups:
SimulateNative
throws. This is a property that native event dispatching doesn’t have so I can’t convert that easily. Affected test suites:EventPluginHub-test
,ReactBrowserEventEmitter-test
.ReactTestUtils
directly. Affected test suites:ReactBrowserEventEmitter-test
(this file has one test that reads "should have mouse enter simulated by test utils"),ReactTestUtils-test
.change
event. The reason here goes a bit deeper and is rooted in the way we shim onChange. Usually when using native event dispatching, you would set the node’s.value
and then dispatch the event. However insideinputValueTracking.js
we install a setter on the node’s.value
that will ignore the nextchange
event (I found this article from Sophie that explains that this is to avoid onChange when updating the value via JavaScript).All remaining usages of
Simulate
orSimulateNative
can be avoided by mounting the containers inside thedocument
and dispatching native events.Here some remarks:
Element#click()
instead ofdispatchEvent
. In the jsdom changelog I read thatclick()
now properly sets the correct values (you can also verify it does the same thing by looking at the source).TouchEvent
constructors working (and while doing so also updated jest). There was one unexpected surprise:ReactScheduler-test
was relying on not havingwindow.performance
available. I’ve recreated the previous environment by deleting this property from the global object.ReactTestUtils.renderIntoDocument()
does not render into the document 🤷