feat: Use act to flush instead of custom implementation

[eps1lon]

Replaces #514 with wrapping unmountComponentAtNode in act i.e. we no longer flush microtasks manually. We need to wrap it in act for React 17 so that effect cleanup functions are flushed.

I extracted the original description into a test (as I understood the description) and the test is still passing with this chamge

The current implementation is problematic since it relies on a specific implementation of the scheduler i.e. a specific config. Some aspects already changed over time and might change again (considering main thread scheduling proposal for chrome). So having the custom implementation is a liability that already caused problems in the past.

As far as I can tell the underlying problem is tests that do not properly reset their timers. You shouldn't use fake timers between tests if you're using third party libraries. As soon as they use any async APIs in beforeEach or afterEach (like we're doing) you can get into incredibly hard to debug situations. At least we shouldn't be solving this problems since jest's fake timers aren't the only fake timers.

We also currently didn't handle any macrotasks so I don't even think we solved that many issues.

PS: Not having any repros (does not have to be deterministic) is problematic for code that is supposed to stay. I'd release this in a major or alpha to see if this causes any problems. My guess is that we should add a warning in our automatic cleanup if fake timers are still on.

[codesandbox-ci]

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 4b62cc3:

Sandbox	Source
React	Configuration
kentcdodds/react-testing-library-examples	Configuration

[codecov]

Codecov Report

Merging #768 into master will not change coverage.
The diff coverage is 100.00%.

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

Impacted Files	Coverage Δ
src/pure.js	100.00% <100.00%> (ø)

---

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 5a10621...4b62cc3. Read the comment docs.

[MatanBobi]

This looks good IMO, mostly because the flushWorkAndMicroTasks here covers the use cases we're covering.
The problem you're talking about though is tricky, we can either "break" this API and put a warning if fake timers are still in use outside the test scope, or we can keep it this way and will have to "follow up" on changes happening in the scheduler.
A warning sounds good to me.

[kentcdodds]

I'm a huge fan of this change.

To be clear, could you give an example of a typical kind of way some folks are writing their tests with fake timers that won't work with this change and what they'd have to do to migrate?

[MatanBobi]

@kentcdodds I think this relates to this issue we had after we changed the flush implementation.
If I understand the change, users who won't useRealTimers in their afterEach but setting up fakeTimers in before each, will have a problem with this.

beforeEach(() => {
  jest.useFakeTimers()
})

Without this, it will fail IMO:

afterEach(() => {
  jest.runOnlyPendingTimers()
  jest.useRealTimers()
})

[kentcdodds]

Ok, so after this change, folks only need to do this, correct?

beforeEach(() => jest.useFakeTimers())
afterEach(() => jest.useRealTimers())

Thanks to auto-cleanup being wrapped in act. Is that right? If so, then that's terrific!

[MatanBobi]

Ok, so after this change, folks only need to do this, correct?

beforeEach(() => jest.useFakeTimers())
afterEach(() => jest.useRealTimers())

Thanks to auto-cleanup being wrapped in act. Is that right? If so, then that's terrific!

I think so.. Maybe @eps1lon will be able to confirm this :)
If that's the case, I agree that will be amazing..

[eps1lon]

#768 (comment)

Yep, that's what I think is the correct pattern for using fake timers. Otherwise you're creating an environment for a test that leaks to possibly unrelated files.

Though I think we should add a warning to our cleanup function if we detect fake timers when cleaning up. Then we can explain the reason and it hopefully helps people detect these issues.

And to re-iterate: This change is backwards incompatible. I suspect most people don't clean up their timers so we should probably release this as a major.

[kentcdodds]

Yep, makes sense.

In my own tests, I'm thinking that I'll do this in my setup file:

afterEach(() => jest.useRealTimers())

And then just call jest.useFakeTimers() for the ones that need fake timers. However, React isn't the only thing that has timers that may need to be run, so I think we should probably still recommend running pending timers:

afterEach(() => {
  jest.runOnlyPendingTimers()
  jest.useRealTimers()
})

So not a ton will change for the way I do things (though I'm sure it will for others). In any case, I'm super jazzed about this change. Thanks!

[kentcdodds]

Let's think about other breaking changes we could make at this point as well...

[kentcdodds]

FYI, I tried this on https://github.com/kentcdodds/bookshelf and it works unless I upgrade to 2.11.0 of react-query: TanStack/query#909 (comment)

Still need to do some investigation there. I'm guessing it's a problem with react-query (because even without these changes, I start getting act warnings with that version of react-query), but I thought I'd mention it here to keep everyone informed.

[kentcdodds]

Found the problem was with react-query. This change makes things better with that as well as it turns out.

TanStack/query#909 (comment)

Unless anyone can think of other breaking changes we should consider, I think this is ready to go.

[eps1lon]

@kentcdodds Yeah I only had this as a draft because I was hoping for some more tests in other repos. With bookshelf tested this looks fine by me.

We had some pretty bad timing issues with React 17 in Material-UI and and already use patch internally.

Without this patch we'll probably see some very odd bug reports for React 17 depending on the test runner/timers used. I just hope we don't swallow some missing act warnings but since we're not inside the test this seems safe to do.

[kentcdodds]

Let's do a beta release with this.

[kentcdodds]

Another thing to consider is whether we want to make any breaking changes to DOM Testing Library which would impact this lib and we'd like to bundle up...

[testing-library-bot]

🎉 This PR is included in version 11.0.0-beta.1 🎉

The release is available on:

• npm package (@beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀