Skip to content

Commit c672128

Browse files
committed
Update Suspense fuzz tests to use act
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 fc90eb6 commit c672128

File tree

2 files changed

+47
-30
lines changed

2 files changed

+47
-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: 34 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@ describe('ReactSuspenseFuzz', () => {
5757
};
5858
const timeoutID = setTimeout(() => {
5959
pendingTasks.delete(task);
60-
Scheduler.log(task.label);
6160
setStep(i + 1);
6261
}, remountAfter);
6362
pendingTasks.add(task);
@@ -87,7 +86,6 @@ describe('ReactSuspenseFuzz', () => {
8786
};
8887
const timeoutID = setTimeout(() => {
8988
pendingTasks.delete(task);
90-
Scheduler.log(task.label);
9189
setStep([i + 1, suspendFor]);
9290
}, beginAfter);
9391
pendingTasks.add(task);
@@ -138,43 +136,50 @@ describe('ReactSuspenseFuzz', () => {
138136
return resolvedText;
139137
}
140138

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-
157139
async function testResolvedOutput(unwrappedChildren) {
158140
const children = (
159141
<Suspense fallback="Loading...">{unwrappedChildren}</Suspense>
160142
);
161143

144+
// Render the app multiple times: once without suspending (as if all the
145+
// data was already preloaded), and then again with suspensey data.
162146
resetCache();
163147
const expectedRoot = ReactNoop.createRoot();
164-
expectedRoot.render(
165-
<ShouldSuspendContext.Provider value={false}>
166-
{children}
167-
</ShouldSuspendContext.Provider>,
168-
);
169-
await resolveAllTasks();
148+
await act(() => {
149+
expectedRoot.render(
150+
<ShouldSuspendContext.Provider value={false}>
151+
{children}
152+
</ShouldSuspendContext.Provider>,
153+
);
154+
});
155+
170156
const expectedOutput = expectedRoot.getChildrenAsJSX();
171157

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

180185
function pickRandomWeighted(rand, options) {

0 commit comments

Comments
 (0)