From c73e1f236f932c76fbed5be87bf0fd91da6c0549 Mon Sep 17 00:00:00 2001
From: Sunil Pai <threepointone@fb.com>
Date: Wed, 24 Jul 2019 00:20:28 +0100
Subject: [PATCH] flush work on exiting outermost act(), with nested act()s
 from different renderers (#16181)

Given this snippet:
```jsx
    TestRenderer.act(() => {
      TestUtils.act(() => {
        TestRenderer.create(<Effecty />);
      });
    });
```
We want to make sure that all work is only flushed on exiting the outermost act().

Now, naively doing this based on actingScopeDepth would work with a mocked scheduler, where flushAll() would flush all work across renderers.

This doesn't work without mocking the scheduler though; and where flushing work only works per renderer. So we disable this behaviour for a non-mocked scenario. This seems like an ok tradeoff.
---
 fixtures/dom/src/index.test.js                | 308 +++++++++++-------
 .../src/test-utils/ReactTestUtilsAct.js       |  28 +-
 .../src/createReactNoop.js                    |  29 +-
 .../src/ReactTestRendererAct.js               |  28 +-
 4 files changed, 237 insertions(+), 156 deletions(-)

diff --git a/fixtures/dom/src/index.test.js b/fixtures/dom/src/index.test.js
index 365afe5752ab4..e562fd61c845b 100644
--- a/fixtures/dom/src/index.test.js
+++ b/fixtures/dom/src/index.test.js
@@ -7,15 +7,14 @@
  * @emails react-core
  */
 
-import React from 'react';
-import ReactDOM from 'react-dom';
-import ReactART from 'react-art';
-import ARTSVGMode from 'art/modes/svg';
-import ARTCurrentMode from 'art/modes/current';
-import TestUtils from 'react-dom/test-utils';
-import TestRenderer from 'react-test-renderer';
-
-ARTCurrentMode.setCurrent(ARTSVGMode);
+let React;
+let ReactDOM;
+let ReactART;
+let ARTSVGMode;
+let ARTCurrentMode;
+let TestUtils;
+let TestRenderer;
+let ARTTest;
 
 global.__DEV__ = process.env.NODE_ENV !== 'production';
 
@@ -25,153 +24,210 @@ function App(props) {
   return 'hello world';
 }
 
-it("doesn't warn when you use the right act + renderer: dom", () => {
-  TestUtils.act(() => {
-    TestUtils.renderIntoDocument(<App />);
-  });
+describe('legacy mode', () => {
+  runTests();
 });
 
-it("doesn't warn when you use the right act + renderer: test", () => {
-  TestRenderer.act(() => {
-    TestRenderer.create(<App />);
+describe('mocked scheduler', () => {
+  beforeEach(() => {
+    jest.mock('scheduler', () =>
+      require.requireActual('scheduler/unstable_mock')
+    );
   });
+  afterEach(() => {
+    jest.unmock('scheduler');
+  });
+  runTests();
 });
 
-it('resets correctly across renderers', () => {
-  function Effecty() {
-    React.useEffect(() => {}, []);
-    return null;
-  }
-  TestUtils.act(() => {
-    TestRenderer.act(() => {});
-    expect(() => {
-      TestRenderer.create(<Effecty />);
-    }).toWarnDev(["It looks like you're using the wrong act()"], {
-      withoutStack: true,
+function runTests() {
+  beforeEach(() => {
+    jest.resetModules();
+    React = require('react');
+    ReactDOM = require('react-dom');
+    ReactART = require('react-art');
+    ARTSVGMode = require('art/modes/svg');
+    ARTCurrentMode = require('art/modes/current');
+    TestUtils = require('react-dom/test-utils');
+    TestRenderer = require('react-test-renderer');
+
+    ARTCurrentMode.setCurrent(ARTSVGMode);
+
+    ARTTest = function ARTTest(props) {
+      return (
+        <ReactART.Surface width={150} height={200}>
+          <ReactART.Group>
+            <ReactART.Shape
+              d="M0,0l50,0l0,50l-50,0z"
+              fill={new ReactART.LinearGradient(['black', 'white'])}
+              key="a"
+              width={50}
+              height={50}
+              x={50}
+              y={50}
+              opacity={0.1}
+            />
+            <ReactART.Shape
+              fill="#3C5A99"
+              key="b"
+              scale={0.5}
+              x={50}
+              y={50}
+              title="This is an F"
+              cursor="pointer">
+              M64.564,38.583H54l0.008-5.834c0-3.035,0.293-4.666,4.657-4.666
+              h5.833V16.429h-9.33c-11.213,0-15.159,5.654-15.159,15.16v6.994
+              h-6.99v11.652h6.99v33.815H54V50.235h9.331L64.564,38.583z
+            </ReactART.Shape>
+          </ReactART.Group>
+        </ReactART.Surface>
+      );
+    };
+  });
+  it("doesn't warn when you use the right act + renderer: dom", () => {
+    TestUtils.act(() => {
+      TestUtils.renderIntoDocument(<App />);
     });
   });
-});
 
-it('warns when using createRoot() + .render', () => {
-  const root = ReactDOM.unstable_createRoot(document.createElement('div'));
-  expect(() => {
+  it("doesn't warn when you use the right act + renderer: test", () => {
     TestRenderer.act(() => {
-      root.render(<App />);
+      TestRenderer.create(<App />);
     });
-  }).toWarnDev(["It looks like you're using the wrong act()"], {
-    withoutStack: true,
   });
-});
 
-it('warns when using the wrong act version - test + dom: render', () => {
-  expect(() => {
-    TestRenderer.act(() => {
-      TestUtils.renderIntoDocument(<App />);
+  it('resets correctly across renderers', () => {
+    function Effecty() {
+      React.useEffect(() => {}, []);
+      return null;
+    }
+    TestUtils.act(() => {
+      TestRenderer.act(() => {});
+      expect(() => {
+        TestRenderer.create(<Effecty />);
+      }).toWarnDev(["It looks like you're using the wrong act()"], {
+        withoutStack: true,
+      });
     });
-  }).toWarnDev(["It looks like you're using the wrong act()"], {
-    withoutStack: true,
   });
-});
 
-it('warns when using the wrong act version - test + dom: updates', () => {
-  let setCtr;
-  function Counter(props) {
-    const [ctr, _setCtr] = React.useState(0);
-    setCtr = _setCtr;
-    return ctr;
-  }
-  TestUtils.renderIntoDocument(<Counter />);
-  expect(() => {
-    TestRenderer.act(() => {
-      setCtr(1);
+  it('warns when using createRoot() + .render', () => {
+    const root = ReactDOM.unstable_createRoot(document.createElement('div'));
+    expect(() => {
+      TestRenderer.act(() => {
+        root.render(<App />);
+      });
+    }).toWarnDev(["It looks like you're using the wrong act()"], {
+      withoutStack: true,
     });
-  }).toWarnDev(["It looks like you're using the wrong act()"]);
-});
+  });
 
-it('warns when using the wrong act version - dom + test: .create()', () => {
-  expect(() => {
-    TestUtils.act(() => {
-      TestRenderer.create(<App />);
+  it('warns when using the wrong act version - test + dom: render', () => {
+    expect(() => {
+      TestRenderer.act(() => {
+        TestUtils.renderIntoDocument(<App />);
+      });
+    }).toWarnDev(["It looks like you're using the wrong act()"], {
+      withoutStack: true,
     });
-  }).toWarnDev(["It looks like you're using the wrong act()"], {
-    withoutStack: true,
   });
-});
 
-it('warns when using the wrong act version - dom + test: .update()', () => {
-  const root = TestRenderer.create(<App key="one" />);
-  expect(() => {
-    TestUtils.act(() => {
-      root.update(<App key="two" />);
+  it('warns when using the wrong act version - test + dom: updates', () => {
+    let setCtr;
+    function Counter(props) {
+      const [ctr, _setCtr] = React.useState(0);
+      setCtr = _setCtr;
+      return ctr;
+    }
+    TestUtils.renderIntoDocument(<Counter />);
+    expect(() => {
+      TestRenderer.act(() => {
+        setCtr(1);
+      });
+    }).toWarnDev(["It looks like you're using the wrong act()"]);
+  });
+
+  it('warns when using the wrong act version - dom + test: .create()', () => {
+    expect(() => {
+      TestUtils.act(() => {
+        TestRenderer.create(<App />);
+      });
+    }).toWarnDev(["It looks like you're using the wrong act()"], {
+      withoutStack: true,
     });
-  }).toWarnDev(["It looks like you're using the wrong act()"], {
-    withoutStack: true,
   });
-});
 
-it('warns when using the wrong act version - dom + test: updates', () => {
-  let setCtr;
-  function Counter(props) {
-    const [ctr, _setCtr] = React.useState(0);
-    setCtr = _setCtr;
-    return ctr;
-  }
-  const root = TestRenderer.create(<Counter />);
-  expect(() => {
-    TestUtils.act(() => {
-      setCtr(1);
+  it('warns when using the wrong act version - dom + test: .update()', () => {
+    const root = TestRenderer.create(<App key="one" />);
+    expect(() => {
+      TestUtils.act(() => {
+        root.update(<App key="two" />);
+      });
+    }).toWarnDev(["It looks like you're using the wrong act()"], {
+      withoutStack: true,
     });
-  }).toWarnDev(["It looks like you're using the wrong act()"]);
-});
+  });
 
-const {Surface, Group, Shape} = ReactART;
-function ARTTest(props) {
-  return (
-    <Surface width={150} height={200}>
-      <Group>
-        <Shape
-          d="M0,0l50,0l0,50l-50,0z"
-          fill={new ReactART.LinearGradient(['black', 'white'])}
-          key="a"
-          width={50}
-          height={50}
-          x={50}
-          y={50}
-          opacity={0.1}
-        />
-        <Shape
-          fill="#3C5A99"
-          key="b"
-          scale={0.5}
-          x={50}
-          y={50}
-          title="This is an F"
-          cursor="pointer">
-          M64.564,38.583H54l0.008-5.834c0-3.035,0.293-4.666,4.657-4.666
-          h5.833V16.429h-9.33c-11.213,0-15.159,5.654-15.159,15.16v6.994
-          h-6.99v11.652h6.99v33.815H54V50.235h9.331L64.564,38.583z
-        </Shape>
-      </Group>
-    </Surface>
-  );
-}
+  it('warns when using the wrong act version - dom + test: updates', () => {
+    let setCtr;
+    function Counter(props) {
+      const [ctr, _setCtr] = React.useState(0);
+      setCtr = _setCtr;
+      return ctr;
+    }
+    const root = TestRenderer.create(<Counter />);
+    expect(() => {
+      TestUtils.act(() => {
+        setCtr(1);
+      });
+    }).toWarnDev(["It looks like you're using the wrong act()"]);
+  });
+
+  it('does not warn when nesting react-act inside react-dom', () => {
+    TestUtils.act(() => {
+      TestUtils.renderIntoDocument(<ARTTest />);
+    });
+  });
 
-it('does not warn when nesting react-act inside react-dom', () => {
-  TestUtils.act(() => {
-    TestUtils.renderIntoDocument(<ARTTest />);
+  it('does not warn when nesting react-act inside react-test-renderer', () => {
+    TestRenderer.act(() => {
+      TestRenderer.create(<ARTTest />);
+    });
   });
-});
 
-it('does not warn when nesting react-act inside react-test-renderer', () => {
-  TestRenderer.act(() => {
-    TestRenderer.create(<ARTTest />);
+  it("doesn't warn if you use nested acts from different renderers", () => {
+    TestRenderer.act(() => {
+      TestUtils.act(() => {
+        TestRenderer.create(<App />);
+      });
+    });
   });
-});
 
-it("doesn't warn if you use nested acts from different renderers", () => {
-  TestRenderer.act(() => {
+  it('flushes work only outside the outermost act(), even when nested from different renderers', () => {
+    const log = [];
+    function Effecty() {
+      React.useEffect(() => {
+        log.push('called');
+      }, []);
+      return null;
+    }
+    // in legacy mode, this tests whether an act only flushes its own effects
+    // with a mocked scheduler, this tests whether it flushes all work only on the outermost act
+    TestRenderer.act(() => {
+      TestUtils.act(() => {
+        TestRenderer.create(<Effecty />);
+      });
+      expect(log).toEqual([]);
+    });
+    expect(log).toEqual(['called']);
+
+    log.splice(0);
+    // for doublechecking, we flip it inside out, and assert on the outermost
     TestUtils.act(() => {
-      TestRenderer.create(<App />);
+      TestRenderer.act(() => {
+        TestRenderer.create(<Effecty />);
+      });
     });
+    expect(log).toEqual(['called']);
   });
-});
+}
diff --git a/packages/react-dom/src/test-utils/ReactTestUtilsAct.js b/packages/react-dom/src/test-utils/ReactTestUtilsAct.js
index b0775cc3bce05..bfeb849d7dae0 100644
--- a/packages/react-dom/src/test-utils/ReactTestUtilsAct.js
+++ b/packages/react-dom/src/test-utils/ReactTestUtilsAct.js
@@ -44,6 +44,8 @@ const {IsSomeRendererActing} = ReactSharedInternals;
 // ReactTestUtilsAct.js, ReactTestRendererAct.js, createReactNoop.js
 
 let hasWarnedAboutMissingMockScheduler = false;
+const isSchedulerMocked =
+  typeof Scheduler.unstable_flushAllWithoutAsserting === 'function';
 const flushWork =
   Scheduler.unstable_flushAllWithoutAsserting ||
   function() {
@@ -89,18 +91,17 @@ function act(callback: () => Thenable) {
   let previousIsSomeRendererActing;
   let previousIsThisRendererActing;
   actingUpdatesScopeDepth++;
-  if (__DEV__) {
-    previousIsSomeRendererActing = IsSomeRendererActing.current;
-    previousIsThisRendererActing = IsThisRendererActing.current;
-    IsSomeRendererActing.current = true;
-    IsThisRendererActing.current = true;
-  }
+
+  previousIsSomeRendererActing = IsSomeRendererActing.current;
+  previousIsThisRendererActing = IsThisRendererActing.current;
+  IsSomeRendererActing.current = true;
+  IsThisRendererActing.current = true;
 
   function onDone() {
     actingUpdatesScopeDepth--;
+    IsSomeRendererActing.current = previousIsSomeRendererActing;
+    IsThisRendererActing.current = previousIsThisRendererActing;
     if (__DEV__) {
-      IsSomeRendererActing.current = previousIsSomeRendererActing;
-      IsThisRendererActing.current = previousIsThisRendererActing;
       if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) {
         // if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned
         warningWithoutStack(
@@ -155,7 +156,11 @@ function act(callback: () => Thenable) {
         called = true;
         result.then(
           () => {
-            if (actingUpdatesScopeDepth > 1) {
+            if (
+              actingUpdatesScopeDepth > 1 ||
+              (isSchedulerMocked === true &&
+                previousIsSomeRendererActing === true)
+            ) {
               onDone();
               resolve();
               return;
@@ -190,7 +195,10 @@ function act(callback: () => Thenable) {
 
     // flush effects until none remain, and cleanup
     try {
-      if (actingUpdatesScopeDepth === 1) {
+      if (
+        actingUpdatesScopeDepth === 1 &&
+        (isSchedulerMocked === false || previousIsSomeRendererActing === false)
+      ) {
         // we're about to exit the act() scope,
         // now's the time to flush effects
         flushWork();
diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js
index 579bf0dcda1d5..1f868e4fcda17 100644
--- a/packages/react-noop-renderer/src/createReactNoop.js
+++ b/packages/react-noop-renderer/src/createReactNoop.js
@@ -600,6 +600,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
   // ReactTestUtilsAct.js, ReactTestRendererAct.js, createReactNoop.js
 
   let hasWarnedAboutMissingMockScheduler = false;
+  const isSchedulerMocked =
+    typeof Scheduler.unstable_flushAllWithoutAsserting === 'function';
   const flushWork =
     Scheduler.unstable_flushAllWithoutAsserting ||
     function() {
@@ -645,18 +647,17 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
     let previousIsSomeRendererActing;
     let previousIsThisRendererActing;
     actingUpdatesScopeDepth++;
-    if (__DEV__) {
-      previousIsSomeRendererActing = IsSomeRendererActing.current;
-      previousIsThisRendererActing = IsThisRendererActing.current;
-      IsSomeRendererActing.current = true;
-      IsThisRendererActing.current = true;
-    }
+
+    previousIsSomeRendererActing = IsSomeRendererActing.current;
+    previousIsThisRendererActing = IsThisRendererActing.current;
+    IsSomeRendererActing.current = true;
+    IsThisRendererActing.current = true;
 
     function onDone() {
       actingUpdatesScopeDepth--;
+      IsSomeRendererActing.current = previousIsSomeRendererActing;
+      IsThisRendererActing.current = previousIsThisRendererActing;
       if (__DEV__) {
-        IsSomeRendererActing.current = previousIsSomeRendererActing;
-        IsThisRendererActing.current = previousIsThisRendererActing;
         if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) {
           // if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned
           warningWithoutStack(
@@ -711,7 +712,11 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
           called = true;
           result.then(
             () => {
-              if (actingUpdatesScopeDepth > 1) {
+              if (
+                actingUpdatesScopeDepth > 1 ||
+                (isSchedulerMocked === true &&
+                  previousIsSomeRendererActing === true)
+              ) {
                 onDone();
                 resolve();
                 return;
@@ -746,7 +751,11 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
 
       // flush effects until none remain, and cleanup
       try {
-        if (actingUpdatesScopeDepth === 1) {
+        if (
+          actingUpdatesScopeDepth === 1 &&
+          (isSchedulerMocked === false ||
+            previousIsSomeRendererActing === false)
+        ) {
           // we're about to exit the act() scope,
           // now's the time to flush effects
           flushWork();
diff --git a/packages/react-test-renderer/src/ReactTestRendererAct.js b/packages/react-test-renderer/src/ReactTestRendererAct.js
index de6a76a06c155..4b6dd2864e226 100644
--- a/packages/react-test-renderer/src/ReactTestRendererAct.js
+++ b/packages/react-test-renderer/src/ReactTestRendererAct.js
@@ -25,6 +25,8 @@ const {IsSomeRendererActing} = ReactSharedInternals;
 // ReactTestUtilsAct.js, ReactTestRendererAct.js, createReactNoop.js
 
 let hasWarnedAboutMissingMockScheduler = false;
+const isSchedulerMocked =
+  typeof Scheduler.unstable_flushAllWithoutAsserting === 'function';
 const flushWork =
   Scheduler.unstable_flushAllWithoutAsserting ||
   function() {
@@ -70,18 +72,17 @@ function act(callback: () => Thenable) {
   let previousIsSomeRendererActing;
   let previousIsThisRendererActing;
   actingUpdatesScopeDepth++;
-  if (__DEV__) {
-    previousIsSomeRendererActing = IsSomeRendererActing.current;
-    previousIsThisRendererActing = IsThisRendererActing.current;
-    IsSomeRendererActing.current = true;
-    IsThisRendererActing.current = true;
-  }
+
+  previousIsSomeRendererActing = IsSomeRendererActing.current;
+  previousIsThisRendererActing = IsThisRendererActing.current;
+  IsSomeRendererActing.current = true;
+  IsThisRendererActing.current = true;
 
   function onDone() {
     actingUpdatesScopeDepth--;
+    IsSomeRendererActing.current = previousIsSomeRendererActing;
+    IsThisRendererActing.current = previousIsThisRendererActing;
     if (__DEV__) {
-      IsSomeRendererActing.current = previousIsSomeRendererActing;
-      IsThisRendererActing.current = previousIsThisRendererActing;
       if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) {
         // if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned
         warningWithoutStack(
@@ -136,7 +137,11 @@ function act(callback: () => Thenable) {
         called = true;
         result.then(
           () => {
-            if (actingUpdatesScopeDepth > 1) {
+            if (
+              actingUpdatesScopeDepth > 1 ||
+              (isSchedulerMocked === true &&
+                previousIsSomeRendererActing === true)
+            ) {
               onDone();
               resolve();
               return;
@@ -171,7 +176,10 @@ function act(callback: () => Thenable) {
 
     // flush effects until none remain, and cleanup
     try {
-      if (actingUpdatesScopeDepth === 1) {
+      if (
+        actingUpdatesScopeDepth === 1 &&
+        (isSchedulerMocked === false || previousIsSomeRendererActing === false)
+      ) {
         // we're about to exit the act() scope,
         // now's the time to flush effects
         flushWork();