-
Notifications
You must be signed in to change notification settings - Fork 46.7k
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
Improve error boundaries tests #7569
Conversation
container.innerHTML = ReactDOMServer.renderToString(<Boundary />); | ||
expect(container.firstChild.innerHTML).toBe('Happy Birthday!'); | ||
expect(EventPluginHub.putListener).not.toBeCalled(); | ||
// note: renderToString does not call componentDidMount |
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.
Not sure why this even uses renderToString. Mind changing it to use ReactDOM.render so that we test the queueing behavior better? (See, I didn't even notice that the old test was testing the wrong thing but now that componentDidMount is completely absent here it's much more obvious. Thanks.)
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.
Should we have tests for both renderToString
and ReactDOM.render
? The title of this test block should be changed too if we're not testing SSR.
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.
In most cases it's fine to just test client rendering since the server codepath is mostly a subset of the client one.
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.
OK, updated the test to use ReactDOM.render
instead. FYI, original test was introduced by #6694
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.
rawr. okay.
lgtm otherwise, thanks! |
106008e
to
0c031c5
Compare
this should be good to go, updated all the test to use |
Thanks! |
(cherry picked from commit 7d7defe)
this is kinda of a follow up to #7558 where @spicyj said:
the new tests makes sure the methods are executed in the proper order and don't rely on
EventPluginHub
anymore - the'does not register event handlers for unmounted children'
test still usesEventPluginHub
since it looks like it makes sense to leave that check there...also removed some parentheses and wrapped some lines.