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

Flush all pending microtasks and updates before returning from waitFor #1366

Merged
merged 5 commits into from
Mar 23, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions src/__tests__/waitFor.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -263,3 +263,52 @@ test.each([false, true])(
expect(mockFn).toHaveBeenCalledTimes(3);
}
);

test.each([false, true])(
'flushes scheduled updates before returning (fakeTimers = %s)',
async (fakeTimers) => {
if (fakeTimers) {
jest.useFakeTimers();
}

function Apple({ onPress }: { onPress: (color: string) => void }) {
const [color, setColor] = React.useState('green');
const [syncedColor, setSyncedColor] = React.useState(color);

// On mount, set the color to "red" in a promise microtask
React.useEffect(() => {
Promise.resolve('red').then((c) => setColor(c));
mdjastrzebski marked this conversation as resolved.
Show resolved Hide resolved
}, []);

// Sync the `color` state to `syncedColor` state, but with a delay caused by the effect
React.useEffect(() => {
setSyncedColor(color);
}, [color]);

return (
<View>
<Text>Apple</Text>
<Text>{color}</Text>
<Pressable onPress={() => onPress(syncedColor)}>
<Text>Trigger</Text>
</Pressable>
</View>
);
}

const spy = jest.fn<void, [string]>();
const { getByText } = render(<Apple onPress={spy} />);

// This `waitFor` will succeed on first check, because the "Apple" text is there
// since the initial mount.
await waitFor(() => getByText('Apple'));

// This `waitFor` will also succeed on first check, because the promise that sets the
// `color` state to "red" resolves right after the previous `await waitFor` statement.
await waitFor(() => getByText('red'));

// Check that the `onPress` callback is called with the already-updated value of `syncedColor`.
fireEvent.press(getByText('Trigger'));
expect(spy).toHaveBeenCalledWith('red');
}
);
5 changes: 4 additions & 1 deletion src/waitFor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,10 @@ export default async function waitFor<T>(
setReactActEnvironment(false);

try {
return await waitForInternal(expectation, optionsWithStackTrace);
const result = await waitForInternal(expectation, optionsWithStackTrace);
// drain the microtask queue before restoring the `act` environment
await new Promise((resolve) => setImmediate(resolve));
return result;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} finally {
setReactActEnvironment(previousActEnvironment);
}
Expand Down