flush fallbacks in tests (#16240)

Sunil Pai · web-flow · commit db3ae32b8fe2 · 2019-07-30T19:12:06.000+01:00
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.
diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
@@ -32,36 +32,63 @@ 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;
   function renderBatched(el, dom) {
     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(<Component />, container);
+                rerender(<Component />);
               },
             );
           });
@@ -576,7 +603,7 @@ function runActTests(label, render, unmount) {
                 expect(interactions.size).toBe(1);
                 expectedInteraction = Array.from(interactions)[0];
 
-                render(<Component />, secondContainer);
+                rerender(<Component />);
               });
             },
           );
@@ -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 (
+            <React.Suspense
+              fallback={<span data-test-id="spinner">loading...</span>}>
+              {props.suspend ? <Suspends /> : 'content'}
+            </React.Suspense>
+          );
+        }
+
+        // render something so there's content
+        act(() => {
+          render(<App suspend={false} />, container);
+        });
+
+        // trigger a suspendy update
+        act(() => {
+          rerender(<App suspend={true} />);
+        });
+        expect(document.querySelector('[data-test-id=spinner]')).not.toBeNull();
+
+        // now render regular content again
+        act(() => {
+          rerender(<App suspend={false} />);
+        });
+        expect(document.querySelector('[data-test-id=spinner]')).toBeNull();
+
+        // trigger a suspendy update with a delay
+        React.unstable_withSuspenseConfig(
+          () => {
+            act(() => {
+              rerender(<App suspend={true} />);
+            });
+          },
+          {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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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;
