From dd96b3314cf0ec0ad50d38d9348b591068c492c5 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Sat, 4 May 2019 09:36:13 -0700 Subject: [PATCH] Cleaned up tests a bit. Profiling test uses mock timers now. --- .../__snapshots__/profiler-test.js.snap | 110 --------------- .../__snapshots__/profiling-test.js.snap | 61 ++++++++ src/__tests__/profiler-test.js | 131 ------------------ src/__tests__/profiling-test.js | 108 +++++++++++++++ src/__tests__/profilingSummarySerializer.js | 6 +- src/__tests__/store-test.js | 8 +- src/__tests__/storeOwners-test.js | 11 +- src/__tests__/storeSerializer.js | 6 +- src/__tests__/storeStressSync-test.js | 11 +- .../storeStressTestConcurrent-test.js | 12 +- src/__tests__/utils.js | 66 +++++++++ src/devtools/store.js | 55 +++++--- 12 files changed, 286 insertions(+), 299 deletions(-) delete mode 100644 src/__tests__/__snapshots__/profiler-test.js.snap create mode 100644 src/__tests__/__snapshots__/profiling-test.js.snap delete mode 100644 src/__tests__/profiler-test.js create mode 100644 src/__tests__/profiling-test.js create mode 100644 src/__tests__/utils.js diff --git a/src/__tests__/__snapshots__/profiler-test.js.snap b/src/__tests__/__snapshots__/profiler-test.js.snap deleted file mode 100644 index 3c68497cad..0000000000 --- a/src/__tests__/__snapshots__/profiler-test.js.snap +++ /dev/null @@ -1,110 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`Profiler should clean up after a root has been unmounted: 1: mount 1`] = ` -[root] - ▾ - - - -[root] - ▾ - - -`; - -exports[`Profiler should clean up after a root has been unmounted: 2: profiling started 1`] = ` -[root] - ▾ - - - -[root] - ▾ - - -`; - -exports[`Profiler should clean up after a root has been unmounted: 3: update 1`] = ` -[root] - ▾ - - - - -[root] - ▾ - -`; - -exports[`Profiler should clean up after a root has been unmounted: 4: unmount B 1`] = ` -[root] - ▾ - - - - -`; - -exports[`Profiler should clean up after a root has been unmounted: 5: unmount A 1`] = ``; - -exports[`Profiler should clean up after a root has been unmounted: 6: profiling stopped 1`] = ``; - -exports[`Profiler should collect basic profiling metrics: 1: mount 1`] = ` -[root] - ▾ - - -`; - -exports[`Profiler should collect basic profiling metrics: 2: add child 1`] = ` -[root] - ▾ - - - -`; - -exports[`Profiler should collect basic profiling metrics: 3: remove children 1`] = ` -[root] - ▾ - -`; - -exports[`Profiler should collect basic profiling metrics: 4: profiling stopped 1`] = ` -[root] - ▾ - -`; - -exports[`Profiler should collect basic profiling metrics: ProfilingSummary 1`] = ` -{ - "rootID": 1, - "commitDurations": [ - 0, - 1 - ], - "commitTimes": [ - 0, - 1 - ], - "initialTreeBaseDurations": [ - [ - 1, - 0 - ], - [ - 2, - 1 - ], - [ - 3, - 2 - ], - [ - 4, - 3 - ] - ], - "interactionCount": 0 -} -`; diff --git a/src/__tests__/__snapshots__/profiling-test.js.snap b/src/__tests__/__snapshots__/profiling-test.js.snap new file mode 100644 index 0000000000..a1e084986e --- /dev/null +++ b/src/__tests__/__snapshots__/profiling-test.js.snap @@ -0,0 +1,61 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`profiling profilingSummary should be collected for each commit: 1: mount 1`] = ` +[root] + ▾ + + +`; + +exports[`profiling profilingSummary should be collected for each commit: 2: add child 1`] = ` +[root] + ▾ + + + +`; + +exports[`profiling profilingSummary should be collected for each commit: 3: remove children 1`] = ` +[root] + ▾ + +`; + +exports[`profiling profilingSummary should be collected for each commit: 4: profiling stopped 1`] = ` +[root] + ▾ + +`; + +exports[`profiling profilingSummary should be collected for each commit: ProfilingSummary 1`] = ` +{ + "rootID": 1, + "commitDurations": [ + 16, + 12 + ], + "commitTimes": [ + 16, + 28 + ], + "initialTreeBaseDurations": [ + [ + 1, + 14 + ], + [ + 2, + 14 + ], + [ + 3, + 2 + ], + [ + 4, + 2 + ] + ], + "interactionCount": 0 +} +`; diff --git a/src/__tests__/profiler-test.js b/src/__tests__/profiler-test.js deleted file mode 100644 index 98c1172ae3..0000000000 --- a/src/__tests__/profiler-test.js +++ /dev/null @@ -1,131 +0,0 @@ -// @flow - -describe('Profiler', () => { - let React; - let ReactDOM; - let TestRenderer; - let TestUtils; - let agent; - let store; - - const act = (callback: Function) => { - TestUtils.act(() => { - callback(); - }); - jest.runAllTimers(); // Flush Bridge operations - }; - - const renderAndResolve = async (root, element) => { - // $FlowFixMe Flow doens't know about "await act()" yet - await TestUtils.act(async () => { - root.update(element); - - // Resolve pending suspense promises - jest.runAllTimers(); - }); - - // Re-render after resolved promises - jest.runAllTimers(); - }; - - beforeEach(() => { - agent = global.agent; - store = global.store; - store.collapseNodesByDefault = false; - - React = require('react'); - ReactDOM = require('react-dom'); - TestUtils = require('react-dom/test-utils'); - - // Hide the hook before requiring TestRenderer, so we don't end up with a loop. - const hook = global.__REACT_DEVTOOLS_GLOBAL_HOOK__; - delete global.__REACT_DEVTOOLS_GLOBAL_HOOK__; - TestRenderer = require('react-test-renderer'); - global.__REACT_DEVTOOLS_GLOBAL_HOOK__ = hook; - }); - - it('should collect basic profiling metrics', async done => { - const Parent = ({ count }) => - new Array(count).fill(true).map((_, index) => ); - const Child = () => { - jest.advanceTimersByTime(1); - return null; - }; - - const container = document.createElement('div'); - - act(() => ReactDOM.render(, container)); - expect(store).toMatchSnapshot('1: mount'); - - act(() => store.startProfiling()); - - act(() => ReactDOM.render(, container)); - expect(store).toMatchSnapshot('2: add child'); - - act(() => ReactDOM.render(, container)); - expect(store).toMatchSnapshot('3: remove children'); - - act(() => store.stopProfiling()); - expect(store).toMatchSnapshot('4: profiling stopped'); - - let profilingSummary; - function Suspender({ rendererID, rootID }) { - profilingSummary = store.profilingCache.ProfilingSummary.read({ - rendererID, - rootID, - }); - return null; - } - - // HACK There's only one renderer for this test - const rendererID = Object.keys(agent._rendererInterfaces)[0]; - const rootID = store.roots[0]; - - let root = TestRenderer.create(); - await renderAndResolve( - root, - - - - ); - - // HACK root.toTree() doesn't handle Suspense yet - // but Jest serializer wouldn't work with a JSON string - expect(profilingSummary).toMatchSnapshot('ProfilingSummary'); - - done(); - }); - - it('should clean up after a root has been unmounted', async () => { - const Parent = ({ count }) => - new Array(count).fill(true).map((_, index) => ); - const Child = () =>
Hi!
; - - const containerA = document.createElement('div'); - const containerB = document.createElement('div'); - - act(() => { - ReactDOM.render(, containerA); - ReactDOM.render(, containerB); - }); - expect(store).toMatchSnapshot('1: mount'); - - act(() => store.startProfiling()); - expect(store).toMatchSnapshot('2: profiling started'); - - act(() => { - ReactDOM.render(, containerA); - ReactDOM.render(, containerB); - }); - expect(store).toMatchSnapshot('3: update'); - - act(() => ReactDOM.unmountComponentAtNode(containerB)); - expect(store).toMatchSnapshot('4: unmount B'); - - act(() => ReactDOM.unmountComponentAtNode(containerA)); - expect(store).toMatchSnapshot('5: unmount A'); - - act(() => store.stopProfiling()); - expect(store).toMatchSnapshot('6: profiling stopped'); - }); -}); diff --git a/src/__tests__/profiling-test.js b/src/__tests__/profiling-test.js new file mode 100644 index 0000000000..e740d45e4a --- /dev/null +++ b/src/__tests__/profiling-test.js @@ -0,0 +1,108 @@ +// @flow + +describe('profiling', () => { + let React; + let ReactDOM; + let Scheduler; + let TestRenderer; + let store; + let utils; + + beforeEach(() => { + utils = require('./utils'); + utils.beforeEachProfiling(); + + store = global.store; + store.collapseNodesByDefault = false; + + React = require('react'); + ReactDOM = require('react-dom'); + Scheduler = require('scheduler'); + TestRenderer = utils.requireTestRenderer(); + }); + + describe('profilingSummary', () => { + it('should be collected for each commit', async done => { + const Parent = ({ count }) => { + Scheduler.advanceTime(10); + return new Array(count) + .fill(true) + .map((_, index) => ); + }; + const Child = () => { + Scheduler.advanceTime(2); + return null; + }; + + const container = document.createElement('div'); + + utils.act(() => ReactDOM.render(, container)); + expect(store).toMatchSnapshot('1: mount'); + + utils.act(() => store.startProfiling()); + + utils.act(() => ReactDOM.render(, container)); + expect(store).toMatchSnapshot('2: add child'); + + utils.act(() => ReactDOM.render(, container)); + expect(store).toMatchSnapshot('3: remove children'); + + utils.act(() => store.stopProfiling()); + expect(store).toMatchSnapshot('4: profiling stopped'); + + let profilingSummary; + function Suspender({ rendererID, rootID }) { + profilingSummary = store.profilingCache.ProfilingSummary.read({ + rendererID, + rootID, + }); + return null; + } + + const rendererID = utils.getRendererID(); + const rootID = store.roots[0]; + + await utils.actSuspense(() => + TestRenderer.create( + + + + ) + ); + + expect(profilingSummary).toMatchSnapshot('ProfilingSummary'); + + done(); + }); + }); + + it('should remove profiling data when roots are unmounted', async () => { + const Parent = ({ count }) => + new Array(count).fill(true).map((_, index) => ); + const Child = () =>
Hi!
; + + const containerA = document.createElement('div'); + const containerB = document.createElement('div'); + + utils.act(() => { + ReactDOM.render(, containerA); + ReactDOM.render(, containerB); + }); + + utils.act(() => store.startProfiling()); + + utils.act(() => { + ReactDOM.render(, containerA); + ReactDOM.render(, containerB); + }); + + utils.act(() => ReactDOM.unmountComponentAtNode(containerB)); + + utils.act(() => ReactDOM.unmountComponentAtNode(containerA)); + + utils.act(() => store.stopProfiling()); + + // Assert all maps are empty + store.assertExpectedRootMapSizes(); + }); +}); diff --git a/src/__tests__/profilingSummarySerializer.js b/src/__tests__/profilingSummarySerializer.js index 45e8477dfd..a2bcb1b5a0 100644 --- a/src/__tests__/profilingSummarySerializer.js +++ b/src/__tests__/profilingSummarySerializer.js @@ -17,11 +17,7 @@ export function print(profilingSummary, serialize, indent) { return JSON.stringify( { ...profilingSummary, - commitDurations: profilingSummary.commitDurations.map((_, i) => i), - commitTimes: profilingSummary.commitTimes.map((_, i) => i), - initialTreeBaseDurations: [ - ...profilingSummary.initialTreeBaseDurations, - ].map(([id, _], index) => [id, index]), + initialTreeBaseDurations: [...profilingSummary.initialTreeBaseDurations], }, null, 2 diff --git a/src/__tests__/store-test.js b/src/__tests__/store-test.js index 5942574a77..4208eb06ec 100644 --- a/src/__tests__/store-test.js +++ b/src/__tests__/store-test.js @@ -6,6 +6,7 @@ describe('Store', () => { let TestUtils; let agent; let store; + let utils; const act = (callback: Function) => { TestUtils.act(() => { @@ -21,6 +22,7 @@ describe('Store', () => { React = require('react'); ReactDOM = require('react-dom'); TestUtils = require('react-dom/test-utils'); + utils = require('./utils'); }); it('should not allow a root node to be collapsed', () => { @@ -281,8 +283,7 @@ describe('Store', () => { ); expect(store).toMatchSnapshot('7: only third child is suspended'); - // HACK There's only one renderer for this test - const rendererID = Object.keys(agent._rendererInterfaces)[0]; + const rendererID = utils.getRendererID(); act(() => agent.overrideSuspense({ id: store.getElementIDAtIndex(4), @@ -673,8 +674,7 @@ describe('Store', () => { act(() => store.toggleIsCollapsed(store.getElementIDAtIndex(1), false)); expect(store).toMatchSnapshot('2: expand tree'); - // HACK There's only one renderer for this test - const rendererID = Object.keys(agent._rendererInterfaces)[0]; + const rendererID = utils.getRendererID(); const suspenseID = store.getElementIDAtIndex(1); act(() => diff --git a/src/__tests__/storeOwners-test.js b/src/__tests__/storeOwners-test.js index f4853b2fe0..9ed0e53fdc 100644 --- a/src/__tests__/storeOwners-test.js +++ b/src/__tests__/storeOwners-test.js @@ -5,23 +5,16 @@ const { printOwnersList } = require('./storeSerializer'); describe('Store owners list', () => { let React; let ReactDOM; - let TestUtils; + let act; let store; - const act = (callback: Function) => { - TestUtils.act(() => { - callback(); - }); - jest.runAllTimers(); // Flush Bridge operations - }; - beforeEach(() => { store = global.store; store.collapseNodesByDefault = false; React = require('react'); ReactDOM = require('react-dom'); - TestUtils = require('react-dom/test-utils'); + act = require('./utils').act; }); it('should drill through intermediate components', () => { diff --git a/src/__tests__/storeSerializer.js b/src/__tests__/storeSerializer.js index f41cb0fb84..80a5b3f1e1 100644 --- a/src/__tests__/storeSerializer.js +++ b/src/__tests__/storeSerializer.js @@ -70,9 +70,9 @@ export function printStore(store, includeWeight = false) { ); } - if (store.roots.length === 0) { - store.assertEmptyMaps(); - } + // If roots have been unmounted, verify that they've been removed from maps. + // This helps ensure the Store doesn't leak memory. + store.assertExpectedRootMapSizes(); return snapshotLines.join('\n'); } diff --git a/src/__tests__/storeStressSync-test.js b/src/__tests__/storeStressSync-test.js index 54d60ef13e..8877d5d999 100644 --- a/src/__tests__/storeStressSync-test.js +++ b/src/__tests__/storeStressSync-test.js @@ -3,18 +3,11 @@ describe('StoreStress (Sync Mode)', () => { let React; let ReactDOM; - let TestUtils; + let act; let bridge; let store; let print; - const act = (callback: Function) => { - TestUtils.act(() => { - callback(); - }); - jest.runAllTimers(); // Flush Bridge operations - }; - beforeEach(() => { bridge = global.bridge; store = global.store; @@ -22,7 +15,7 @@ describe('StoreStress (Sync Mode)', () => { React = require('react'); ReactDOM = require('react-dom'); - TestUtils = require('react-dom/test-utils'); + act = require('./utils').act; print = require('./storeSerializer').print; }); diff --git a/src/__tests__/storeStressTestConcurrent-test.js b/src/__tests__/storeStressTestConcurrent-test.js index 546db55066..d2a834ca98 100644 --- a/src/__tests__/storeStressTestConcurrent-test.js +++ b/src/__tests__/storeStressTestConcurrent-test.js @@ -3,19 +3,11 @@ describe('StoreStressConcurrent', () => { let React; let ReactDOM; - let TestUtils; + let act; let bridge; let store; let print; - const act = (callback: Function) => { - TestUtils.act(() => { - callback(); - }); - jest.advanceTimersByTime(1000); // Flush rendering and Suspense - jest.runAllTimers(); // Flush Bridge operations - }; - beforeEach(() => { bridge = global.bridge; store = global.store; @@ -23,7 +15,7 @@ describe('StoreStressConcurrent', () => { React = require('react'); ReactDOM = require('react-dom'); - TestUtils = require('react-dom/test-utils'); + act = require('./utils').act; print = require('./storeSerializer').print; }); diff --git a/src/__tests__/utils.js b/src/__tests__/utils.js new file mode 100644 index 0000000000..cb15590ee5 --- /dev/null +++ b/src/__tests__/utils.js @@ -0,0 +1,66 @@ +// @flow + +export function act(callback: Function): void { + const TestUtils = require('react-dom/test-utils'); + TestUtils.act(() => { + callback(); + }); + + // Flush Bridge operations + jest.runAllTimers(); +} + +export async function actSuspense(callback: Function) { + const TestUtils = require('react-dom/test-utils'); + const Scheduler = require('scheduler'); + + // $FlowFixMe Flow doens't know about "await act()" yet + await TestUtils.act(async () => { + callback(); + + // Resolve pending suspense promises + jest.runAllTimers(); + }); + + // Re-render after resolved promises + Scheduler.flushAll(); +} + +export function beforeEachProfiling() { + // Mock React's timing information so that test runs are predictable. + jest.mock('scheduler', () => + // $FlowFixMe Flow does not konw about requireActual + require.requireActual('scheduler/unstable_mock') + ); + + // DevTools itself uses performance.now() to offset commit times + // so they appear relative to when profiling was started in the UI. + jest.spyOn(performance, 'now').mockImplementation( + // $FlowFixMe Flow does not konw about requireActual + require.requireActual('scheduler/unstable_mock').unstable_now + ); +} + +export function getRendererID() { + if (global.agent == null) { + throw Error('Agent unavailable.'); + } + const ids = Object.keys(global.agent._rendererInterfaces); + if (ids.length !== 1) { + throw Error('Multiple renderers attached.'); + } + return ids[0]; +} + +export function requireTestRenderer() { + let hook; + try { + // Hide the hook before requiring TestRenderer, so we don't end up with a loop. + hook = global.__REACT_DEVTOOLS_GLOBAL_HOOK__; + delete global.__REACT_DEVTOOLS_GLOBAL_HOOK__; + + return require('react-test-renderer'); + } finally { + global.__REACT_DEVTOOLS_GLOBAL_HOOK__ = hook; + } +} diff --git a/src/devtools/store.js b/src/devtools/store.js index 8321702717..d6e93d9fb9 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -192,30 +192,49 @@ export default class Store extends EventEmitter { } // This is only used in tests to avoid memory leaks. - assertEmptyMaps() { - this.assertEmptyMap(this._idToElement, '_idToElement'); - this.assertEmptyMap(this._ownersMap, '_ownersMap'); - this.assertEmptyMap( - this._profilingOperationsByRootID, - '_profilingOperationsByRootID' + assertExpectedRootMapSizes() { + if (this.roots.length === 0) { + // The only safe time to assert these maps are empty is when the store is empty. + this.assertMapSizeMatchesRootCount(this._idToElement, '_idToElement'); + this.assertMapSizeMatchesRootCount(this._ownersMap, '_ownersMap'); + + // These maps will be empty unless profiling mode has been started. + // After this, their size should always match the number of roots, + // but unless we want to track additional metadata about profiling history, + // the only safe time to assert this is when the store is empty. + this.assertMapSizeMatchesRootCount( + this._profilingOperationsByRootID, + '_profilingOperationsByRootID' + ); + this.assertMapSizeMatchesRootCount( + this._profilingScreenshotsByRootID, + '_profilingScreenshotsByRootID' + ); + this.assertMapSizeMatchesRootCount( + this._profilingSnapshotsByRootID, + '_profilingSnapshotsByRootID' + ); + } + + // These maps should always be the same size as the number of roots + this.assertMapSizeMatchesRootCount( + this._rootIDToCapabilities, + '_rootIDToCapabilities' ); - this.assertEmptyMap( - this._profilingScreenshotsByRootID, - '_profilingScreenshotsByRootID' + this.assertMapSizeMatchesRootCount( + this._rootIDToRendererID, + '_rootIDToRendererID' ); - this.assertEmptyMap( - this._profilingSnapshotsByRootID, - '_profilingSnapshotsByRootID' - ); - this.assertEmptyMap(this._rootIDToCapabilities, '_rootIDToCapabilities'); - this.assertEmptyMap(this._rootIDToRendererID, '_rootIDToRendererID'); } // This is only used in tests to avoid memory leaks. - assertEmptyMap(map: Map, mapName: string) { - if (map.size !== 0) { + assertMapSizeMatchesRootCount(map: Map, mapName: string) { + const expectedSize = this.roots.length; + if (map.size !== expectedSize) { throw new Error( - `Expected ${mapName} to be empty, got ${map.size}: ${inspect(map, { + `Expected ${mapName} to contain ${expectedSize} items, but it contains ${ + map.size + } items\n\n${inspect(map, { depth: 20, })}` );