New internal testing helpers: waitFor, waitForAll, waitForPaint

[acdlite]

Over the years, we've gradually aligned on a set of best practices for for testing concurrent React features in this repo. The default in most cases is to use act, the same as you would do when testing a real React app. However, because we're testing React itself, as opposed to an app that uses React, our internal tests sometimes need to make assertions on intermediate states that act intentionally disallows.

For those cases, we built a custom set of Jest assertion matchers that provide greater control over the concurrent work queue. It works by mocking the Scheduler package. (When we eventually migrate to using native postTask, it would probably work by stubbing that instead.)

A problem with these helpers that we recently discovered is, because they are synchronous function calls, they aren't sufficient if the work you need to flush is scheduled in a microtask — we don't control the microtask queue, and can't mock it.

act addresses this problem by encouraging you to await the result of the act call. (It's not currently required to await, but in future versions of React it likely will be.) It will then continue flushing work until both the microtask queue and the Scheduler queue is exhausted.

We can follow a similar strategy for our custom test helpers, by replacing the current set of synchronous helpers with a corresponding set of async ones:

• expect(Scheduler).toFlushAndYield(log) -> await waitForAll(log)
• expect(Scheduler).toFlushAndYieldThrough(log) -> await waitFor(log)
• expect(Scheduler).toFlushUntilNextPaint(log) -> await waitForPaint(log)

These APIs are inspired by the existing best practice for writing e2e React tests. Rather than mock all task queues, in an e2e test you set up a timer loop and wait for the UI to match an expecte condition. Although we are mocking some of the task queues in our tests, the general principle still holds: it makes it less likely that our tests will diverge from real world behavior in an actual browser.

In this commit, I've implemented the new testing helpers and converted one of the Suspense tests to use them. In subsequent steps, I'll codemod the rest of our test suite.

[react-sizebot]

Comparing: d49e0e0...8aadd4f

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	155.25 kB	155.25 kB	=	48.98 kB	48.98 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	157.24 kB	157.24 kB	=	49.64 kB	49.64 kB
facebook-www/ReactDOM-prod.classic.js	=	532.50 kB	532.50 kB	=	94.85 kB	94.85 kB
facebook-www/ReactDOM-prod.modern.js	=	516.42 kB	516.42 kB	=	92.45 kB	92.45 kB
oss-experimental/jest-react/cjs/jest-react.production.min.js	+2.18%	2.43 kB	2.48 kB	+2.28%	1.19 kB	1.21 kB
oss-stable-semver/jest-react/cjs/jest-react.production.min.js	+2.18%	2.43 kB	2.48 kB	+2.28%	1.19 kB	1.21 kB
oss-stable/jest-react/cjs/jest-react.production.min.js	+2.18%	2.43 kB	2.48 kB	+2.28%	1.19 kB	1.21 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-experimental/jest-react/cjs/jest-react.production.min.js	+2.18%	2.43 kB	2.48 kB	+2.28%	1.19 kB	1.21 kB
oss-stable-semver/jest-react/cjs/jest-react.production.min.js	+2.18%	2.43 kB	2.48 kB	+2.28%	1.19 kB	1.21 kB
oss-stable/jest-react/cjs/jest-react.production.min.js	+2.18%	2.43 kB	2.48 kB	+2.28%	1.19 kB	1.21 kB
oss-experimental/jest-react/cjs/jest-react.development.js	+0.75%	10.66 kB	10.74 kB	+0.51%	3.76 kB	3.78 kB
oss-stable-semver/jest-react/cjs/jest-react.development.js	+0.75%	10.66 kB	10.74 kB	+0.51%	3.76 kB	3.78 kB
oss-stable/jest-react/cjs/jest-react.development.js	+0.75%	10.66 kB	10.74 kB	+0.51%	3.76 kB	3.78 kB

Generated by 🚫 dangerJS against 8aadd4f

[acdlite]

Need to decide what to do about jest-react package. My idea of just deleting the prod build and using the dev one for both didn't work because I forgot the dev build still checks NODE_ENV. I feel like the whole thing should move to shared/ (maybe not literally shared/ but a folder the behaves the same way — only exists for our internal purposes).

[tyao1]

Looks great!

Probably unrelated to the PR. For tests that do a sync work and expect the output only for the sync work, what should be done?

react/packages/react-reconciler/src/__tests__/ReactIncremental-test.js

Lines 1692 to 1722 in 2881d56

	ReactNoop.flushSync(() => {
	ReactNoop.batchedUpdates(() => {
	instance.setState({n: 1}, () =>
	Scheduler.unstable_yieldValue('setState 1'),
	);
	instance.setState({n: 2}, () =>
	Scheduler.unstable_yieldValue('setState 2'),
	);
	ReactNoop.batchedUpdates(() => {
	instance.setState({n: 3}, () =>
	Scheduler.unstable_yieldValue('setState 3'),
	);
	instance.setState({n: 4}, () =>
	Scheduler.unstable_yieldValue('setState 4'),
	);
	Scheduler.unstable_yieldValue('end inner batchedUpdates');
	});
	Scheduler.unstable_yieldValue('end outer batchedUpdates');
	});
	});
	// ReactNoop.flush() not needed because updates are synchronous
	expect(Scheduler).toHaveYielded([
	'end inner batchedUpdates',
	'end outer batchedUpdates',
	'setState 1',
	'setState 2',
	'setState 3',
	'setState 4',
	]);

[tyao1]

Is it because we would get wrong stack trace that we use diff instead of expect?

[acdlite]

Yeah

[acdlite]

Probably unrelated to the PR. For tests that do a sync work and expect the output only for the sync work, what should be done?

That pattern is still fine but maybe as a follow up we could replace toHaveYielded with a new API that looks more like the others. Just for symmetry.

[tyao1]

Probably unrelated to the PR. For tests that do a sync work and expect the output only for the sync work, what should be done?

That pattern is still fine but maybe as a follow up we could replace toHaveYielded with a new API that looks more like the others. Just for symmetry.

I'm thinking if we are going to put everything inside a microtask, the toHaveYielded above would also need to wait for the microtask.

[acdlite]

I'm thinking if we are going to put everything inside a microtask, the toHaveYielded above would also need to wait for the microtask.

The idea is you would only call it after an event + microtasks have just run. The most common case is right after await act(...).

The other case is after flushSync. The contract of flushSync is that it should force all sync work to happen immediately, even before the microtask queue. So that's a case where it's specifically important not to await.

[gaearon]

gamechanger... i'll understand our tests now!