Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test run that uses www feature flags #18234

Merged
merged 1 commit into from
Mar 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed this was off in the www build. I think maybe we wanted to ramp this up in case it breaks something? Seems unlikely, but to decouple it from the rest of the changes I've made it a dynamic flag.

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;
Copy link
Collaborator Author

@acdlite acdlite Mar 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trueadm When I ran the tests with Flare enabled, this test started failing sporadically:

Summary of all failing tests
 FAIL  packages/react-dom/src/__tests__/ReactDOMFiber-test.js
  ● ReactDOMFiber › should not update event handlers until commit

    Expected warning was not recorded:
      "Warning: unstable_flushDiscreteUpdates: Cannot flush updates when React is already rendering."

      1051 |         expect(() => {
      1052 |           node.click();
    > 1053 |         }).toErrorDev(
           |            ^
      1054 |           'Warning: unstable_flushDiscreteUpdates: Cannot flush updates when React is already rendering.',
      1055 |         );
      1056 |       }

Details: https://circleci.com/gh/facebook/react/97022?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Not sure if this is an issue or not, but letting you know just in case. Since Flare is enabled in the www build.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is known, and it seemed like maybe a fragile test from memory as it’s related to the discrete flushing logic based on the timeStamp logic.


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