From 1f5cdf8c77182fc51910787e48384ec4620dc40d Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 28 Mar 2023 14:40:28 -0400 Subject: [PATCH] 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. --- packages/internal-test-utils/internalAct.js | 14 +++- .../ReactSuspenseFuzz-test.internal.js | 65 ++++++++++--------- 2 files changed, 49 insertions(+), 30 deletions(-) diff --git a/packages/internal-test-utils/internalAct.js b/packages/internal-test-utils/internalAct.js index 6794e1b64cec8..082be156dc89b 100644 --- a/packages/internal-test-utils/internalAct.js +++ b/packages/internal-test-utils/internalAct.js @@ -80,7 +80,19 @@ export async function act(scope: () => Thenable): Thenable { 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 { diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js index 9ec744b430feb..ce83b244eeba5 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js @@ -32,6 +32,8 @@ describe('ReactSuspenseFuzz', () => { Random = require('random-seed'); }); + jest.setTimeout(20000); + function createFuzzer() { const {useState, useContext, useLayoutEffect} = React; @@ -57,7 +59,6 @@ describe('ReactSuspenseFuzz', () => { }; const timeoutID = setTimeout(() => { pendingTasks.delete(task); - Scheduler.log(task.label); setStep(i + 1); }, remountAfter); pendingTasks.add(task); @@ -87,7 +88,6 @@ describe('ReactSuspenseFuzz', () => { }; const timeoutID = setTimeout(() => { pendingTasks.delete(task); - Scheduler.log(task.label); setStep([i + 1, suspendFor]); }, beginAfter); pendingTasks.add(task); @@ -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 = ( {unwrappedChildren} ); + // 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( - - {children} - , - ); - await resolveAllTasks(); + await act(() => { + expectedRoot.render( + + {children} + , + ); + }); + 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) {