From 0d6e95ad971643803e6fc5f2d3124568d80931fe Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Fri, 21 Aug 2020 12:14:48 +0200 Subject: [PATCH 1/3] Add more tests around cleanup with fake timers --- src/__tests__/cleanup.js | 83 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 1 deletion(-) diff --git a/src/__tests__/cleanup.js b/src/__tests__/cleanup.js index c0f1676d..feb30b26 100644 --- a/src/__tests__/cleanup.js +++ b/src/__tests__/cleanup.js @@ -1,5 +1,5 @@ import React from 'react' -import {render, cleanup} from '../' +import {render, cleanup, act} from '../' test('cleans up the document', async () => { const spy = jest.fn() @@ -40,3 +40,84 @@ test('cleanup runs effect cleanup functions', async () => { await cleanup() expect(spy).toHaveBeenCalledTimes(1) }) + +describe('fake timers and missing act warnings', () => { + beforeEach(() => { + jest.resetAllMocks() + jest.spyOn(console, 'error').mockImplementation(() => { + // assert messages aexplicitly + }) + jest.useFakeTimers() + }) + + afterEach(() => { + jest.useRealTimers() + }) + + test('cleanup does not flush immediates', async () => { + const microTaskSpy = jest.fn() + function Test() { + const counter = 1 + const [, setDeferredCounter] = React.useState(null) + React.useEffect(() => { + let cancelled = false + setImmediate(() => { + microTaskSpy() + if (!cancelled) { + setDeferredCounter(counter) + } + }) + + return () => { + cancelled = true + } + }, [counter]) + + return null + } + render() + + await cleanup() + + expect(microTaskSpy).toHaveBeenCalledTimes(0) + // console.error is mocked + // eslint-disable-next-line no-console + expect(console.error).toHaveBeenCalledTimes(0) + }) + + test('cleanup does not swallow missing act warnings', async () => { + const deferredStateUpdateSpy = jest.fn() + function Test() { + const counter = 1 + const [, setDeferredCounter] = React.useState(null) + React.useEffect(() => { + let cancelled = false + setImmediate(() => { + deferredStateUpdateSpy() + if (!cancelled) { + setDeferredCounter(counter) + } + }) + + return () => { + cancelled = true + } + }, [counter]) + + return null + } + render() + + jest.runAllImmediates() + await cleanup() + + expect(deferredStateUpdateSpy).toHaveBeenCalledTimes(1) + // console.error is mocked + // eslint-disable-next-line no-console + expect(console.error).toHaveBeenCalledTimes(1) + // eslint-disable-next-line no-console + expect(console.error.mock.calls[0][0]).toMatch( + 'a test was not wrapped in act(...)', + ) + }) +}) From e8f48aee7d546efe7bef79a85eb72ccde3ade8ed Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Fri, 21 Aug 2020 12:26:08 +0200 Subject: [PATCH 2/3] feat: Use React scheduler to flush instead of custom implementation --- src/__tests__/cleanup.js | 22 ++++++------- src/flush-microtasks.js | 67 ---------------------------------------- src/pure.js | 10 +++--- 3 files changed, 15 insertions(+), 84 deletions(-) delete mode 100644 src/flush-microtasks.js diff --git a/src/__tests__/cleanup.js b/src/__tests__/cleanup.js index feb30b26..40c3cbaa 100644 --- a/src/__tests__/cleanup.js +++ b/src/__tests__/cleanup.js @@ -1,7 +1,7 @@ import React from 'react' -import {render, cleanup, act} from '../' +import {render, cleanup} from '../' -test('cleans up the document', async () => { +test('cleans up the document', () => { const spy = jest.fn() const divId = 'my-div' @@ -17,17 +17,17 @@ test('cleans up the document', async () => { } render() - await cleanup() + cleanup() expect(document.body).toBeEmptyDOMElement() expect(spy).toHaveBeenCalledTimes(1) }) -test('cleanup does not error when an element is not a child', async () => { +test('cleanup does not error when an element is not a child', () => { render(
, {container: document.createElement('div')}) - await cleanup() + cleanup() }) -test('cleanup runs effect cleanup functions', async () => { +test('cleanup runs effect cleanup functions', () => { const spy = jest.fn() const Test = () => { @@ -37,7 +37,7 @@ test('cleanup runs effect cleanup functions', async () => { } render() - await cleanup() + cleanup() expect(spy).toHaveBeenCalledTimes(1) }) @@ -54,7 +54,7 @@ describe('fake timers and missing act warnings', () => { jest.useRealTimers() }) - test('cleanup does not flush immediates', async () => { + test('cleanup does not flush immediates', () => { const microTaskSpy = jest.fn() function Test() { const counter = 1 @@ -77,7 +77,7 @@ describe('fake timers and missing act warnings', () => { } render() - await cleanup() + cleanup() expect(microTaskSpy).toHaveBeenCalledTimes(0) // console.error is mocked @@ -85,7 +85,7 @@ describe('fake timers and missing act warnings', () => { expect(console.error).toHaveBeenCalledTimes(0) }) - test('cleanup does not swallow missing act warnings', async () => { + test('cleanup does not swallow missing act warnings', () => { const deferredStateUpdateSpy = jest.fn() function Test() { const counter = 1 @@ -109,7 +109,7 @@ describe('fake timers and missing act warnings', () => { render() jest.runAllImmediates() - await cleanup() + cleanup() expect(deferredStateUpdateSpy).toHaveBeenCalledTimes(1) // console.error is mocked diff --git a/src/flush-microtasks.js b/src/flush-microtasks.js deleted file mode 100644 index e1d8fe6f..00000000 --- a/src/flush-microtasks.js +++ /dev/null @@ -1,67 +0,0 @@ -/* istanbul ignore file */ -// the part of this file that we need tested is definitely being run -// and the part that is not cannot easily have useful tests written -// anyway. So we're just going to ignore coverage for this file -/** - * copied and modified from React's enqueueTask.js - */ - -function getIsUsingFakeTimers() { - return ( - typeof jest !== 'undefined' && - typeof setTimeout !== 'undefined' && - (setTimeout.hasOwnProperty('_isMockFunction') || - setTimeout.hasOwnProperty('clock')) - ) -} - -let didWarnAboutMessageChannel = false -let enqueueTask - -try { - // read require off the module object to get around the bundlers. - // we don't want them to detect a require and bundle a Node polyfill. - const requireString = `require${Math.random()}`.slice(0, 7) - const nodeRequire = module && module[requireString] - // assuming we're in node, let's try to get node's - // version of setImmediate, bypassing fake timers if any. - enqueueTask = nodeRequire.call(module, 'timers').setImmediate -} catch (_err) { - // we're in a browser - // we can't use regular timers because they may still be faked - // so we try MessageChannel+postMessage instead - enqueueTask = callback => { - const supportsMessageChannel = typeof MessageChannel === 'function' - if (supportsMessageChannel) { - const channel = new MessageChannel() - channel.port1.onmessage = callback - channel.port2.postMessage(undefined) - } else if (didWarnAboutMessageChannel === false) { - didWarnAboutMessageChannel = true - - // eslint-disable-next-line no-console - console.error( - 'This browser does not have a MessageChannel implementation, ' + - 'so enqueuing tasks via await act(async () => ...) will fail. ' + - 'Please file an issue at https://github.com/facebook/react/issues ' + - 'if you encounter this warning.', - ) - } - } -} - -export default function flushMicroTasks() { - return { - then(resolve) { - if (getIsUsingFakeTimers()) { - // without this, a test using fake timers would never get microtasks - // actually flushed. I spent several days on this... Really hard to - // reproduce the problem, so there's no test for it. But it works! - jest.advanceTimersByTime(0) - resolve() - } else { - enqueueTask(resolve) - } - }, - } -} diff --git a/src/pure.js b/src/pure.js index f2f3438f..8a062038 100644 --- a/src/pure.js +++ b/src/pure.js @@ -7,7 +7,6 @@ import { } from '@testing-library/dom' import act, {asyncAct} from './act-compat' import {fireEvent} from './fire-event' -import flush from './flush-microtasks' configureDTL({ asyncWrapper: async cb => { @@ -100,17 +99,16 @@ function render( } } -async function cleanup() { +function cleanup() { mountedContainers.forEach(cleanupAtContainer) - // flush microtask queue after unmounting in case - // unmount sequence generates new microtasks - await flush() } // maybe one day we'll expose this (perhaps even as a utility returned by render). // but let's wait until someone asks for it. function cleanupAtContainer(container) { - ReactDOM.unmountComponentAtNode(container) + act(() => { + ReactDOM.unmountComponentAtNode(container) + }) if (container.parentNode === document.body) { document.body.removeChild(container) } From 4b62cc312717d42279b5d43f4b7ff9a9ba90555d Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Mon, 24 Aug 2020 19:40:59 +0200 Subject: [PATCH 3/3] Update src/__tests__/cleanup.js --- src/__tests__/cleanup.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/__tests__/cleanup.js b/src/__tests__/cleanup.js index 40c3cbaa..4b67814a 100644 --- a/src/__tests__/cleanup.js +++ b/src/__tests__/cleanup.js @@ -45,7 +45,7 @@ describe('fake timers and missing act warnings', () => { beforeEach(() => { jest.resetAllMocks() jest.spyOn(console, 'error').mockImplementation(() => { - // assert messages aexplicitly + // assert messages explicitly }) jest.useFakeTimers() })