New internal testing helpers: waitFor, waitForAll, waitForPaint by acdlite · Pull Request #26285 · facebook/react

acdlite · 2023-03-02T21:42:12Z

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 · 2023-03-02T21:56:12Z

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 · 2023-03-02T22:21:32Z

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', 
        
           ]);

packages/jest-react/npm/index.js

tyao1 · 2023-03-02T22:37:20Z

packages/jest-react/src/JestReact.js

+  error.message = `
+Expected sequence of events did not occur.
+
+${diff(expectedLog, actualLog)}


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

acdlite · 2023-03-02T23:27:43Z

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 · 2023-03-02T23:48:32Z

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 · 2023-03-02T23:52:33Z

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.

Our internal test utils are currently located in the jest-react package. At one point, the idea was to publish some of these as public, officially supported APIs, but we abandoned that idea in favor of `act`. We should really just delete the jest-react package from our repo. What we can do instead is put our internal test utils into a private package. This adds such a package, called internal-test-utils. There are no exported APIs yet but I'll add some in the next step.

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.

gaearon · 2023-03-03T01:41:50Z

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

This converts some of our test suite to use the `waitFor` test pattern, instead of the `expect(Scheduler).toFlushAndYield` pattern. Most of these changes are automated with jscodeshift, with some slight manual cleanup in certain cases. See facebook#26285 for full context.

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. DiffTrain build for [e524467](e524467)

This converts some of our test suite to use the `waitFor` test pattern, instead of the `expect(Scheduler).toFlushAndYield` pattern. Most of these changes are automated with jscodeshift, with some slight manual cleanup in certain cases. See facebook#26285 for full context.

This converts some of our test suite to use the `waitFor` test pattern, instead of the `expect(Scheduler).toFlushAndYield` pattern. Most of these changes are automated with jscodeshift, with some slight manual cleanup in certain cases. See #26285 for full context.

This converts some of our test suite to use the `waitFor` test pattern, instead of the `expect(Scheduler).toFlushAndYield` pattern. Most of these changes are automated with jscodeshift, with some slight manual cleanup in certain cases. See facebook#26285 for full context.

This converts some of our test suite to use the `waitFor` test pattern, instead of the `expect(Scheduler).toFlushAndYield` pattern. Most of these changes are automated with jscodeshift, with some slight manual cleanup in certain cases. See #26285 for full context.

This converts some of our test suite to use the `waitFor` test pattern, instead of the `expect(Scheduler).toFlushAndYield` pattern. Most of these changes are automated with jscodeshift, with some slight manual cleanup in certain cases. See facebook#26285 for full context.

This converts some of our test suite to use the `waitFor` test pattern, instead of the `expect(Scheduler).toFlushAndYield` pattern. Most of these changes are automated with jscodeshift, with some slight manual cleanup in certain cases. See #26285 for full context.

Now that the tests have been codemodded, we can delete toFlushAndYield et al from the codebase. See facebook#26285 for full context. The mock implementation of Scheduler still exposes some (unstable- prefixed) APIs that can be used for some edge cases, like when testing the mock Scheduler itself. There are only a few tests that do this, and even some of those could be rewritten to use `act` or `waitFor`.

This converts some of our test suite to use the `waitFor` test pattern, instead of the `expect(Scheduler).toFlushAndYield` pattern. Most of these changes are automated with jscodeshift, with some slight manual cleanup in certain cases. See #26285 for full context. DiffTrain build for [3cb5afb](3cb5afb)

This converts some of our test suite to use the `waitFor` test pattern, instead of the `expect(Scheduler).toFlushAndYield` pattern. Most of these changes are automated with jscodeshift, with some slight manual cleanup in certain cases. See #26285 for full context. DiffTrain build for [64dde70](64dde70)

This converts some of our test suite to use the `waitFor` test pattern, instead of the `expect(Scheduler).toFlushAndYield` pattern. Most of these changes are automated with jscodeshift, with some slight manual cleanup in certain cases. See #26285 for full context. DiffTrain build for [25685d8](25685d8)

Similar to the rationale for `waitFor` (see facebook#26285), we should always await the result of an `act` call so that microtasks have a chance to fire. This only affects the internal `act` that we use in our repo, for now. In the public `act` API, we don't yet require this; however, we effectively will for any update that triggers suspense once `use` lands. So we likely will start warning in an upcoming minor.

Similar to the rationale for `waitFor` (see #26285), we should always await the result of an `act` call so that microtasks have a chance to fire. This only affects the internal `act` that we use in our repo, for now. In the public `act` API, we don't yet require this; however, we effectively will for any update that triggers suspense once `use` lands. So we likely will start warning in an upcoming minor.

acdlite requested review from sebmarkbage and tyao1 March 2, 2023 21:42

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 2, 2023

acdlite force-pushed the new-test-helpers-waitFor branch from ef4c880 to 4ea9353 Compare March 2, 2023 21:51

acdlite force-pushed the new-test-helpers-waitFor branch 3 times, most recently from cc1f99a to e550732 Compare March 2, 2023 22:11

tyao1 reviewed Mar 2, 2023

View reviewed changes

acdlite force-pushed the new-test-helpers-waitFor branch from e550732 to 319e6af Compare March 2, 2023 23:46

tyao1 approved these changes Mar 2, 2023

View reviewed changes

acdlite force-pushed the new-test-helpers-waitFor branch 3 times, most recently from 07f991e to e28f28c Compare March 3, 2023 00:59

acdlite force-pushed the new-test-helpers-waitFor branch from e28f28c to 8aadd4f Compare March 3, 2023 01:25

acdlite merged commit e524467 into facebook:main Mar 3, 2023

acdlite mentioned this pull request Mar 3, 2023

Codemod tests to waitFor pattern (1/?) #26288

Merged

acdlite mentioned this pull request Mar 4, 2023

Codemod tests to waitFor pattern (7/?) #26307

Merged

acdlite mentioned this pull request Mar 4, 2023

Codemod tests to waitFor pattern (8/?) #26308

Merged

acdlite mentioned this pull request Mar 4, 2023

Codemod tests to waitFor pattern (9/?) #26309

Merged

acdlite mentioned this pull request Mar 4, 2023

Delete toFlushAndYield test helpers #26311

Closed

acdlite mentioned this pull request Mar 7, 2023

Codemod act -> await act (1/?) #26334

Merged

This was referenced Mar 7, 2023

Codemod act -> await act (2/?) #26335

Merged

Codemod act -> await act (3/?) #26336

Merged

Codemod act -> await act (4/?) #26338

Merged

sammy-SC mentioned this pull request Apr 5, 2023

Use native scheduler if defined sammy-SC/react#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New internal testing helpers: waitFor, waitForAll, waitForPaint#26285

New internal testing helpers: waitFor, waitForAll, waitForPaint#26285
acdlite merged 2 commits intofacebook:mainfrom
acdlite:new-test-helpers-waitFor

acdlite commented Mar 2, 2023

Uh oh!

react-sizebot commented Mar 2, 2023 •

edited

Loading

Uh oh!

acdlite commented Mar 2, 2023

Uh oh!

tyao1 left a comment

Uh oh!

Uh oh!

tyao1 Mar 2, 2023

Uh oh!

acdlite Mar 2, 2023

Uh oh!

acdlite commented Mar 2, 2023

Uh oh!

tyao1 commented Mar 2, 2023

Uh oh!

acdlite commented Mar 2, 2023

Uh oh!

gaearon commented Mar 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	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',
	]);

Conversation

acdlite commented Mar 2, 2023

Uh oh!

react-sizebot commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Critical size changes

Significant size changes

Uh oh!

acdlite commented Mar 2, 2023

Uh oh!

tyao1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tyao1 Mar 2, 2023

Choose a reason for hiding this comment

Uh oh!

acdlite Mar 2, 2023

Choose a reason for hiding this comment

Uh oh!

acdlite commented Mar 2, 2023

Uh oh!

tyao1 commented Mar 2, 2023

Uh oh!

acdlite commented Mar 2, 2023

Uh oh!

gaearon commented Mar 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

react-sizebot commented Mar 2, 2023 •

edited

Loading