Skip to content

Commit

Permalink
Update Suspense fuzz tests to use act
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 committed Mar 28, 2023
1 parent fc90eb6 commit 99ea7d9
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 99ea7d9

Please sign in to comment.