Skip to content

Commit

Permalink
Decouple public, internal act implementation (#19745)
Browse files Browse the repository at this point in the history
In the next major release, we intend to drop support for using the `act`
testing helper in production. (It already fires a warning.) The
rationale is that, in order for `act` to work, you must either mock the
testing environment or add extra logic at runtime. Mocking the testing
environment isn't ideal because it requires extra set up for the user.
Extra logic at runtime is fine only in development mode — we don't want
to slow down the production builds.

Since most people only run their tests in development mode, dropping
support for production should be fine; if there's demand, we can add it
back later using a special testing build that is identical to the
production build except for the additional testing logic.

One blocker for removing production support is that we currently use
`act` to test React itself. We must test React in both development and
production modes.

So, the solution is to fork `act` into separate public and
internal implementations:

- *public implementation of `act`* – exposed to users, only works in
  development mode, uses special runtime logic, does not support partial
  rendering
- *internal implementation of `act`* – private, works in both
  development and productionm modes, only used by the React Core test
  suite, uses no special runtime logic, supports partial rendering (i.e.
  `toFlushAndYieldThrough`)

The internal implementation should mostly match the public
implementation's behavior, but since it's a private API, it doesn't have
to match exactly. It works by mocking the test environment: it uses a
mock build of Scheduler to flush rendering tasks, and Jest's mock timers
to flush Suspense placeholders.

---

In this first commit, I've added the internal forks of `act` and
migrated our tests to use them. The public `act` implementation is
unaffected for now; I will leave refactoring/clean-up for a later step.
  • Loading branch information
acdlite authored Sep 8, 2020
1 parent d38ec17 commit d17086c
Show file tree
Hide file tree
Showing 36 changed files with 465 additions and 298 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe('ReactHooksInspectionIntegration', () => {
React = require('react');
ReactTestRenderer = require('react-test-renderer');
Scheduler = require('scheduler');
act = ReactTestRenderer.act;
act = ReactTestRenderer.unstable_concurrentAct;
ReactDebugTools = require('react-debug-tools');
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ describe('InspectedElementContext', () => {
let TestUtils;
let TreeContextController;

let TestUtilsAct;
let TestRendererAct;

beforeEach(() => {
utils = require('./utils');
utils.beforeEachProfiling();
Expand All @@ -47,7 +50,9 @@ describe('InspectedElementContext', () => {
ReactDOM = require('react-dom');
PropTypes = require('prop-types');
TestUtils = require('react-dom/test-utils');
TestUtilsAct = TestUtils.unstable_concurrentAct;
TestRenderer = utils.requireTestRenderer();
TestRendererAct = TestUtils.unstable_concurrentAct;

BridgeContext = require('react-devtools-shared/src/devtools/views/context')
.BridgeContext;
Expand Down Expand Up @@ -999,8 +1004,8 @@ describe('InspectedElementContext', () => {
expect(inspectedElement).toMatchSnapshot('1: Initially inspect element');

inspectedElement = null;
TestUtils.act(() => {
TestRenderer.act(() => {
TestUtilsAct(() => {
TestRendererAct(() => {
getInspectedElementPath(id, ['props', 'nestedObject', 'a']);
jest.runOnlyPendingTimers();
});
Expand All @@ -1009,8 +1014,8 @@ describe('InspectedElementContext', () => {
expect(inspectedElement).toMatchSnapshot('2: Inspect props.nestedObject.a');

inspectedElement = null;
TestUtils.act(() => {
TestRenderer.act(() => {
TestUtilsAct(() => {
TestRendererAct(() => {
getInspectedElementPath(id, ['props', 'nestedObject', 'a', 'b', 'c']);
jest.runOnlyPendingTimers();
});
Expand All @@ -1021,8 +1026,8 @@ describe('InspectedElementContext', () => {
);

inspectedElement = null;
TestUtils.act(() => {
TestRenderer.act(() => {
TestUtilsAct(() => {
TestRendererAct(() => {
getInspectedElementPath(id, [
'props',
'nestedObject',
Expand All @@ -1041,8 +1046,8 @@ describe('InspectedElementContext', () => {
);

inspectedElement = null;
TestUtils.act(() => {
TestRenderer.act(() => {
TestUtilsAct(() => {
TestRendererAct(() => {
getInspectedElementPath(id, ['hooks', 0, 'value']);
jest.runOnlyPendingTimers();
});
Expand All @@ -1051,8 +1056,8 @@ describe('InspectedElementContext', () => {
expect(inspectedElement).toMatchSnapshot('5: Inspect hooks.0.value');

inspectedElement = null;
TestUtils.act(() => {
TestRenderer.act(() => {
TestUtilsAct(() => {
TestRendererAct(() => {
getInspectedElementPath(id, ['hooks', 0, 'value', 'foo', 'bar']);
jest.runOnlyPendingTimers();
});
Expand Down Expand Up @@ -1108,8 +1113,8 @@ describe('InspectedElementContext', () => {
expect(inspectedElement).toMatchSnapshot('1: Initially inspect element');

inspectedElement = null;
TestUtils.act(() => {
TestRenderer.act(() => {
TestUtilsAct(() => {
TestRendererAct(() => {
getInspectedElementPath(id, ['props', 'set_of_sets', 0]);
jest.runOnlyPendingTimers();
});
Expand Down Expand Up @@ -1179,23 +1184,23 @@ describe('InspectedElementContext', () => {
expect(inspectedElement).toMatchSnapshot('1: Initially inspect element');

inspectedElement = null;
TestRenderer.act(() => {
TestRendererAct(() => {
getInspectedElementPath(id, ['props', 'nestedObject', 'a']);
jest.runOnlyPendingTimers();
});
expect(inspectedElement).not.toBeNull();
expect(inspectedElement).toMatchSnapshot('2: Inspect props.nestedObject.a');

inspectedElement = null;
TestRenderer.act(() => {
TestRendererAct(() => {
getInspectedElementPath(id, ['props', 'nestedObject', 'c']);
jest.runOnlyPendingTimers();
});
expect(inspectedElement).not.toBeNull();
expect(inspectedElement).toMatchSnapshot('3: Inspect props.nestedObject.c');

TestRenderer.act(() => {
TestUtils.act(() => {
TestRendererAct(() => {
TestUtilsAct(() => {
ReactDOM.render(
<Example
nestedObject={{
Expand All @@ -1221,7 +1226,7 @@ describe('InspectedElementContext', () => {
});
});

TestRenderer.act(() => {
TestRendererAct(() => {
inspectedElement = null;
jest.advanceTimersByTime(1000);
});
Expand Down Expand Up @@ -1281,7 +1286,7 @@ describe('InspectedElementContext', () => {
expect(inspectedElement).not.toBeNull();
expect(inspectedElement).toMatchSnapshot('1: Initially inspect element');

TestUtils.act(() => {
TestUtilsAct(() => {
ReactDOM.render(
<Example
nestedObject={{
Expand All @@ -1300,8 +1305,8 @@ describe('InspectedElementContext', () => {

inspectedElement = null;

TestRenderer.act(() => {
TestUtils.act(() => {
TestRendererAct(() => {
TestUtilsAct(() => {
getInspectedElementPath(id, ['props', 'nestedObject', 'a']);
jest.runOnlyPendingTimers();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ describe('Store component filters', () => {
let utils;

const act = (callback: Function) => {
TestUtils.act(() => {
TestUtils.unstable_concurrentAct(() => {
callback();
});
jest.runAllTimers(); // Flush Bridge operations
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ describe('ReactDOMFiberAsync', () => {
container = document.createElement('div');
React = require('react');
ReactDOM = require('react-dom');
act = require('react-dom/test-utils').act;
act = require('react-dom/test-utils').unstable_concurrentAct;
Scheduler = require('scheduler');

document.body.appendChild(container);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ let React;
let ReactDOM;
let ReactDOMServer;
let ReactTestUtils;
let act;
let Scheduler;
let useState;
let useReducer;
Expand All @@ -43,6 +44,7 @@ function initModules() {
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');
Scheduler = require('scheduler');
act = ReactTestUtils.unstable_concurrentAct;
useState = React.useState;
useReducer = React.useReducer;
useEffect = React.useEffect;
Expand Down Expand Up @@ -1063,7 +1065,7 @@ describe('ReactDOMServerHooks', () => {
expect(domNode.children.length).toEqual(1);
expect(oldClientId).not.toBeNull();

await ReactTestUtils.act(async () => _setShowId(true));
await act(async () => _setShowId(true));

expect(domNode.children.length).toEqual(2);
expect(domNode.children[0].getAttribute('aria-labelledby')).toEqual(
Expand Down Expand Up @@ -1281,7 +1283,7 @@ describe('ReactDOMServerHooks', () => {
const oldServerId = container.children[0].children[0].getAttribute('id');
expect(oldServerId).not.toBeNull();

await ReactTestUtils.act(async () => {
await act(async () => {
_setShowDiv(true);
});
expect(container.children[0].children.length).toEqual(2);
Expand Down Expand Up @@ -1322,7 +1324,7 @@ describe('ReactDOMServerHooks', () => {
const oldServerId = container.children[0].children[0].getAttribute('id');
expect(oldServerId).not.toBeNull();

await ReactTestUtils.act(async () => {
await act(async () => {
_setShowDiv(true);
});
expect(container.children[0].children.length).toEqual(2);
Expand Down Expand Up @@ -1356,12 +1358,12 @@ describe('ReactDOMServerHooks', () => {
document.body.append(container);
container.innerHTML = ReactDOMServer.renderToString(<App />);
const root = ReactDOM.unstable_createRoot(container, {hydrate: true});
ReactTestUtils.act(() => {
act(() => {
root.render(<App />);
});
expect(Scheduler).toHaveYielded(['App', 'App']);
// The ID goes from not being used to being added to the page
ReactTestUtils.act(() => {
act(() => {
_setShow(true);
});
expect(Scheduler).toHaveYielded(['App', 'App']);
Expand Down Expand Up @@ -1391,7 +1393,7 @@ describe('ReactDOMServerHooks', () => {
ReactDOM.hydrate(<App />, container);
expect(Scheduler).toHaveYielded(['App', 'App']);
// The ID goes from not being used to being added to the page
ReactTestUtils.act(() => {
act(() => {
_setShow(true);
});
expect(Scheduler).toHaveYielded(['App']);
Expand All @@ -1418,12 +1420,12 @@ describe('ReactDOMServerHooks', () => {
document.body.append(container);
container.innerHTML = ReactDOMServer.renderToString(<App />);
const root = ReactDOM.unstable_createRoot(container, {hydrate: true});
ReactTestUtils.act(() => {
act(() => {
root.render(<App />);
});

// The ID goes from not being used to being added to the page
ReactTestUtils.act(() => {
act(() => {
ReactDOM.flushSync(() => {
_setShow(true);
});
Expand Down Expand Up @@ -1518,7 +1520,7 @@ describe('ReactDOMServerHooks', () => {
expect(child1Ref.current).toBe(null);
expect(Scheduler).toHaveYielded([]);

ReactTestUtils.act(() => {
act(() => {
_setShow(true);

// State update should trigger the ID to update, which changes the props
Expand Down Expand Up @@ -1603,7 +1605,7 @@ describe('ReactDOMServerHooks', () => {

suspend = true;
const root = ReactDOM.unstable_createRoot(container, {hydrate: true});
await ReactTestUtils.act(async () => {
await act(async () => {
root.render(<App />);
});
jest.runAllTimers();
Expand All @@ -1616,7 +1618,7 @@ describe('ReactDOMServerHooks', () => {
container.children[0].children[0].getAttribute('id'),
).not.toBeNull();

await ReactTestUtils.act(async () => {
await act(async () => {
suspend = false;
resolve();
await promise;
Expand Down Expand Up @@ -1703,7 +1705,7 @@ describe('ReactDOMServerHooks', () => {

suspend = false;
const root = ReactDOM.unstable_createRoot(container, {hydrate: true});
await ReactTestUtils.act(async () => {
await act(async () => {
root.render(<App />);
});
jest.runAllTimers();
Expand Down Expand Up @@ -1968,7 +1970,7 @@ describe('ReactDOMServerHooks', () => {
expect(Scheduler).toHaveYielded([]);
expect(Scheduler).toFlushAndYield([]);

ReactTestUtils.act(() => {
act(() => {
_setShow(false);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ describe('ReactDOMServerPartialHydration', () => {

React = require('react');
ReactDOM = require('react-dom');
act = require('react-dom/test-utils').act;
act = require('react-dom/test-utils').unstable_concurrentAct;
ReactDOMServer = require('react-dom/server');
Scheduler = require('scheduler');
Suspense = React.Suspense;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ let ReactDOMServer;
let ReactTestUtils;
let Scheduler;
let Suspense;
let act;

function dispatchMouseHoverEvent(to, from) {
if (!to) {
Expand Down Expand Up @@ -101,6 +102,7 @@ describe('ReactDOMServerSelectiveHydration', () => {
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');
act = ReactTestUtils.unstable_concurrentAct;
Scheduler = require('scheduler');
Suspense = React.Suspense;
});
Expand Down Expand Up @@ -880,7 +882,7 @@ describe('ReactDOMServerSelectiveHydration', () => {
const spanC = container.getElementsByTagName('span')[4];

const root = ReactDOM.createRoot(container, {hydrate: true});
ReactTestUtils.act(() => {
act(() => {
root.render(<App a="A" />);

// Hydrate the shell.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ let React;
let ReactDOM;
let ReactDOMServer;
let ReactTestUtils;
let act;

function initModules() {
// Reset warning cache.
Expand All @@ -24,6 +25,7 @@ function initModules() {
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');
act = ReactTestUtils.unstable_concurrentAct;

// Make them available to the helpers.
return {
Expand Down Expand Up @@ -124,7 +126,7 @@ describe('ReactDOMServerSuspense', () => {
expect(divB.tagName).toBe('DIV');
expect(divB.textContent).toBe('B');

ReactTestUtils.act(() => {
act(() => {
const root = ReactDOM.createBlockingRoot(parent, {hydrate: true});
root.render(example);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ describe('ReactDOMSuspensePlaceholder', () => {
ReactCache = require('react-cache');
ReactTestUtils = require('react-dom/test-utils');
Scheduler = require('scheduler');
act = ReactTestUtils.act;
act = ReactTestUtils.unstable_concurrentAct;
Suspense = React.Suspense;
container = document.createElement('div');
document.body.appendChild(container);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ describe('ReactErrorBoundaries', () => {
ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false;
ReactDOM = require('react-dom');
React = require('react');
act = require('react-dom/test-utils').act;
act = require('react-dom/test-utils').unstable_concurrentAct;
Scheduler = require('scheduler');

BrokenConstructor = class extends React.Component {
Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/__tests__/ReactUpdates-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ describe('ReactUpdates', () => {
React = require('react');
ReactDOM = require('react-dom');
ReactTestUtils = require('react-dom/test-utils');
act = ReactTestUtils.act;
act = ReactTestUtils.unstable_concurrentAct;
Scheduler = require('scheduler');
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,11 @@ module.exports = function(initModules) {
function asyncReactDOMRender(reactElement, domElement, forceHydrate) {
return new Promise(resolve => {
if (forceHydrate) {
ReactTestUtils.act(() => {
ReactTestUtils.unstable_concurrentAct(() => {
ReactDOM.hydrate(reactElement, domElement);
});
} else {
ReactTestUtils.act(() => {
ReactTestUtils.unstable_concurrentAct(() => {
ReactDOM.render(reactElement, domElement);
});
}
Expand Down
Loading

0 comments on commit d17086c

Please sign in to comment.