From ad9b407558f98113d0b42bf34e4e78eb930ecd52 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Mon, 29 Jul 2019 12:19:21 +0100 Subject: [PATCH] flush fallbacks in tests In this PR, for tests (specifically, code inside an `act()` scope), we immediately trigger work that would have otherwise required a timeout. This makes it simpler to tests loading/spinner states, and makes tests resilient to changes in React. For some of our tests(specifically, ReactSuspenseWithNoopRenderer-test.internal), we _don't_ want fallbacks to immediately trigger, because we're testing intermediate states and such. Added a feature flag `flushSuspenseFallbacksInTests` to disable this behaviour on a per case basis. --- .../src/__tests__/ReactTestUtilsAct-test.js | 104 +++++++++++++++++- .../src/ReactFiberWorkLoop.js | 19 +++- ...tSuspenseWithNoopRenderer-test.internal.js | 1 + packages/shared/ReactFeatureFlags.js | 4 + .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.persistent.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 1 + .../shared/forks/ReactFeatureFlags.www.js | 2 + 10 files changed, 126 insertions(+), 9 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index c369eb97ed4c..883ee65a243e 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -32,22 +32,43 @@ describe('ReactTestUtils.act()', () => { concurrentRoot = ReactDOM.unstable_createRoot(dom); concurrentRoot.render(el); } + function unmountConcurrent(_dom) { if (concurrentRoot !== null) { concurrentRoot.unmount(); concurrentRoot = null; } } - runActTests('concurrent mode', renderConcurrent, unmountConcurrent); + + function rerenderConcurrent(el) { + concurrentRoot.render(el); + } + + runActTests( + 'concurrent mode', + renderConcurrent, + unmountConcurrent, + rerenderConcurrent, + ); // and then in sync mode + + let syncDom = null; function renderSync(el, dom) { + syncDom = dom; ReactDOM.render(el, dom); } + function unmountSync(dom) { + syncDom = null; ReactDOM.unmountComponentAtNode(dom); } - runActTests('legacy sync mode', renderSync, unmountSync); + + function rerenderSync(el) { + ReactDOM.render(el, syncDom); + } + + runActTests('legacy sync mode', renderSync, unmountSync, rerenderSync); // and then in batched mode let batchedRoot; @@ -55,13 +76,19 @@ describe('ReactTestUtils.act()', () => { batchedRoot = ReactDOM.unstable_createSyncRoot(dom); batchedRoot.render(el); } + function unmountBatched(dom) { if (batchedRoot !== null) { batchedRoot.unmount(); batchedRoot = null; } } - runActTests('batched mode', renderBatched, unmountBatched); + + function rerenderBatched(el) { + batchedRoot.render(el); + } + + runActTests('batched mode', renderBatched, unmountBatched, rerenderBatched); describe('unacted effects', () => { function App() { @@ -117,7 +144,7 @@ describe('ReactTestUtils.act()', () => { }); }); -function runActTests(label, render, unmount) { +function runActTests(label, render, unmount, rerender) { describe(label, () => { beforeEach(() => { jest.resetModules(); @@ -546,7 +573,7 @@ function runActTests(label, render, unmount) { expect(interactions.size).toBe(1); expectedInteraction = Array.from(interactions)[0]; - render(, container); + rerender(); }, ); }); @@ -576,7 +603,7 @@ function runActTests(label, render, unmount) { expect(interactions.size).toBe(1); expectedInteraction = Array.from(interactions)[0]; - render(, secondContainer); + rerender(); }); }, ); @@ -693,5 +720,70 @@ function runActTests(label, render, unmount) { } }); }); + + describe('suspense', () => { + it('triggers fallbacks if available', async () => { + let resolved = false; + let resolve; + const promise = new Promise(_resolve => { + resolve = _resolve; + }); + + function Suspends() { + if (resolved) { + return 'was suspended'; + } + throw promise; + } + + function App(props) { + return ( + loading...}> + {props.suspend ? : 'content'} + + ); + } + + // render something so there's content + act(() => { + render(, container); + }); + + // trigger a suspendy update + act(() => { + rerender(); + }); + expect(document.querySelector('[data-test-id=spinner]')).not.toBeNull(); + + // now render regular content again + act(() => { + rerender(); + }); + expect(document.querySelector('[data-test-id=spinner]')).toBeNull(); + + // trigger a suspendy update with a delay + React.unstable_withSuspenseConfig( + () => { + act(() => { + rerender(); + }); + }, + {timeout: 5000}, + ); + // the spinner shows up regardless + expect(document.querySelector('[data-test-id=spinner]')).not.toBeNull(); + + // resolve the promise + await act(async () => { + resolved = true; + resolve(); + }); + + // spinner gone, content showing + expect(document.querySelector('[data-test-id=spinner]')).toBeNull(); + expect(container.textContent).toBe('was suspended'); + }); + }); }); } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 53442df6e70e..0af8817ff98e 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -26,6 +26,7 @@ import { enableSchedulerTracing, revertPassiveEffectsChange, warnAboutUnmockedScheduler, + flushSuspenseFallbacksInTests, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import invariant from 'shared/invariant'; @@ -993,7 +994,7 @@ function renderRoot( case RootIncomplete: { invariant(false, 'Should have a work-in-progress.'); } - // Flow knows about invariant, so it compains if I add a break statement, + // Flow knows about invariant, so it complains if I add a break statement, // but eslint doesn't know about invariant, so it complains if I do. // eslint-disable-next-line no-fallthrough case RootErrored: { @@ -1027,7 +1028,12 @@ function renderRoot( // possible. const hasNotProcessedNewUpdates = workInProgressRootLatestProcessedExpirationTime === Sync; - if (hasNotProcessedNewUpdates && !isSync) { + if ( + hasNotProcessedNewUpdates && + !isSync && + // do not delay if we're inside an act() scope + !(flushSuspenseFallbacksInTests && IsThisRendererActing.current) + ) { // If we have not processed any new updates during this pass, then this is // either a retry of an existing fallback state or a hidden tree. // Hidden trees shouldn't be batched with other work and after that's @@ -1064,7 +1070,11 @@ function renderRoot( return commitRoot.bind(null, root); } case RootSuspendedWithDelay: { - if (!isSync) { + if ( + !isSync && + // do not delay if we're inside an act() scope + !(flushSuspenseFallbacksInTests && IsThisRendererActing.current) + ) { // We're suspended in a state that should be avoided. We'll try to avoid committing // it for as long as the timeouts let us. if (workInProgressRootHasPendingPing) { @@ -1135,6 +1145,8 @@ function renderRoot( // The work completed. Ready to commit. if ( !isSync && + // do not delay if we're inside an act() scope + !(flushSuspenseFallbacksInTests && IsThisRendererActing.current) && workInProgressRootLatestProcessedExpirationTime !== Sync && workInProgressRootCanSuspendUsingConfig !== null ) { @@ -2439,6 +2451,7 @@ function warnAboutInvalidUpdatesOnClassComponentsInDEV(fiber) { } } +// a 'shared' variable that changes when act() opens/closes in tests. export const IsThisRendererActing = {current: (false: boolean)}; export function warnIfNotScopedWithMatchingAct(fiber: Fiber): void { diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 8c9773009b2b..c95e058bb6af 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -15,6 +15,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false; + ReactFeatureFlags.flushSuspenseFallbacksInTests = false; React = require('react'); Fragment = React.Fragment; ReactNoop = require('react-noop-renderer'); diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 083da1b75075..6dc443b3e7ed 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -74,6 +74,10 @@ export const warnAboutUnmockedScheduler = false; // Temporary flag to revert the fix in #15650 export const revertPassiveEffectsChange = false; +// For tests, we flush suspense fallbacks in an act scope; +// *except* in some of our own tests, where we test incremental loading states. +export const flushSuspenseFallbacksInTests = true; + // Changes priority of some events like mousemove to user-blocking priority, // but without making them discrete. The flag exists in case it causes // starvation problems. diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 427db540edc8..7c6783463aa5 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -36,6 +36,7 @@ export const enableFundamentalAPI = false; export const enableJSXTransformAPI = false; export const warnAboutUnmockedScheduler = true; export const revertPassiveEffectsChange = false; +export const flushSuspenseFallbacksInTests = true; export const enableUserBlockingEvents = false; export const enableSuspenseCallback = false; export const warnAboutDefaultPropsOnFunctionComponents = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index cf69b2dc91cb..291e5d9c58b8 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -31,6 +31,7 @@ export const enableFundamentalAPI = false; export const enableJSXTransformAPI = false; export const warnAboutUnmockedScheduler = false; export const revertPassiveEffectsChange = false; +export const flushSuspenseFallbacksInTests = true; export const enableUserBlockingEvents = false; export const enableSuspenseCallback = false; export const warnAboutDefaultPropsOnFunctionComponents = false; diff --git a/packages/shared/forks/ReactFeatureFlags.persistent.js b/packages/shared/forks/ReactFeatureFlags.persistent.js index 7258b2ae1a7c..e62a6f88dc76 100644 --- a/packages/shared/forks/ReactFeatureFlags.persistent.js +++ b/packages/shared/forks/ReactFeatureFlags.persistent.js @@ -31,6 +31,7 @@ export const enableFundamentalAPI = false; export const enableJSXTransformAPI = false; export const warnAboutUnmockedScheduler = true; export const revertPassiveEffectsChange = false; +export const flushSuspenseFallbacksInTests = true; export const enableUserBlockingEvents = false; export const enableSuspenseCallback = false; export const warnAboutDefaultPropsOnFunctionComponents = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index e9e16e4b3f7d..8b8013db7bd3 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -31,6 +31,7 @@ export const enableFundamentalAPI = false; export const enableJSXTransformAPI = false; export const warnAboutUnmockedScheduler = false; export const revertPassiveEffectsChange = false; +export const flushSuspenseFallbacksInTests = true; export const enableUserBlockingEvents = false; export const enableSuspenseCallback = false; export const warnAboutDefaultPropsOnFunctionComponents = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 86222406e391..813691c9dfb1 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -31,6 +31,7 @@ export const enableFlareAPI = true; export const enableFundamentalAPI = false; export const enableJSXTransformAPI = true; export const warnAboutUnmockedScheduler = true; +export const flushSuspenseFallbacksInTests = true; export const enableUserBlockingEvents = false; export const enableSuspenseCallback = true; export const warnAboutDefaultPropsOnFunctionComponents = false; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index f0123e67b41a..e176657cad28 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -80,6 +80,8 @@ export const enableSuspenseCallback = true; export const warnAboutDefaultPropsOnFunctionComponents = false; +export const flushSuspenseFallbacksInTests = true; + // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars type Check<_X, Y: _X, X: Y = _X> = null;