-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Explore alternative event dispatch/simulation #35
Comments
Alrighty, it looks like the best solution would be to implement this: facebook/react#2043 The latest effort behind it was closed because issues were raised which were never addressed. However, the changes appear fairly minimal and should give anyone who wants to give it a try a head start! Review the discussion in there and open a new PR that addresses those issues. Anyone want to do that? |
After further review (and tweets), it appears that this will be a fairly significant effort and perhaps even require a breaking change in React. So we'll have to wait until React 17. Which is unfortunately, but I think it'd be great if we could make this happen in time to get on the React 17 train (whenever that leaves the station...). |
The native click and focus methods do not trigger the DOM side-effects (events) that React and the tested app rely on. Using them contradicts to the core principle of react-testing-library to use the components the same way the users would use them. Ideally, there has to be a way to simulate real user input followed by its side-effects such as DOM state changes (inputs value/checked/selected, scroll position including overflow, urlbar location, fullscreen API unblock, video play API unblock), DOM events that tell the app about some of these changes, CSS hidden state changes (hover, transition state, animation state) from within the JS platform, but this contradicts to the JS/browser security policy of app sandboxing, thus renders the test automation tools that work from within the JS/browser platform (hi Cypress) helpless about the real user input simulation, leaving them with the hacks like dispatchEvent. |
Hi @sompylasar, Thanks for a good overview of the problem. What solutions do you have for us that make attractive trade-offs? |
Hi @kentcdodds Unfortunately I don't see a solution from within the browser/JS sandbox. The limited options I see now are:
|
Thanks @sompylasar, I'm afraid that neither of those solutions is reasonable for this project so we'll have to try for the next best thing. |
@kentcdodds Just a dumb question here.. I'm just trying to understand the reason behind this idea. Is there any downside of using |
Because Dan said so 🤷♂️ as noted in the first comment. |
@kentcdodds Yup, saw the comment from Dan as well.. But was just wondering, why we need to do so.. Is there any benefit for |
I think we should add something like this to dom-testing-library instead. I was thinking about it for the initial release but didn't have time and didn't want to rush it. |
Really awesome progress here: testing-library/dom-testing-library#13 Thanks @jomaxx!! I'll go ahead and leave this open until that gets released and we update react-testing-library to re-export it. |
* get/query by value * updated test
We're attaching to roots in 17. |
Our tests are already passing on React 17's RC. Is there anything else we'd need to change to ensure stability in future React 17 releases? Based on my understanding of the blog post I couldn't find any major issues. Is it just extra code for React 16 compatibility? |
If tests are passing you're probably all good. :-) I commented on this thread because binding to document was brought up as a limitation. |
@nickmccurdy are we sure that's the case? What I remember is that we're running our tests on |
@MatanBobi We added The only change we did for React 17 was dispatching focusin as well when calling |
Do you also dispatch focusout for the "left" element? |
No. We also don't dispatch mouseout on
Edit: |
This is why we suggest people use I'm excited about the events being attached to the root. Because they were attached to the However, I'm actually pretty happy with what we have now and I don't see a reason to change. I think it still makes the most sense to actually render everything in So while I think it's great that events are now attached to the root, I don't think that will change anything in React Testing Library. We ended up with a better solution anyway :) |
Based on a twitter conversation with Dan Abramov where he suggests that this dispatch events rather than use TestUtils.Simulate, I think it may be worth looking into. As he mentioned:
I think adding the
container
to the document should be fine normally, especially in Jest where the test files run in isolated environments. But I'm a little bit worried that we'd fill up the document with mounted components that never get unmounted. We could encourage people to have abeforeEach
that cleans things up, but that'd be annoying and reduce the simplicity. We could simplify it with aimport 'react-testing-library/autoclean'
or something, but still that's a reduction in simplicity. So I'm not certain how to solve this problem...It would be cool if we could have a utility that does actual events and also encourage people to just use
.click()
and.focus()
methods native to DOM nodes. So I'd definitely like to explore this.The text was updated successfully, but these errors were encountered: