From d412eec8396e9b800b2e75a2585111ffa09b4978 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Fri, 26 Jul 2019 17:48:42 +0100 Subject: [PATCH] [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. --- .../src/__tests__/ReactTestUtilsAct-test.js | 10 ++ ...ReactTestUtilsActUnmockedScheduler-test.js | 166 ++++++++++++++++++ .../src/test-utils/ReactTestUtilsAct.js | 8 +- .../src/createReactNoop.js | 8 +- .../src/ReactTestRendererAct.js | 8 +- 5 files changed, 197 insertions(+), 3 deletions(-) create mode 100644 packages/react-dom/src/__tests__/ReactTestUtilsActUnmockedScheduler-test.js diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index 827b3aa2f1855..c369eb97ed4c7 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/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; diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsActUnmockedScheduler-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsActUnmockedScheduler-test.js new file mode 100644 index 0000000000000..baa4f63e4ba75 --- /dev/null +++ b/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(, 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 ( + + ); + } + + act(() => { + render(, 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(, 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(, 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(, container); + }); + // all 5 ticks present and accounted for + expect(clearYields()).toEqual([0, 1, 2, 3, 4]); + expect(container.innerHTML).toBe('5'); +}); diff --git a/packages/react-dom/src/test-utils/ReactTestUtilsAct.js b/packages/react-dom/src/test-utils/ReactTestUtilsAct.js index bfeb849d7dae0..a0ccce2c9c9a4 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtilsAct.js +++ b/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) { diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 1f868e4fcda17..451c3c4dccc88 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/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) { diff --git a/packages/react-test-renderer/src/ReactTestRendererAct.js b/packages/react-test-renderer/src/ReactTestRendererAct.js index 4b6dd2864e226..83408301a6994 100644 --- a/packages/react-test-renderer/src/ReactTestRendererAct.js +++ b/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) {