Flush all pending microtasks and updates before returning from waitFor

[jsnajdr]

It can happen that a waitFor call like

await waitFor(() => getByText('green'));

successfully returns because the "green" text has been rendered, but after returning, the effects or state updates scheduled by the "green" render haven't been executed yet.

That's because waitFor internally leaves the act environment for a moment, lets a render to be performed and to schedule updates outside the act queue, and then returns before these updates were flushed.

I'm submitting a test that exposes this bug, using a series of cascaded effects and state updates, and also a fix.

Accidentally, @eps1lon landed almost exactly the same fix to @testing-library/react a month ago: testing-library/react-testing-library#1137. But I discovered the React Native bug independently, while investigating why our own tests are failing. 🙂

[AugustinLF]

Nice catch, I've had this problem in several of my tests, looking forward for this fix!

[AugustinLF]

I can see in the reference PR that there is

if (jestFakeTimersAreEnabled()) {
  jest.advanceTimersByTime(0)
}

Do you know why this is not needed here?

Did your added test failed with both fake and real timers?

[jsnajdr]

Did your added test failed with both fake and real timers?

Yes.

Do you know why this is not needed here?

Yes and it's an interesting story 🙂 The setImmediate function that is being called here, it's imported from the ./helpers/timers module, and it's the real Node.js function, not the fake one provided by Jest. Therefore, fake timers don't need to be advanced.

We could use global.setImmediate, and then we'd need to advance the timers, otherwise the promise would never resolve. Like this:

await new Promise((resolve) => {
  global.setImmediate(resolve);
  if (jestFakeTimersAreEnabled()) {
    jest.advanceTimersByTime(0);
  }
});

But, if you really do this, the test will start failing with fake timers! The scheduled effects won't be executed. That's because there is a bug in React: facebook/react#25889. When the react-dom module is loaded, it will save the current value of window.setImmediate to a local variable, and will use it later to schedule effects. If your test suite calls jest.useFakeTimers() later, that's too late. React is already using real timers.

With fake setImmediate used in the promise above, Jest doesn't wait long enough for the real setImmediate timer to fire. After returning from waitFor, the effect has not been executed yet.

To conclude, the "waitFor + fake timers" scenario is currently broken and is not going to work reliably. I think I know how to break @testing-library/react even with the testing-library/react-testing-library#1137 patch applied. I'll try it out.

[AugustinLF]

Oh, that's a really good catch, this setImmediate problem explains the issue we've had in #1347, it makes sense to me.

[jsnajdr]

So I had a closer look at the question whether we should call jest.advanceTimersByTime(0) and my conclusion is no, it's not needed.

Our issue is that when a component like this is rerendered:

function Comp({ text }) {
  useEffect(() => {
    sideEffectWith(text);
  }, [text]);

  return <div>{ text }</div>;
}

then React, when committing the rerender (the commitRootImpl function), writes the new text to the DOM (commitMutationEffects), and schedules a callback to flush the effects (scheduleCallback(flushPassiveEffects)).

A waitFor like

await waitFor(() => getByText(text));

looks at the DOM, and can be satisfied and return immediately after the DOM update. But at that time, the effects are not yet flushed, the sideEffectWith function hasn't yet been called with the new text value. And that's the problem, both effects (mutation and passive) belong logically together.

The effects are scheduled with the unstable_scheduleCallback function from the scheduler package, which uses:

• window.postMessage in a real browser
• setImmediate in Node.js
• setTimeout(0) in JSDOM environment, which supports neither window.MessageChannel nor setImmediate

React Native tests usually run in jest-environment-node, not in jest-environment-jsdom, so the callback is scheduled with setImmediate. So the waitFor implementation in @testing-library/react-native needs to wait for the next setImmediate. And the @testing-library/react implementation needs to wait for the next setTimeout(0).

Because of facebook/react#25889, the callback is always scheduled using real timers. We don't need to advance any fake timers. We're waiting solely for things scheduled in React internals, never for any user-space callback.

The extra jest.advanceTimersByTime(0) call could even be considered a bug: if there's user-space code that schedules a fake setTimeout(0), then it's the user's, i.e., the test writer's, responsibility to advance the timers accordingly, whenever the user wants the timers to be advanced. The library shouldn't do it for them, at a time that's outside user's control.

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (f5fc6d4) 96.13% compared to head (ac9597b) 96.14%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1366   +/-   ##
=======================================
  Coverage   96.13%   96.14%           
=======================================
  Files          49       49           
  Lines        3314     3318    +4     
  Branches      491      492    +1     
=======================================
+ Hits         3186     3190    +4     
  Misses        128      128           

Impacted Files	Coverage Δ
src/flushMicroTasks.ts	100.00% <100.00%> (ø)
src/waitFor.ts	76.47% <100.00%> (+0.43%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mdjastrzebski]

@jsnajdr Awesome work and research. Thanks for contributing this fix!

[jsnajdr]

This is a weird thenable because the then function doesn't return anything and is therefore not chainable. Also, the setImmediate timer starts running at the time when the promise chain is constructed, not when it runs.

flushMicroTasks().then( flushMicroTasks ).then( flushMicroTasks )

should run there setImmediates after each other, but instead it will crash, and even after fixing the crash, it will run all three timers in parallel.

The way how await works doesn't expose these bugs, but outside await, the helper won't work.

My intuition is that the really correct version will look differently 🙂

[mdjastrzebski]

My bad, I've missed that comment.

So when used this in instead of inline Promise it passed the test correctly. @jsnajdr would you be able to submit a PR with better implementation. You seem to now the Promise stuff very well, so help in this area would be very welcomed.

[mdjastrzebski]

@jsnajdr Should the implementation be just:

import { setImmediate } from './helpers/timers';

export function flushMicroTasks() {
  return new Promise((resolve) => setImmediate(resolve));
}

[mdjastrzebski]

Add #1374 so that we have a proper place for discussion