Skip to content

Commit

Permalink
Update Suspense fuzz tests to use act (#26498)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite authored Mar 28, 2023
1 parent f62cb39 commit 1f5cdf8
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 30 deletions.
14 changes: 13 additions & 1 deletion packages/internal-test-utils/internalAct.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,19 @@ export async function act<T>(scope: () => Thenable<T>): Thenable<T> {

if (!Scheduler.unstable_hasPendingWork()) {
// $FlowFixMe[cannot-resolve-name]: Flow doesn't know about global Jest object
jest.runOnlyPendingTimers();
const j = jest;
if (j.getTimerCount() > 0) {
// There's a pending timer. Flush it now. We only do this in order to
// force Suspense fallbacks to display; the fact that it's a timer
// is an implementation detail. If there are other timers scheduled,
// those will also fire now, too, which is not ideal. (The public
// version of `act` doesn't do this.) For this reason, we should try
// to avoid using timers in our internal tests.
j.runOnlyPendingTimers();
// If a committing a fallback triggers another update, it might not
// get scheduled until a microtask. So wait one more time.
await waitForMicrotasks();
}
if (Scheduler.unstable_hasPendingWork()) {
// Committing a fallback scheduled additional work. Continue flushing.
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ describe('ReactSuspenseFuzz', () => {
Random = require('random-seed');
});

jest.setTimeout(20000);

function createFuzzer() {
const {useState, useContext, useLayoutEffect} = React;

Expand All @@ -57,7 +59,6 @@ describe('ReactSuspenseFuzz', () => {
};
const timeoutID = setTimeout(() => {
pendingTasks.delete(task);
Scheduler.log(task.label);
setStep(i + 1);
}, remountAfter);
pendingTasks.add(task);
Expand Down Expand Up @@ -87,7 +88,6 @@ describe('ReactSuspenseFuzz', () => {
};
const timeoutID = setTimeout(() => {
pendingTasks.delete(task);
Scheduler.log(task.label);
setStep([i + 1, suspendFor]);
}, beginAfter);
pendingTasks.add(task);
Expand Down Expand Up @@ -138,43 +138,50 @@ describe('ReactSuspenseFuzz', () => {
return resolvedText;
}

async function resolveAllTasks() {
Scheduler.unstable_flushAllWithoutAsserting();
let elapsedTime = 0;
while (pendingTasks && pendingTasks.size > 0) {
if ((elapsedTime += 1000) > 1000000) {
throw new Error('Something did not resolve properly.');
}
await act(() => {
ReactNoop.batchedUpdates(() => {
jest.advanceTimersByTime(1000);
});
});
Scheduler.unstable_flushAllWithoutAsserting();
}
}

async function testResolvedOutput(unwrappedChildren) {
const children = (
<Suspense fallback="Loading...">{unwrappedChildren}</Suspense>
);

// Render the app multiple times: once without suspending (as if all the
// data was already preloaded), and then again with suspensey data.
resetCache();
const expectedRoot = ReactNoop.createRoot();
expectedRoot.render(
<ShouldSuspendContext.Provider value={false}>
{children}
</ShouldSuspendContext.Provider>,
);
await resolveAllTasks();
await act(() => {
expectedRoot.render(
<ShouldSuspendContext.Provider value={false}>
{children}
</ShouldSuspendContext.Provider>,
);
});

const expectedOutput = expectedRoot.getChildrenAsJSX();

resetCache();
ReactNoop.renderLegacySyncRoot(children);
await resolveAllTasks();
const legacyOutput = ReactNoop.getChildrenAsJSX();
expect(legacyOutput).toEqual(expectedOutput);
ReactNoop.renderLegacySyncRoot(null);

const concurrentRootThatSuspends = ReactNoop.createRoot();
await act(() => {
concurrentRootThatSuspends.render(children);
});

resetCache();

// Do it again in legacy mode.
const legacyRootThatSuspends = ReactNoop.createLegacyRoot();
await act(() => {
legacyRootThatSuspends.render(children);
});

// Now compare the final output. It should be the same.
expect(concurrentRootThatSuspends.getChildrenAsJSX()).toEqual(
expectedOutput,
);
expect(legacyRootThatSuspends.getChildrenAsJSX()).toEqual(expectedOutput);

// TODO: There are Scheduler logs in this test file but they were only
// added for debugging purposes; we don't make any assertions on them.
// Should probably just delete.
Scheduler.unstable_clearLog();
}

function pickRandomWeighted(rand, options) {
Expand Down

0 comments on commit 1f5cdf8

Please sign in to comment.