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(cleanup): remove scheduler code from flush-microtasks #744

Merged
merged 4 commits into from Jul 13, 2020

Conversation

@kentcdodds
Copy link
Member

kentcdodds commented Jul 13, 2020

What: This removes the scheduler code from flush-microtasks

Why: Because the test that failed is fundamentally flawed

How:

  1. Rewrite the test to illustrate why it's flawed (we can't and shouldn't guarantee that all scheduled tasks will run simply as a result of cleanup)
  2. Rewrite the test to not test for that
  3. Fix the implementation of flush-microtasks to not use the scheduler (basically revert everything we did for #726 and change the test instead).

Checklist:

- [ ] Documentation added to the docs site N/A

  • Tests
    - [ ] Typescript definitions updated N/A
  • Ready to be merged

Closes #743

@codesandbox
Copy link

codesandbox bot commented Jul 13, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6faf0bc:

Sandbox Source
React Configuration
kentcdodds/react-testing-library-examples Configuration
@@ -31,7 +31,7 @@ test('cleanup waits for queued microtasks during unmount sequence', async () =>
const spy = jest.fn()

const Test = () => {
React.useEffect(() => () => setImmediate(spy))
React.useEffect(() => () => setTimeout(spy, 200))

This comment has been minimized.

@kentcdodds

kentcdodds Jul 13, 2020 Author Member

Users can do all kinds of async things as a result of their cleanup functions and it's not reasonable for them to expect that we'll wait until all those async things are done. They'll have to wait for those things themselves.

So we shouldn't have a setImmediate in here at all.

const spy = jest.fn()

const Test = () => {
React.useEffect(() => () => setImmediate(spy))
React.useEffect(() => spy)

This comment has been minimized.

@kentcdodds

kentcdodds Jul 13, 2020 Author Member

All we can guarantee is that the cleanup function is called, so that's all we'll test.

This comment has been minimized.

@eps1lon

eps1lon Jul 13, 2020 Member

👍 The problem is that we can't know what kind of task is queued from the cleanup function. We only supported setImmediate but any longer running task wasn't caught as far as I can tell.

@codecov
Copy link

codecov bot commented Jul 13, 2020

Codecov Report

Merging #744 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #744   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          100       100           
  Branches        16        16           
=========================================
  Hits           100       100           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e5cf59...6faf0bc. Read the comment docs.

Copy link
Collaborator

MatanBobi left a comment

This one looks good and as @eps1lon said, we were probably only waiting for setImmediate anyways.
Looks like we got ourselves to the wrong adventure 😅 Sorry about that.
I'll close the PR for mocking shceduler (#743).
This PR will resolve https://github.com/testing-library/react-testing-library/issues/738 and #735.

@kentcdodds kentcdodds merged commit 240900c into master Jul 13, 2020
4 checks passed
4 checks passed
ci/codesandbox Building packages succeeded.
Details
codecov/patch Coverage not affected when comparing 9e5cf59...6faf0bc
Details
codecov/project 100.00% (+0.00%) compared to 9e5cf59
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kentcdodds kentcdodds deleted the pr/remove-scheduler branch Jul 13, 2020
@kentcdodds
Copy link
Member Author

kentcdodds commented Jul 13, 2020

Thanks for all the work you did anyway @MatanBobi!

@MatanBobi
Copy link
Collaborator

MatanBobi commented Jul 13, 2020

Glad I was able to help @kentcdodds

@kentcdodds
Copy link
Member Author

kentcdodds commented Jul 13, 2020

🎉 This PR is included in version 10.4.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.