-
Notifications
You must be signed in to change notification settings - Fork 47.4k
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 act() tests by properly unmounting the container #14974
Conversation
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 moving all the act tests to its own file, so I'll be sure to put this in that too. approving for now, but I'll merge it in once I have the async act stuff ready. thanks!
@@ -678,6 +665,39 @@ describe('ReactTestUtils', () => { | |||
); | |||
}); | |||
|
|||
it('flushes immediate re-renders with act but advances timers later', () => { |
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.
act doesn't advance timers at all, so this title is a bit wrong.
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.
Right, removed that part.
1171f94
to
b5e91d5
Compare
b5e91d5
to
d16f798
Compare
@threepointone Rebased my PR but the issue I noticed earlier is no longer happening (the missing |
Thank you, it just slipped my mind! I do think your structure is better, I'll merge it in today. |
React: size: -0.4%, gzip: 🔺+0.4% ReactDOM: size: -1.4%, gzip: -0.8% Details of bundled changes.Comparing: 543353a...d16f798 react
react-dom
react-art
react-test-renderer
react-noop-renderer
react-reconciler
react-native-renderer
scheduler
react-events
Generated by 🚫 dangerJS |
lol @sizebot go home, you're drunk |
@threepointone For the future — it's a good idea to always do cleanup in |
While playing around with the new
act()
API I discovered a test thatwas unexpectedly failing. After some digging I noticed that the test is
passing when run in isolation and that the test suite can be fixe by
properly calling
ReactDOM.unmountComponentAtNode()
to clean up afterevery test run.
You can reproduce the issue by simply commenting out the call to
unmountComponentAtNode
in theafterEach
callback. The newly addedtest will fail but pass in isolation.
I thought it would be a good idea to fix this upstream so nobody runs
into this issue again.