From 0f5741c9d0cf21c43c192e196d010dfa57ad8e92 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 13 Nov 2020 13:30:07 -0600 Subject: [PATCH] Fix tests: Add `dfsEffectsRefactor` flag Some of the tests that gated on the effects refactor used the `new` flag. In order to bisect, we'll need to decompose the new fork changes into multiple steps. So I added a hardcoded test flag called `dfsEffectsRefactor` and set it to false. Will turn back on when we switch back to traversing the finished tree using DFS and `subtreeTag`. --- ...DOMServerPartialHydration-test.internal.js | 2 +- .../__tests__/ReactWrongReturnPointer-test.js | 1 + .../ReactDoubleInvokeEvents-test.internal.js | 31 ++++++++++++------- .../ReactDOMTracing-test.internal.js | 4 +-- .../__tests__/ReactProfiler-test.internal.js | 7 +++-- scripts/jest/TestFlags.js | 4 +++ 6 files changed, 31 insertions(+), 18 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index 9a897789b47ca..c8e2a0f5ea988 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -367,7 +367,7 @@ describe('ReactDOMServerPartialHydration', () => { // This is a new node. expect(span).not.toBe(span2); - if (gate(flags => flags.new)) { + if (gate(flags => flags.dfsEffectsRefactor)) { // The effects list refactor causes this to be null because the Suspense Offscreen's child // is null. However, since we can't hydrate Suspense in legacy this change in behavior is ok expect(ref.current).toBe(null); diff --git a/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js b/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js index 688fb5c926ed5..72cb43d75386c 100644 --- a/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js +++ b/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js @@ -16,6 +16,7 @@ beforeEach(() => { }); // Don't feel too guilty if you have to delete this test. +// @gate dfsEffectsRefactor // @gate new // @gate __DEV__ test('warns in DEV if return pointer is inconsistent', async () => { diff --git a/packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.internal.js b/packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.internal.js index e5136af4e2406..7c46459da3bab 100644 --- a/packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.internal.js @@ -19,13 +19,20 @@ describe('ReactDoubleInvokeEvents', () => { beforeEach(() => { jest.resetModules(); React = require('react'); - ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactTestRenderer = require('react-test-renderer'); Scheduler = require('scheduler'); - ReactFeatureFlags.enableDoubleInvokingEffects = __VARIANT__; act = ReactTestRenderer.unstable_concurrentAct; }); + function supportsDoubleInvokeEffects() { + return gate( + flags => + flags.build === 'development' && + flags.enableDoubleInvokingEffects && + flags.dfsEffectsRefactor, + ); + } + it('should not double invoke effects in legacy mode', () => { function App({text}) { React.useEffect(() => { @@ -73,7 +80,7 @@ describe('ReactDoubleInvokeEvents', () => { }); }); - if (__DEV__ && __VARIANT__) { + if (supportsDoubleInvokeEffects()) { expect(Scheduler).toHaveYielded([ 'useLayoutEffect mount', 'useEffect mount', @@ -132,7 +139,7 @@ describe('ReactDoubleInvokeEvents', () => { }); }); - if (__DEV__ && __VARIANT__) { + if (supportsDoubleInvokeEffects()) { expect(Scheduler).toHaveYielded([ 'useEffect One mount', 'useEffect Two mount', @@ -193,7 +200,7 @@ describe('ReactDoubleInvokeEvents', () => { }); }); - if (__DEV__ && __VARIANT__) { + if (supportsDoubleInvokeEffects()) { expect(Scheduler).toHaveYielded([ 'useLayoutEffect One mount', 'useLayoutEffect Two mount', @@ -250,7 +257,7 @@ describe('ReactDoubleInvokeEvents', () => { }); }); - if (__DEV__ && __VARIANT__) { + if (supportsDoubleInvokeEffects()) { expect(Scheduler).toHaveYielded([ 'useLayoutEffect mount', 'useEffect mount', @@ -308,7 +315,7 @@ describe('ReactDoubleInvokeEvents', () => { ReactTestRenderer.create(, {unstable_isConcurrent: true}); }); - if (__DEV__ && __VARIANT__) { + if (supportsDoubleInvokeEffects()) { expect(Scheduler).toHaveYielded([ 'componentDidMount', 'componentWillUnmount', @@ -345,7 +352,7 @@ describe('ReactDoubleInvokeEvents', () => { }); }); - if (__DEV__ && __VARIANT__) { + if (supportsDoubleInvokeEffects()) { expect(Scheduler).toHaveYielded([ 'componentDidMount', 'componentWillUnmount', @@ -420,7 +427,7 @@ describe('ReactDoubleInvokeEvents', () => { }); }); - if (__DEV__ && __VARIANT__) { + if (supportsDoubleInvokeEffects()) { expect(Scheduler).toHaveYielded([ 'mount', 'useLayoutEffect mount', @@ -485,7 +492,7 @@ describe('ReactDoubleInvokeEvents', () => { ReactTestRenderer.create(, {unstable_isConcurrent: true}); }); - if (__DEV__ && __VARIANT__) { + if (supportsDoubleInvokeEffects()) { expect(Scheduler).toHaveYielded([ 'App useLayoutEffect mount', 'App useEffect mount', @@ -505,7 +512,7 @@ describe('ReactDoubleInvokeEvents', () => { _setShowChild(true); }); - if (__DEV__ && __VARIANT__) { + if (supportsDoubleInvokeEffects()) { expect(Scheduler).toHaveYielded([ 'App useLayoutEffect unmount', 'Child useLayoutEffect mount', @@ -573,7 +580,7 @@ describe('ReactDoubleInvokeEvents', () => { }); }); - if (__DEV__ && __VARIANT__) { + if (supportsDoubleInvokeEffects()) { expect(Scheduler).toHaveYielded([ 'componentDidMount', 'useLayoutEffect mount', diff --git a/packages/react/src/__tests__/ReactDOMTracing-test.internal.js b/packages/react/src/__tests__/ReactDOMTracing-test.internal.js index 7be6513a5737d..5026605672fcb 100644 --- a/packages/react/src/__tests__/ReactDOMTracing-test.internal.js +++ b/packages/react/src/__tests__/ReactDOMTracing-test.internal.js @@ -152,7 +152,7 @@ describe('ReactDOMTracing', () => { onInteractionScheduledWorkCompleted, ).toHaveBeenLastNotifiedOfInteraction(interaction); - if (gate(flags => flags.new)) { + if (gate(flags => flags.dfsEffectsRefactor)) { expect(onRender).toHaveBeenCalledTimes(3); } else { // TODO: This is 4 instead of 3 because this update was scheduled at @@ -310,7 +310,7 @@ describe('ReactDOMTracing', () => { expect( onInteractionScheduledWorkCompleted, ).toHaveBeenLastNotifiedOfInteraction(interaction); - if (gate(flags => flags.new)) { + if (gate(flags => flags.dfsEffectsRefactor)) { expect(onRender).toHaveBeenCalledTimes(3); } else { // TODO: This is 4 instead of 3 because this update was scheduled at diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js index 831861941a24d..a87c83e9c180e 100644 --- a/packages/react/src/__tests__/ReactProfiler-test.internal.js +++ b/packages/react/src/__tests__/ReactProfiler-test.internal.js @@ -368,7 +368,7 @@ describe('Profiler', () => { renderer.update(); - if (gate(flags => flags.new)) { + if (gate(flags => flags.dfsEffectsRefactor)) { // None of the Profiler's subtree was rendered because App bailed out before the Profiler. // So we expect onRender not to be called. expect(callback).not.toHaveBeenCalled(); @@ -4292,7 +4292,7 @@ describe('Profiler', () => { // because the resolved suspended subtree doesn't contain any passive effects. // If or its decendents had a passive effect, // onPostCommit would be called again. - if (gate(flags => flags.new)) { + if (gate(flags => flags.dfsEffectsRefactor)) { expect(Scheduler).toFlushAndYield([]); } else { expect(Scheduler).toFlushAndYield(['onPostCommit']); @@ -4783,7 +4783,8 @@ describe('Profiler', () => { }); if (__DEV__) { - // @gate new + // @gate dfsEffectsRefactor + // @gate enableDoubleInvokingEffects it('double invoking does not disconnect wrapped async work', () => { ReactFeatureFlags.enableDoubleInvokingEffects = true; diff --git a/scripts/jest/TestFlags.js b/scripts/jest/TestFlags.js index 6a2be742e2960..f869620f03290 100644 --- a/scripts/jest/TestFlags.js +++ b/scripts/jest/TestFlags.js @@ -44,6 +44,10 @@ const environmentFlags = { // Use this for tests that are known to be broken. FIXME: false, + + // Turn this flag back on (or delete) once the effect list is removed in favor + // of a depth-first traversal using `subtreeTags`. + dfsEffectsRefactor: false, }; function getTestFlags() {