Skip to content

Commit

Permalink
Prefer JSX in ReactNoop assertions (to combat out-of-memory test runs) (
Browse files Browse the repository at this point in the history
#26127)

## Summary

Prefer `getChildrenAsJSX` or `toMatchRenderedOutput` over `getChildren`.
Use `dangerouslyGetChildren` if you really need to (e.g. for `toBe`
assertions).

Prefer `getPendingChildrenAsJSX` over `getPendingChildren`. Use
`dangerouslyGetPendingChildren` if you really need to (e.g. for `toBe`
assertions).

`ReactNoop.getChildren` contains the fibers as non-enumerable
properties. If you pass the children to `toEqual` and have a mismatch,
Jest performance is very poor (to the point of causing out-of-memory
crashes e.g.
https://app.circleci.com/pipelines/github/facebook/react/38084/workflows/02ca0cbb-bab4-4c19-8d7d-ada814eeebb9/jobs/624297/parallel-runs/5?filterBy=ALL&invite=true#step-106-27).
Mismatches can sometimes be intended e.g. on gated tests.

Instead, I converted almost all of the `toEqual` assertions to
`toMatchRenderedOutput` assertions or compare the JSX instead. For
ReactNoopPersistent we still use `getChildren` since we have assertions
on referential equality. `toMatchRenderedOutput` is more accurate in
some instances anyway. I highlighted some of those more accurate
assertions in review-comments.

## How did you test this change?

- [x] `CIRCLE_NODE_TOTAL=20 CIRCLE_NODE_INDEX=5 yarn test
-r=experimental --env=development --ci`: Can take up to 350s (and use up
to 7GB of memory) on `main` but 11s on this branch
- [x] No more slow `yarn test` parallel runs of `yarn_test` jobs (the
steps in these runs should take <1min but sometimes they take 3min and
end with OOM like
https://app.circleci.com/pipelines/github/facebook/react/38084/workflows/02ca0cbb-bab4-4c19-8d7d-ada814eeebb9/jobs/624258/parallel-runs/5?filterBy=ALL:
Looks good with a sample size of 1
https://app.circleci.com/pipelines/github/facebook/react/38110/workflows/745109a2-b86b-429f-8c01-9b23a245417a/jobs/624651
  • Loading branch information
eps1lon authored Feb 9, 2023
1 parent 78d2e9e commit 3ff1540
Show file tree
Hide file tree
Showing 20 changed files with 1,985 additions and 1,372 deletions.
2 changes: 2 additions & 0 deletions packages/react-noop-renderer/src/ReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ import createReactNoop from './createReactNoop';
export const {
_Scheduler,
getChildren,
dangerouslyGetChildren,
getPendingChildren,
dangerouslyGetPendingChildren,
getOrCreateRootContainer,
createRoot,
createLegacyRoot,
Expand Down
2 changes: 2 additions & 0 deletions packages/react-noop-renderer/src/ReactNoopPersistent.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ import createReactNoop from './createReactNoop';
export const {
_Scheduler,
getChildren,
dangerouslyGetChildren,
getPendingChildren,
dangerouslyGetPendingChildren,
getOrCreateRootContainer,
createRoot,
createLegacyRoot,
Expand Down
26 changes: 25 additions & 1 deletion packages/react-noop-renderer/src/createReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -789,11 +789,35 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
_Scheduler: Scheduler,

getChildren(rootID: string = DEFAULT_ROOT_ID) {
throw new Error(
'No longer supported due to bad performance when used with `expect()`. ' +
'Use `ReactNoop.getChildrenAsJSX()` instead or, if you really need to, `dangerouslyGetChildren` after you carefully considered the warning in its JSDOC.',
);
},

getPendingChildren(rootID: string = DEFAULT_ROOT_ID) {
throw new Error(
'No longer supported due to bad performance when used with `expect()`. ' +
'Use `ReactNoop.getPendingChildrenAsJSX()` instead or, if you really need to, `dangerouslyGetPendingChildren` after you carefully considered the warning in its JSDOC.',
);
},

/**
* Prefer using `getChildrenAsJSX`.
* Using the returned children in `.toEqual` has very poor performance on mismatch due to deep equality checking of fiber structures.
* Make sure you deeply remove enumerable properties before passing it to `.toEqual`, or, better, use `getChildrenAsJSX` or `toMatchRenderedOutput`.
*/
dangerouslyGetChildren(rootID: string = DEFAULT_ROOT_ID) {
const container = rootContainers.get(rootID);
return getChildren(container);
},

getPendingChildren(rootID: string = DEFAULT_ROOT_ID) {
/**
* Prefer using `getPendingChildrenAsJSX`.
* Using the returned children in `.toEqual` has very poor performance on mismatch due to deep equality checking of fiber structures.
* Make sure you deeply remove enumerable properties before passing it to `.toEqual`, or, better, use `getChildrenAsJSX` or `toMatchRenderedOutput`.
*/
dangerouslyGetPendingChildren(rootID: string = DEFAULT_ROOT_ID) {
const container = rootContainers.get(rootID);
return getPendingChildren(container);
},
Expand Down
24 changes: 10 additions & 14 deletions packages/react-reconciler/src/__tests__/ReactExpiration-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,6 @@ describe('ReactExpiration', () => {
}
}

function span(prop) {
return {type: 'span', children: [], prop, hidden: false};
}

function flushNextRenderIfExpired() {
// This will start rendering the next level of work. If the work hasn't
// expired yet, React will exit without doing anything. If it has expired,
Expand All @@ -127,21 +123,21 @@ describe('ReactExpiration', () => {
ReactNoop.render(<span prop="done" />);
}

expect(ReactNoop.getChildren()).toEqual([]);
expect(ReactNoop).toMatchRenderedOutput(null);

// Nothing has expired yet because time hasn't advanced.
flushNextRenderIfExpired();
expect(ReactNoop.getChildren()).toEqual([]);
expect(ReactNoop).toMatchRenderedOutput(null);

// Advance time a bit, but not enough to expire the low pri update.
ReactNoop.expire(4500);
flushNextRenderIfExpired();
expect(ReactNoop.getChildren()).toEqual([]);
expect(ReactNoop).toMatchRenderedOutput(null);

// Advance by another second. Now the update should expire and flush.
ReactNoop.expire(500);
flushNextRenderIfExpired();
expect(ReactNoop.getChildren()).toEqual([span('done')]);
expect(ReactNoop).toMatchRenderedOutput(<span prop="done" />);
});

it('two updates of like priority in the same event always flush within the same batch', () => {
Expand Down Expand Up @@ -181,20 +177,20 @@ describe('ReactExpiration', () => {

// Don't advance time by enough to expire the first update.
expect(Scheduler).toHaveYielded([]);
expect(ReactNoop.getChildren()).toEqual([]);
expect(ReactNoop).toMatchRenderedOutput(null);

// Schedule another update.
ReactNoop.render(<TextClass text="B" />);
// Both updates are batched
expect(Scheduler).toFlushAndYield(['B [render]', 'B [commit]']);
expect(ReactNoop.getChildren()).toEqual([span('B')]);
expect(ReactNoop).toMatchRenderedOutput(<span prop="B" />);

// Now do the same thing again, except this time don't flush any work in
// between the two updates.
ReactNoop.render(<TextClass text="A" />);
Scheduler.unstable_advanceTime(2000);
expect(Scheduler).toHaveYielded([]);
expect(ReactNoop.getChildren()).toEqual([span('B')]);
expect(ReactNoop).toMatchRenderedOutput(<span prop="B" />);
// Schedule another update.
ReactNoop.render(<TextClass text="B" />);
// The updates should flush in the same batch, since as far as the scheduler
Expand Down Expand Up @@ -242,20 +238,20 @@ describe('ReactExpiration', () => {

// Don't advance time by enough to expire the first update.
expect(Scheduler).toHaveYielded([]);
expect(ReactNoop.getChildren()).toEqual([]);
expect(ReactNoop).toMatchRenderedOutput(null);

// Schedule another update.
ReactNoop.render(<TextClass text="B" />);
// Both updates are batched
expect(Scheduler).toFlushAndYield(['B [render]', 'B [commit]']);
expect(ReactNoop.getChildren()).toEqual([span('B')]);
expect(ReactNoop).toMatchRenderedOutput(<span prop="B" />);

// Now do the same thing again, except this time don't flush any work in
// between the two updates.
ReactNoop.render(<TextClass text="A" />);
Scheduler.unstable_advanceTime(2000);
expect(Scheduler).toHaveYielded([]);
expect(ReactNoop.getChildren()).toEqual([span('B')]);
expect(ReactNoop).toMatchRenderedOutput(<span prop="B" />);

// Perform some synchronous work. The scheduler must assume we're inside
// the same event.
Expand Down
Loading

0 comments on commit 3ff1540

Please sign in to comment.