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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
70edb16
Flush all pending microtasks and updates before returning from waitFor
jsnajdr Mar 15, 2023
1c11074
Disable a linter error, its advice is not good
jsnajdr Mar 18, 2023
70fc3cd
refactor: use `flushMicroTasks` util
mdjastrzebski Mar 23, 2023
c229766
refactor: add legacy fake timer test
mdjastrzebski Mar 23, 2023
ac9597b
refactor: tweaks
mdjastrzebski Mar 23, 2023
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
53 changes: 53 additions & 0 deletions src/__tests__/waitFor.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,3 +263,56 @@ test.each([false, true])(
expect(mockFn).toHaveBeenCalledTimes(3);
}
);

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

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(() => {
// eslint-disable-next-line promise/prefer-await-to-then, promise/catch-or-return
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 testID="root">
<Text>{color}</Text>
<Pressable onPress={() => onPress(syncedColor)}>
<Text>Trigger</Text>
</Pressable>
</View>
);
}

const onPress = jest.fn();
const view = render(<Apple onPress={onPress} />);

// Required: this `waitFor` will succeed on first check, because the "root" view is there
// since the initial mount.
await waitFor(() => view.getByTestId('root'));

// 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(() => view.getByText('red'));

// Check that the `onPress` callback is called with the already-updated value of `syncedColor`.
fireEvent.press(view.getByText('Trigger'));
expect(onPress).toHaveBeenCalledWith('red');
}
);
2 changes: 1 addition & 1 deletion src/flushMicroTasks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { setImmediate } from './helpers/timers';

type Thenable<T> = { then: (callback: () => T) => unknown };

export function flushMicroTasks<T>(): Thenable<T> {
export function flushMicroTasks(): Thenable<void> {
return {
// using "thenable" instead of a Promise, because otherwise it breaks when
// using "modern" fake 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.

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 🙂

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@mdjastrzebski mdjastrzebski Mar 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@jsnajdr Should the implementation be just:

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Expand Down
6 changes: 5 additions & 1 deletion src/waitFor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* globals jest */
import act, { setReactActEnvironment, getIsReactActEnvironment } from './act';
import { getConfig } from './config';
import { flushMicroTasks } from './flushMicroTasks';
import { ErrorWithStack, copyStackTrace } from './helpers/errors';
import {
setTimeout,
Expand Down Expand Up @@ -196,7 +197,10 @@ export default async function waitFor<T>(
setReactActEnvironment(false);

try {
return await waitForInternal(expectation, optionsWithStackTrace);
const result = await waitForInternal(expectation, optionsWithStackTrace);
// Flush the microtask queue before restoring the `act` environment
await flushMicroTasks();
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