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

Fix test failing because of react error boundaries #222

Merged
merged 5 commits into from
Oct 4, 2017

Conversation

gribnoysup
Copy link
Contributor

@gribnoysup gribnoysup commented Oct 4, 2017

Fixing the test that was disabled due to the React 16 error boundaries handling: #215 (comment)

I found out that with error boundaries, error throws twice in development mode, here is a justification for that from React team: facebook/react#10384 (comment)

I found a fix for that although it looks a little hacky. The other option could be to run tests in production mode, but that breaks debug tests and I'm not sure how to fix this.

@gribnoysup
Copy link
Contributor Author

Oops, now it is failing for older react versions 😿

I'll try something else

@gribnoysup
Copy link
Contributor Author

It's working now 🎉

Copy link
Collaborator

@lencioni lencioni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! Looks good to me, just one small change before we can merge it.

this.props.children = [
<div key={1} />,
<div key={2} />,
<div key={3} />,
];

expect(this.subject).toThrowError(notValidErrorMessage);

window.onerror = undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If window.onerror had been previously set to something else, this will break it. It would be a good idea to store the original value before assigning it, and then set it back to that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, forgot about that! Fixed :)

@lencioni lencioni merged commit 08e2d7c into civiccc:master Oct 4, 2017
@lencioni
Copy link
Collaborator

lencioni commented Oct 4, 2017

Thanks for fixing this!

@gribnoysup
Copy link
Contributor Author

No problem! Happy to help! 😄

@gribnoysup gribnoysup deleted the tests-fix branch October 4, 2017 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants