Skip to content

Commit

Permalink
Fix tests: Add dfsEffectsRefactor flag
Browse files Browse the repository at this point in the history
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`.
  • Loading branch information
acdlite committed Nov 13, 2020
1 parent eb2758b commit 1efb4a8
Show file tree
Hide file tree
Showing 6 changed files with 31 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
'use strict';

let React;
let ReactFeatureFlags;
let ReactTestRenderer;
let Scheduler;
let act;
Expand All @@ -19,13 +18,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(() => {
Expand Down Expand Up @@ -73,7 +79,7 @@ describe('ReactDoubleInvokeEvents', () => {
});
});

if (__DEV__ && __VARIANT__) {
if (supportsDoubleInvokeEffects()) {
expect(Scheduler).toHaveYielded([
'useLayoutEffect mount',
'useEffect mount',
Expand Down Expand Up @@ -132,7 +138,7 @@ describe('ReactDoubleInvokeEvents', () => {
});
});

if (__DEV__ && __VARIANT__) {
if (supportsDoubleInvokeEffects()) {
expect(Scheduler).toHaveYielded([
'useEffect One mount',
'useEffect Two mount',
Expand Down Expand Up @@ -193,7 +199,7 @@ describe('ReactDoubleInvokeEvents', () => {
});
});

if (__DEV__ && __VARIANT__) {
if (supportsDoubleInvokeEffects()) {
expect(Scheduler).toHaveYielded([
'useLayoutEffect One mount',
'useLayoutEffect Two mount',
Expand Down Expand Up @@ -250,7 +256,7 @@ describe('ReactDoubleInvokeEvents', () => {
});
});

if (__DEV__ && __VARIANT__) {
if (supportsDoubleInvokeEffects()) {
expect(Scheduler).toHaveYielded([
'useLayoutEffect mount',
'useEffect mount',
Expand Down Expand Up @@ -308,7 +314,7 @@ describe('ReactDoubleInvokeEvents', () => {
ReactTestRenderer.create(<App />, {unstable_isConcurrent: true});
});

if (__DEV__ && __VARIANT__) {
if (supportsDoubleInvokeEffects()) {
expect(Scheduler).toHaveYielded([
'componentDidMount',
'componentWillUnmount',
Expand Down Expand Up @@ -345,7 +351,7 @@ describe('ReactDoubleInvokeEvents', () => {
});
});

if (__DEV__ && __VARIANT__) {
if (supportsDoubleInvokeEffects()) {
expect(Scheduler).toHaveYielded([
'componentDidMount',
'componentWillUnmount',
Expand Down Expand Up @@ -420,7 +426,7 @@ describe('ReactDoubleInvokeEvents', () => {
});
});

if (__DEV__ && __VARIANT__) {
if (supportsDoubleInvokeEffects()) {
expect(Scheduler).toHaveYielded([
'mount',
'useLayoutEffect mount',
Expand Down Expand Up @@ -485,7 +491,7 @@ describe('ReactDoubleInvokeEvents', () => {
ReactTestRenderer.create(<App />, {unstable_isConcurrent: true});
});

if (__DEV__ && __VARIANT__) {
if (supportsDoubleInvokeEffects()) {
expect(Scheduler).toHaveYielded([
'App useLayoutEffect mount',
'App useEffect mount',
Expand All @@ -505,7 +511,7 @@ describe('ReactDoubleInvokeEvents', () => {
_setShowChild(true);
});

if (__DEV__ && __VARIANT__) {
if (supportsDoubleInvokeEffects()) {
expect(Scheduler).toHaveYielded([
'App useLayoutEffect unmount',
'Child useLayoutEffect mount',
Expand Down Expand Up @@ -573,7 +579,7 @@ describe('ReactDoubleInvokeEvents', () => {
});
});

if (__DEV__ && __VARIANT__) {
if (supportsDoubleInvokeEffects()) {
expect(Scheduler).toHaveYielded([
'componentDidMount',
'useLayoutEffect mount',
Expand Down
4 changes: 2 additions & 2 deletions packages/react/src/__tests__/ReactDOMTracing-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
7 changes: 4 additions & 3 deletions packages/react/src/__tests__/ReactProfiler-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ describe('Profiler', () => {

renderer.update(<App />);

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();
Expand Down Expand Up @@ -4292,7 +4292,7 @@ describe('Profiler', () => {
// because the resolved suspended subtree doesn't contain any passive effects.
// If <AsyncComponentWithCascadingWork> 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']);
Expand Down Expand Up @@ -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;

Expand Down
4 changes: 4 additions & 0 deletions scripts/jest/TestFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down

0 comments on commit 1efb4a8

Please sign in to comment.