Skip to content

Commit 3ff1540

Browse files
authored
Prefer JSX in ReactNoop assertions (to combat out-of-memory test runs) (#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
1 parent 78d2e9e commit 3ff1540

20 files changed

+1985
-1372
lines changed

packages/react-noop-renderer/src/ReactNoop.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,9 @@ import createReactNoop from './createReactNoop';
2020
export const {
2121
_Scheduler,
2222
getChildren,
23+
dangerouslyGetChildren,
2324
getPendingChildren,
25+
dangerouslyGetPendingChildren,
2426
getOrCreateRootContainer,
2527
createRoot,
2628
createLegacyRoot,

packages/react-noop-renderer/src/ReactNoopPersistent.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,9 @@ import createReactNoop from './createReactNoop';
2020
export const {
2121
_Scheduler,
2222
getChildren,
23+
dangerouslyGetChildren,
2324
getPendingChildren,
25+
dangerouslyGetPendingChildren,
2426
getOrCreateRootContainer,
2527
createRoot,
2628
createLegacyRoot,

packages/react-noop-renderer/src/createReactNoop.js

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -789,11 +789,35 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
789789
_Scheduler: Scheduler,
790790

791791
getChildren(rootID: string = DEFAULT_ROOT_ID) {
792+
throw new Error(
793+
'No longer supported due to bad performance when used with `expect()`. ' +
794+
'Use `ReactNoop.getChildrenAsJSX()` instead or, if you really need to, `dangerouslyGetChildren` after you carefully considered the warning in its JSDOC.',
795+
);
796+
},
797+
798+
getPendingChildren(rootID: string = DEFAULT_ROOT_ID) {
799+
throw new Error(
800+
'No longer supported due to bad performance when used with `expect()`. ' +
801+
'Use `ReactNoop.getPendingChildrenAsJSX()` instead or, if you really need to, `dangerouslyGetPendingChildren` after you carefully considered the warning in its JSDOC.',
802+
);
803+
},
804+
805+
/**
806+
* Prefer using `getChildrenAsJSX`.
807+
* Using the returned children in `.toEqual` has very poor performance on mismatch due to deep equality checking of fiber structures.
808+
* Make sure you deeply remove enumerable properties before passing it to `.toEqual`, or, better, use `getChildrenAsJSX` or `toMatchRenderedOutput`.
809+
*/
810+
dangerouslyGetChildren(rootID: string = DEFAULT_ROOT_ID) {
792811
const container = rootContainers.get(rootID);
793812
return getChildren(container);
794813
},
795814

796-
getPendingChildren(rootID: string = DEFAULT_ROOT_ID) {
815+
/**
816+
* Prefer using `getPendingChildrenAsJSX`.
817+
* Using the returned children in `.toEqual` has very poor performance on mismatch due to deep equality checking of fiber structures.
818+
* Make sure you deeply remove enumerable properties before passing it to `.toEqual`, or, better, use `getChildrenAsJSX` or `toMatchRenderedOutput`.
819+
*/
820+
dangerouslyGetPendingChildren(rootID: string = DEFAULT_ROOT_ID) {
797821
const container = rootContainers.get(rootID);
798822
return getPendingChildren(container);
799823
},

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

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -105,10 +105,6 @@ describe('ReactExpiration', () => {
105105
}
106106
}
107107

108-
function span(prop) {
109-
return {type: 'span', children: [], prop, hidden: false};
110-
}
111-
112108
function flushNextRenderIfExpired() {
113109
// This will start rendering the next level of work. If the work hasn't
114110
// expired yet, React will exit without doing anything. If it has expired,
@@ -127,21 +123,21 @@ describe('ReactExpiration', () => {
127123
ReactNoop.render(<span prop="done" />);
128124
}
129125

130-
expect(ReactNoop.getChildren()).toEqual([]);
126+
expect(ReactNoop).toMatchRenderedOutput(null);
131127

132128
// Nothing has expired yet because time hasn't advanced.
133129
flushNextRenderIfExpired();
134-
expect(ReactNoop.getChildren()).toEqual([]);
130+
expect(ReactNoop).toMatchRenderedOutput(null);
135131

136132
// Advance time a bit, but not enough to expire the low pri update.
137133
ReactNoop.expire(4500);
138134
flushNextRenderIfExpired();
139-
expect(ReactNoop.getChildren()).toEqual([]);
135+
expect(ReactNoop).toMatchRenderedOutput(null);
140136

141137
// Advance by another second. Now the update should expire and flush.
142138
ReactNoop.expire(500);
143139
flushNextRenderIfExpired();
144-
expect(ReactNoop.getChildren()).toEqual([span('done')]);
140+
expect(ReactNoop).toMatchRenderedOutput(<span prop="done" />);
145141
});
146142

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

182178
// Don't advance time by enough to expire the first update.
183179
expect(Scheduler).toHaveYielded([]);
184-
expect(ReactNoop.getChildren()).toEqual([]);
180+
expect(ReactNoop).toMatchRenderedOutput(null);
185181

186182
// Schedule another update.
187183
ReactNoop.render(<TextClass text="B" />);
188184
// Both updates are batched
189185
expect(Scheduler).toFlushAndYield(['B [render]', 'B [commit]']);
190-
expect(ReactNoop.getChildren()).toEqual([span('B')]);
186+
expect(ReactNoop).toMatchRenderedOutput(<span prop="B" />);
191187

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

243239
// Don't advance time by enough to expire the first update.
244240
expect(Scheduler).toHaveYielded([]);
245-
expect(ReactNoop.getChildren()).toEqual([]);
241+
expect(ReactNoop).toMatchRenderedOutput(null);
246242

247243
// Schedule another update.
248244
ReactNoop.render(<TextClass text="B" />);
249245
// Both updates are batched
250246
expect(Scheduler).toFlushAndYield(['B [render]', 'B [commit]']);
251-
expect(ReactNoop.getChildren()).toEqual([span('B')]);
247+
expect(ReactNoop).toMatchRenderedOutput(<span prop="B" />);
252248

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

260256
// Perform some synchronous work. The scheduler must assume we're inside
261257
// the same event.

0 commit comments

Comments
 (0)