Skip to content

Commit 1f5cdf8

Browse files
authored
Update Suspense fuzz tests to use act (#26498)
This updates the Suspense fuzz tester to use `act` to recursively flush timers instead of doing it manually. This still isn't great because ideally the fuzz tester wouldn't fake timers at all. It should resolve promises using a custom queue instead of Jest's fake timer queue, like we've started doing in our other Suspense tests (i.e. the `resolveText` pattern). That's because our internal `act` API (not the public one, the one we use in our tests) uses Jest's fake timer queue as a way to force Suspense fallbacks to appear. However I'm not interested in upgrading this test suite to a better strategy right now because if I were writing a Suspense fuzzer today I would probably use an entirely different approach. So this is just an incremental improvement to make it slightly less decoupled to React implementation details.
1 parent f62cb39 commit 1f5cdf8

File tree

2 files changed

+49
-30
lines changed

2 files changed

+49
-30
lines changed

packages/internal-test-utils/internalAct.js

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,19 @@ export async function act<T>(scope: () => Thenable<T>): Thenable<T> {
8080

8181
if (!Scheduler.unstable_hasPendingWork()) {
8282
// $FlowFixMe[cannot-resolve-name]: Flow doesn't know about global Jest object
83-
jest.runOnlyPendingTimers();
83+
const j = jest;
84+
if (j.getTimerCount() > 0) {
85+
// There's a pending timer. Flush it now. We only do this in order to
86+
// force Suspense fallbacks to display; the fact that it's a timer
87+
// is an implementation detail. If there are other timers scheduled,
88+
// those will also fire now, too, which is not ideal. (The public
89+
// version of `act` doesn't do this.) For this reason, we should try
90+
// to avoid using timers in our internal tests.
91+
j.runOnlyPendingTimers();
92+
// If a committing a fallback triggers another update, it might not
93+
// get scheduled until a microtask. So wait one more time.
94+
await waitForMicrotasks();
95+
}
8496
if (Scheduler.unstable_hasPendingWork()) {
8597
// Committing a fallback scheduled additional work. Continue flushing.
8698
} else {

packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js

Lines changed: 36 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ describe('ReactSuspenseFuzz', () => {
3232
Random = require('random-seed');
3333
});
3434

35+
jest.setTimeout(20000);
36+
3537
function createFuzzer() {
3638
const {useState, useContext, useLayoutEffect} = React;
3739

@@ -57,7 +59,6 @@ describe('ReactSuspenseFuzz', () => {
5759
};
5860
const timeoutID = setTimeout(() => {
5961
pendingTasks.delete(task);
60-
Scheduler.log(task.label);
6162
setStep(i + 1);
6263
}, remountAfter);
6364
pendingTasks.add(task);
@@ -87,7 +88,6 @@ describe('ReactSuspenseFuzz', () => {
8788
};
8889
const timeoutID = setTimeout(() => {
8990
pendingTasks.delete(task);
90-
Scheduler.log(task.label);
9191
setStep([i + 1, suspendFor]);
9292
}, beginAfter);
9393
pendingTasks.add(task);
@@ -138,43 +138,50 @@ describe('ReactSuspenseFuzz', () => {
138138
return resolvedText;
139139
}
140140

141-
async function resolveAllTasks() {
142-
Scheduler.unstable_flushAllWithoutAsserting();
143-
let elapsedTime = 0;
144-
while (pendingTasks && pendingTasks.size > 0) {
145-
if ((elapsedTime += 1000) > 1000000) {
146-
throw new Error('Something did not resolve properly.');
147-
}
148-
await act(() => {
149-
ReactNoop.batchedUpdates(() => {
150-
jest.advanceTimersByTime(1000);
151-
});
152-
});
153-
Scheduler.unstable_flushAllWithoutAsserting();
154-
}
155-
}
156-
157141
async function testResolvedOutput(unwrappedChildren) {
158142
const children = (
159143
<Suspense fallback="Loading...">{unwrappedChildren}</Suspense>
160144
);
161145

146+
// Render the app multiple times: once without suspending (as if all the
147+
// data was already preloaded), and then again with suspensey data.
162148
resetCache();
163149
const expectedRoot = ReactNoop.createRoot();
164-
expectedRoot.render(
165-
<ShouldSuspendContext.Provider value={false}>
166-
{children}
167-
</ShouldSuspendContext.Provider>,
168-
);
169-
await resolveAllTasks();
150+
await act(() => {
151+
expectedRoot.render(
152+
<ShouldSuspendContext.Provider value={false}>
153+
{children}
154+
</ShouldSuspendContext.Provider>,
155+
);
156+
});
157+
170158
const expectedOutput = expectedRoot.getChildrenAsJSX();
171159

172160
resetCache();
173-
ReactNoop.renderLegacySyncRoot(children);
174-
await resolveAllTasks();
175-
const legacyOutput = ReactNoop.getChildrenAsJSX();
176-
expect(legacyOutput).toEqual(expectedOutput);
177-
ReactNoop.renderLegacySyncRoot(null);
161+
162+
const concurrentRootThatSuspends = ReactNoop.createRoot();
163+
await act(() => {
164+
concurrentRootThatSuspends.render(children);
165+
});
166+
167+
resetCache();
168+
169+
// Do it again in legacy mode.
170+
const legacyRootThatSuspends = ReactNoop.createLegacyRoot();
171+
await act(() => {
172+
legacyRootThatSuspends.render(children);
173+
});
174+
175+
// Now compare the final output. It should be the same.
176+
expect(concurrentRootThatSuspends.getChildrenAsJSX()).toEqual(
177+
expectedOutput,
178+
);
179+
expect(legacyRootThatSuspends.getChildrenAsJSX()).toEqual(expectedOutput);
180+
181+
// TODO: There are Scheduler logs in this test file but they were only
182+
// added for debugging purposes; we don't make any assertions on them.
183+
// Should probably just delete.
184+
Scheduler.unstable_clearLog();
178185
}
179186

180187
function pickRandomWeighted(rand, options) {

0 commit comments

Comments
 (0)