[act] flush work correctly without a mocked scheduler (#16223)

Not returning the value of flushPassiveEffects() in flushWork() meant that with async act, we wouldn't flush all work with cascading effects. This PR fixes that oversight, and adds some tests to catch this in the future.

Author: Sunil Pai

packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js

@@ -130,10 +130,12 @@ function runActTests(label, render, unmount) {
       container = document.createElement('div');
       document.body.appendChild(container);
     });
+
     afterEach(() => {
       unmount(container);
       document.body.removeChild(container);
     });
+
     describe('sync', () => {
       it('can use act to flush effects', () => {
         function App() {
@@ -240,13 +242,16 @@ function runActTests(label, render, unmount) {
           'An update to App inside a test was not wrapped in act(...).',
         ]);
       });
+
       describe('fake timers', () => {
         beforeEach(() => {
           jest.useFakeTimers();
         });
+
         afterEach(() => {
           jest.useRealTimers();
         });
+
         it('lets a ticker update', () => {
           function App() {
             let [toggle, setToggle] = React.useState(0);
@@ -268,6 +273,7 @@ function runActTests(label, render, unmount) {
 
           expect(container.innerHTML).toBe('1');
         });
+
         it('can use the async version to catch microtasks', async () => {
           function App() {
             let [toggle, setToggle] = React.useState(0);
@@ -289,6 +295,7 @@ function runActTests(label, render, unmount) {
 
           expect(container.innerHTML).toBe('1');
         });
+
         it('can handle cascading promises with fake timers', async () => {
           // this component triggers an effect, that waits a tick,
           // then sets state. repeats this 5 times.
@@ -314,6 +321,7 @@ function runActTests(label, render, unmount) {
           // all 5 ticks present and accounted for
           expect(container.innerHTML).toBe('5');
         });
+
         it('flushes immediate re-renders with act', () => {
           function App() {
             let [ctr, setCtr] = React.useState(0);
@@ -367,6 +375,7 @@ function runActTests(label, render, unmount) {
         );
       });
     });
+
     describe('asynchronous tests', () => {
       it('works with timeouts', async () => {
         function App() {
@@ -577,6 +586,7 @@ function runActTests(label, render, unmount) {
         });
       }
     });
+
     describe('error propagation', () => {
       it('propagates errors - sync', () => {
         let err;

packages/react-dom/src/__tests__/ReactTestUtilsActUnmockedScheduler-test.js

@@ -0,0 +1,166 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @emails react-core
+ */
+
+// sanity tests to make sure act() works without a mocked scheduler
+
+let React;
+let ReactDOM;
+let act;
+let container;
+let yields;
+
+function clearYields() {
+  try {
+    return yields;
+  } finally {
+    yields = [];
+  }
+}
+
+function render(el, dom) {
+  ReactDOM.render(el, dom);
+}
+
+function unmount(dom) {
+  ReactDOM.unmountComponentAtNode(dom);
+}
+
+beforeEach(() => {
+  jest.resetModules();
+  jest.unmock('scheduler');
+  yields = [];
+  React = require('react');
+  ReactDOM = require('react-dom');
+  act = require('react-dom/test-utils').act;
+  container = document.createElement('div');
+  document.body.appendChild(container);
+});
+
+afterEach(() => {
+  unmount(container);
+  document.body.removeChild(container);
+});
+
+it('can use act to flush effects', () => {
+  function App() {
+    React.useEffect(() => {
+      yields.push(100);
+    });
+    return null;
+  }
+
+  act(() => {
+    render(<App />, container);
+  });
+
+  expect(clearYields()).toEqual([100]);
+});
+
+it('flushes effects on every call', () => {
+  function App() {
+    let [ctr, setCtr] = React.useState(0);
+    React.useEffect(() => {
+      yields.push(ctr);
+    });
+    return (
+      <button id="button" onClick={() => setCtr(x => x + 1)}>
+        {ctr}
+      </button>
+    );
+  }
+
+  act(() => {
+    render(<App />, container);
+  });
+
+  expect(clearYields()).toEqual([0]);
+
+  const button = container.querySelector('#button');
+  function click() {
+    button.dispatchEvent(new MouseEvent('click', {bubbles: true}));
+  }
+
+  act(() => {
+    click();
+    click();
+    click();
+  });
+  // it consolidates the 3 updates, then fires the effect
+  expect(clearYields()).toEqual([3]);
+  act(click);
+  expect(clearYields()).toEqual([4]);
+  act(click);
+  expect(clearYields()).toEqual([5]);
+  expect(button.innerHTML).toEqual('5');
+});
+
+it("should keep flushing effects until the're done", () => {
+  function App() {
+    let [ctr, setCtr] = React.useState(0);
+    React.useEffect(() => {
+      if (ctr < 5) {
+        setCtr(x => x + 1);
+      }
+    });
+    return ctr;
+  }
+
+  act(() => {
+    render(<App />, container);
+  });
+
+  expect(container.innerHTML).toEqual('5');
+});
+
+it('should flush effects only on exiting the outermost act', () => {
+  function App() {
+    React.useEffect(() => {
+      yields.push(0);
+    });
+    return null;
+  }
+  // let's nest a couple of act() calls
+  act(() => {
+    act(() => {
+      render(<App />, container);
+    });
+    // the effect wouldn't have yielded yet because
+    // we're still inside an act() scope
+    expect(clearYields()).toEqual([]);
+  });
+  // but after exiting the last one, effects get flushed
+  expect(clearYields()).toEqual([0]);
+});
+
+it('can handle cascading promises', async () => {
+  // this component triggers an effect, that waits a tick,
+  // then sets state. repeats this 5 times.
+  function App() {
+    let [state, setState] = React.useState(0);
+    async function ticker() {
+      await null;
+      setState(x => x + 1);
+    }
+    React.useEffect(
+      () => {
+        yields.push(state);
+        ticker();
+      },
+      [Math.min(state, 4)],
+    );
+    return state;
+  }
+
+  await act(async () => {
+    render(<App />, container);
+  });
+  // all 5 ticks present and accounted for
+  expect(clearYields()).toEqual([0, 1, 2, 3, 4]);
+  expect(container.innerHTML).toBe('5');
+});

packages/react-dom/src/test-utils/ReactTestUtilsAct.js

@@ -63,7 +63,13 @@ const flushWork =
         hasWarnedAboutMissingMockScheduler = true;
       }
     }
-    while (flushPassiveEffects()) {}
+
+    let didFlushWork = false;
+    while (flushPassiveEffects()) {
+      didFlushWork = true;
+    }
+
+    return didFlushWork;
   };
 
 function flushWorkAndMicroTasks(onDone: (err: ?Error) => void) {

packages/react-noop-renderer/src/createReactNoop.js

@@ -619,7 +619,13 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
           hasWarnedAboutMissingMockScheduler = true;
         }
       }
-      while (flushPassiveEffects()) {}
+
+      let didFlushWork = false;
+      while (flushPassiveEffects()) {
+        didFlushWork = true;
+      }
+
+      return didFlushWork;
     };
 
   function flushWorkAndMicroTasks(onDone: (err: ?Error) => void) {

packages/react-test-renderer/src/ReactTestRendererAct.js

@@ -44,7 +44,13 @@ const flushWork =
         hasWarnedAboutMissingMockScheduler = true;
       }
     }
-    while (flushPassiveEffects()) {}
+
+    let didFlushWork = false;
+    while (flushPassiveEffects()) {
+      didFlushWork = true;
+    }
+
+    return didFlushWork;
   };
 
 function flushWorkAndMicroTasks(onDone: (err: ?Error) => void) {