Skip to content

Commit

Permalink
Add test run that uses www feature flags (#18234)
Browse files Browse the repository at this point in the history
In CI, we run our test suite against multiple build configurations. For
example, we run our tests in both dev and prod, and in both the
experimental and stable release channels. This is to prevent accidental
deviations in behavior between the different builds. If there's an
intentional deviation in behavior, the test author must account
for them.

However, we currently don't run tests against the www builds. That's
a problem, because it's common for features to land in www before they
land anywhere else, including the experimental release channel.
Typically we do this so we can gradually roll out the feature behind
a flag before deciding to enable it.

The way we test those features today is by mutating the
`shared/ReactFeatureFlags` module. There are a few downsides to this
approach, though. The flag is only overridden for the specific tests or
test suites where you apply the override. But usually what you want is
to run *all* tests with the flag enabled, to protect against unexpected
regressions.

Also, mutating the feature flags module only works when running the
tests against source, not against the final build artifacts, because the
ReactFeatureFlags module is inlined by the build script.

Instead, we should run the test suite against the www configuration,
just like we do for prod, experimental, and so on. I've added a new
command, `yarn test-www`. It automatically runs in CI.

Some of the www feature flags are dynamic; that is, they depend on
a runtime condition (i.e. a GK). These flags are imported from an
external module that lives in www. Those flags will be enabled for some
clients and disabled for others, so we should run the tests against
*both* modes.

So I've added a new global `__VARIANT__`, and a new test command `yarn
test-www-variant`. `__VARIANT__` is set to false by default; when
running `test-www-variant`, it's set to true.

If we were going for *really* comprehensive coverage, we would run the
tests against every possible configuration of feature flags: 2 ^
numberOfFlags total combinations. That's not practical, though, so
instead we only run against two combinations: once with `__VARIANT__`
set to `true`, and once with it set to `false`. We generally assume that
flags can be toggled independently, so in practice this should
be enough.

You can also refer to `__VARIANT__` in tests to detect which mode you're
running in. Or, you can import `shared/ReactFeatureFlags` and read the
specific flag you can about. However, we should stop mutating that
module going forward. Treat it as read-only.

In this commit, I have only setup the www tests to run against source.
I'll leave running against build for a follow up.

Many of our tests currently assume they run only in the default
configuration, and break when certain flags are toggled. Rather than fix
these all up front, I've hard-coded the relevant flags to the default
values. We can incrementally migrate those tests later.
  • Loading branch information
acdlite committed Mar 6, 2020
1 parent 4027f2a commit 115cd12
Show file tree
Hide file tree
Showing 12 changed files with 233 additions and 9 deletions.
124 changes: 124 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,106 @@ jobs:
RELEASE_CHANNEL: experimental
command: yarn test --maxWorkers=2

test_source_www:
docker: *docker
environment: *environment
steps:
- checkout
- *restore_yarn_cache
- *run_yarn
- run:
environment:
RELEASE_CHANNEL: stable
command: yarn test-www --maxWorkers=2

test_source_www_variant:
docker: *docker
environment: *environment
steps:
- checkout
- *restore_yarn_cache
- *run_yarn
- run:
environment:
RELEASE_CHANNEL: stable
command: yarn test-www-variant --maxWorkers=2

test_source_www_prod:
docker: *docker
environment: *environment
steps:
- checkout
- *restore_yarn_cache
- *run_yarn
- run:
environment:
NODE_ENV: production
RELEASE_CHANNEL: stable
command: yarn test-www --maxWorkers=2

test_source_www_variant_prod:
docker: *docker
environment: *environment
steps:
- checkout
- *restore_yarn_cache
- *run_yarn
- run:
environment:
NODE_ENV: production
RELEASE_CHANNEL: stable
command: yarn test-www-variant --maxWorkers=2

test_source_www_experimental:
docker: *docker
environment: *environment
steps:
- checkout
- *restore_yarn_cache
- *run_yarn
- run:
environment:
RELEASE_CHANNEL: experimental
command: yarn test-www --maxWorkers=2

test_source_www_variant_experimental:
docker: *docker
environment: *environment
steps:
- checkout
- *restore_yarn_cache
- *run_yarn
- run:
environment:
RELEASE_CHANNEL: experimental
command: yarn test-www-variant --maxWorkers=2

test_source_www_prod_experimental:
docker: *docker
environment: *environment
steps:
- checkout
- *restore_yarn_cache
- *run_yarn
- run:
environment:
NODE_ENV: production
RELEASE_CHANNEL: experimental
command: yarn test-www --maxWorkers=2

test_source_www_variant_prod_experimental:
docker: *docker
environment: *environment
steps:
- checkout
- *restore_yarn_cache
- *run_yarn
- run:
environment:
NODE_ENV: production
RELEASE_CHANNEL: experimental
command: yarn test-www-variant --maxWorkers=2

test_source_persistent:
docker: *docker
environment: *environment
Expand Down Expand Up @@ -384,6 +484,18 @@ workflows:
- test_source_persistent:
requires:
- setup
- test_source_www:
requires:
- setup
- test_source_www_variant:
requires:
- setup
- test_source_www_prod:
requires:
- setup
- test_source_www_variant_prod:
requires:
- setup
- build:
requires:
- setup
Expand Down Expand Up @@ -415,6 +527,18 @@ workflows:
- test_source_prod_experimental:
requires:
- setup
- test_source_www_experimental:
requires:
- setup
- test_source_www_variant_experimental:
requires:
- setup
- test_source_www_prod_experimental:
requires:
- setup
- test_source_www_variant_prod_experimental:
requires:
- setup
- build_experimental:
requires:
- setup
Expand Down
8 changes: 3 additions & 5 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,7 @@ const OFF = 0;
const ERROR = 2;

module.exports = {
extends: [
'fbjs',
'prettier'
],
extends: ['fbjs', 'prettier'],

// Stop ESLint from looking for a configuration file in parent folders
root: true,
Expand Down Expand Up @@ -147,7 +144,7 @@ module.exports = {
'scripts/**/*.js',
'packages/*/npm/**/*.js',
'packages/dom-event-testing-library/**/*.js',
'packages/react-devtools*/**/*.js'
'packages/react-devtools*/**/*.js',
],
rules: {
'react-internal/no-production-logging': OFF,
Expand All @@ -171,6 +168,7 @@ module.exports = {
__PROFILE__: true,
__UMD__: true,
__EXPERIMENTAL__: true,
__VARIANT__: true,
trustedTypes: true,
},
};
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@
"postinstall": "node node_modules/fbjs-scripts/node/check-dev-engines.js package.json && node ./scripts/flow/createFlowConfigs.js && node ./scripts/yarn/downloadReactIsForPrettyFormat.js",
"debug-test": "cross-env NODE_ENV=development node --inspect-brk node_modules/jest/bin/jest.js --config ./scripts/jest/config.source.js --runInBand",
"test": "cross-env NODE_ENV=development jest --config ./scripts/jest/config.source.js",
"test-www": "cross-env NODE_ENV=development jest --config ./scripts/jest/config.source-www.js",
"test-www-variant": "cross-env NODE_ENV=development VARIANT=true jest --config ./scripts/jest/config.source-www.js",
"test-persistent": "cross-env NODE_ENV=development jest --config ./scripts/jest/config.source-persistent.js",
"debug-test-persistent": "cross-env NODE_ENV=development node --inspect-brk node_modules/jest/bin/jest.js --config ./scripts/jest/config.source-persistent.js --runInBand",
"test-prod": "cross-env NODE_ENV=production jest --config ./scripts/jest/config.source.js",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ let React;
let ReactDOMServer;
let PropTypes;
let ReactCurrentDispatcher;
let enableSuspenseServerRenderer = require('shared/ReactFeatureFlags')
.enableSuspenseServerRenderer;

function normalizeCodeLocInfo(str) {
return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
Expand Down Expand Up @@ -686,7 +688,7 @@ describe('ReactDOMServer', () => {
expect(markup).toBe('<div></div>');
});

if (!__EXPERIMENTAL__) {
if (!enableSuspenseServerRenderer) {
it('throws for unsupported types on the server', () => {
expect(() => {
ReactDOMServer.renderToString(<React.Suspense />);
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -973,10 +973,10 @@ export function assignFiberPropertiesInDEV(
}
if (enableUserTimingAPI) {
target._debugID = source._debugID;
target._debugIsCurrentlyTiming = source._debugIsCurrentlyTiming;
}
target._debugSource = source._debugSource;
target._debugOwner = source._debugOwner;
target._debugIsCurrentlyTiming = source._debugIsCurrentlyTiming;
target._debugNeedsRemount = source._debugNeedsRemount;
target._debugHookTypes = source._debugHookTypes;
return target;
Expand Down
32 changes: 32 additions & 0 deletions packages/shared/forks/ReactFeatureFlags.www-dynamic.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow strict
*/

// In www, these flags are controlled by GKs. Because most GKs have some
// population running in either mode, we should run our tests that way, too,
//
// Use __VARIANT__ to simulate a GK. The tests will be run twice: once
// with the __VARIANT__ set to `true`, and once set to `false`.

export const deferPassiveEffectCleanupDuringUnmount = __VARIANT__;
export const runAllPassiveEffectDestroysBeforeCreates = __VARIANT__;
export const warnAboutSpreadingKeyToJSX = __VARIANT__;

// These are already tested in both modes using the build type dimension,
// so we don't need to use __VARIANT__ to get extra coverage.
export const debugRenderPhaseSideEffectsForStrictMode = __DEV__;
export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__;

// TODO: These flags are hard-coded to the default values used in open source.
// Update the tests so that they pass in either mode, then set these
// to __VARIANT__.
export const enableTrustedTypesIntegration = false;
export const warnAboutShorthandPropertyCollision = true;
export const disableInputAttributeSyncing = false;
export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
export const enableModernEventSystem = false;
7 changes: 5 additions & 2 deletions packages/shared/forks/ReactFeatureFlags.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@

import typeof * as FeatureFlagsType from 'shared/ReactFeatureFlags';
import typeof * as ExportsType from './ReactFeatureFlags.www';
import typeof * as DynamicFeatureFlags from './ReactFeatureFlags.www-dynamic';

// Re-export dynamic flags from the www version.
const dynamicFeatureFlags: DynamicFeatureFlags = require('ReactFeatureFlags');

export const {
debugRenderPhaseSideEffectsForStrictMode,
deferPassiveEffectCleanupDuringUnmount,
Expand All @@ -20,8 +23,9 @@ export const {
warnAboutShorthandPropertyCollision,
disableSchedulerTimeoutBasedOnReactExpirationTime,
warnAboutSpreadingKeyToJSX,
replayFailedUnitOfWorkWithInvokeGuardedCallback,
enableModernEventSystem,
} = require('ReactFeatureFlags');
} = dynamicFeatureFlags;

// On WWW, __EXPERIMENTAL__ is used for a new modern build.
// It's not used anywhere in production yet.
Expand All @@ -39,7 +43,6 @@ export const enableProfilerCommitHooks = false;
export const enableSchedulerTracing = __PROFILE__;
export const enableSchedulerDebugging = true;

export const replayFailedUnitOfWorkWithInvokeGuardedCallback = false;
export const warnAboutDeprecatedLifecycles = true;
export const disableLegacyContext = __EXPERIMENTAL__;
export const warnAboutStringRefs = false;
Expand Down
1 change: 1 addition & 0 deletions scripts/flow/environment.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
declare var __PROFILE__: boolean;
declare var __UMD__: boolean;
declare var __EXPERIMENTAL__: boolean;
declare var __VARIANT__: boolean;

declare var __REACT_DEVTOOLS_GLOBAL_HOOK__: any; /*?{
inject: ?((stuff: Object) => void)
Expand Down
36 changes: 36 additions & 0 deletions scripts/jest/config.source-www.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
'use strict';

const baseConfig = require('./config.base');

const RELEASE_CHANNEL = process.env.RELEASE_CHANNEL;

// Default to building in experimental mode. If the release channel is set via
// an environment variable, then check if it's "experimental".
const __EXPERIMENTAL__ =
typeof RELEASE_CHANNEL === 'string'
? RELEASE_CHANNEL === 'experimental'
: true;

const preferredExtension = __EXPERIMENTAL__ ? '.js' : '.stable.js';

const moduleNameMapper = {};
moduleNameMapper[
'^react$'
] = `<rootDir>/packages/react/index${preferredExtension}`;
moduleNameMapper[
'^react-dom$'
] = `<rootDir>/packages/react-dom/index${preferredExtension}`;

module.exports = Object.assign({}, baseConfig, {
// Prefer the stable forks for tests.
moduleNameMapper,
modulePathIgnorePatterns: [
...baseConfig.modulePathIgnorePatterns,
'packages/react-devtools-shared',
],
setupFiles: [
...baseConfig.setupFiles,
require.resolve('./setupHostConfigs.js'),
require.resolve('./setupTests.www.js'),
],
});
2 changes: 2 additions & 0 deletions scripts/jest/setupEnvironment.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ global.__EXPERIMENTAL__ =
? RELEASE_CHANNEL === 'experimental'
: true;

global.__VARIANT__ = !!process.env.VARIANT;

if (typeof window !== 'undefined') {
global.requestIdleCallback = function(callback) {
return setTimeout(() => {
Expand Down
23 changes: 23 additions & 0 deletions scripts/jest/setupTests.www.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';

jest.mock('shared/ReactFeatureFlags', () => {
jest.mock(
'ReactFeatureFlags',
() => jest.requireActual('shared/forks/ReactFeatureFlags.www-dynamic'),
{virtual: true}
);

const wwwFlags = jest.requireActual('shared/forks/ReactFeatureFlags.www');
const defaultFlags = jest.requireActual('shared/ReactFeatureFlags');

// TODO: Many tests were written before we started running them against the
// www configuration. Update those tests so that they work against the www
// configuration, too. Then remove these overrides.
wwwFlags.disableLegacyContext = defaultFlags.disableLegacyContext;
wwwFlags.warnAboutUnmockedScheduler = defaultFlags.warnAboutUnmockedScheduler;
wwwFlags.enableUserTimingAPI = defaultFlags.enableUserTimingAPI;
wwwFlags.disableJavaScriptURLs = defaultFlags.disableJavaScriptURLs;
wwwFlags.enableDeprecatedFlareAPI = defaultFlags.enableDeprecatedFlareAPI;

return wwwFlags;
});
1 change: 1 addition & 0 deletions scripts/rollup/build.js
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,7 @@ function getPlugins(
__UMD__: isUMDBundle ? 'true' : 'false',
'process.env.NODE_ENV': isProduction ? "'production'" : "'development'",
__EXPERIMENTAL__,
__VARIANT__: false,
}),
// The CommonJS plugin *only* exists to pull "art" into "react-art".
// I'm going to port "art" to ES modules to avoid this problem.
Expand Down

0 comments on commit 115cd12

Please sign in to comment.