Skip to content
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

Make sure that jest tests fail if an error is thrown within a jsdom event handler #8260

Closed
sophiebits opened this issue Nov 10, 2016 · 7 comments

Comments

@sophiebits
Copy link
Collaborator

In a few situations in our unit tests, we use .dispatchEvent on a jsdom DOM node in order to use the jsdom event system:

instance.a.dispatchEvent(inputEvent);

But it appears that if the event handler throws (ex: change the onChange handler in that test to throw), then the error is silently ignored and does not cause the test to fail.

jsdom does make some attempt to report an error:

https://github.com/tmpvar/jsdom/blob/9.8.3/lib/jsdom/living/events/EventTarget-impl.js#L235

We should hook into this in our jest config (scripts/jest/test-framework-setup.js) and see if we can fail any test that triggers this exception behavior. From the implementation of reportException it looks like we might be able to do an onerror handler; if that doesn't work then hooking into their "virtual console" may be possible.

@franciscofsales
Copy link

I tried playing around with this by catching the error event jsdom dispatches on window and it was kind of going fine but then I had conflicts with tests in here:

https://github.com/facebook/react/blob/master/src/renderers/shared/stack/reconciler/tests/ReactErrorBoundaries-test.js#L256

any ideas what I might be getting wrong?

@renderf0x
Copy link

renderf0x commented Nov 13, 2016

Got this working using the onerror suggestion above; Added #8278 as a PR.

Edit: and low and behold, same issue as @franciscofsales . Good catch, closing the PR.

@franciscofsales
Copy link

@renderf0x that is exactly what I had done, apart from naming and comment. Not sure how to not trigger that issue.

@franciscofsales
Copy link

Got it. Added #8279. Thanks

@renderf0x
Copy link

renderf0x commented Nov 13, 2016

@franciscofsales ah indeed. rad. we'd have to have FB confirm, but this probably just never was a problem because it wasn't being listened for (hence this issue). good stuff.

@franciscofsales
Copy link

@renderf0x yeah, I think so. I got the answer from your commit actually. I was spying on error turns out it's onerror. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants
@sophiebits @gaearon @renderf0x @franciscofsales and others