Add test run that uses www feature flags #18234
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
facebook-github-bot
added
React Core Team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 89ba911:
|
Details of bundled changes.Comparing: 4027f2a...89ba911 react-dom
react-art
Size changes (experimental) |
Details of bundled changes.Comparing: 4027f2a...89ba911 react-art
react-dom
Size changes (stable) |
acdlite
force-pushed
the
www-tests
branch
2 times, most recently
from
a9a522f
to
e7a3213
Compare
acdlite
commented
Mar 6, 2020
| wwwFlags.warnAboutUnmockedScheduler = defaultFlags.warnAboutUnmockedScheduler; | ||
| wwwFlags.enableUserTimingAPI = defaultFlags.enableUserTimingAPI; | ||
| wwwFlags.disableJavaScriptURLs = defaultFlags.disableJavaScriptURLs; | ||
| wwwFlags.enableDeprecatedFlareAPI = defaultFlags.enableDeprecatedFlareAPI; |
There was a problem hiding this comment.
@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 | }
Not sure if this is an issue or not, but letting you know just in case. Since Flare is enabled in the www build.
There was a problem hiding this comment.
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.
acdlite
commented
Mar 6, 2020
| deferPassiveEffectCleanupDuringUnmount, | ||
| disableInputAttributeSyncing, | ||
| enableTrustedTypesIntegration, | ||
| runAllPassiveEffectDestroysBeforeCreates, | ||
| warnAboutShorthandPropertyCollision, | ||
| disableSchedulerTimeoutBasedOnReactExpirationTime, | ||
| warnAboutSpreadingKeyToJSX, | ||
| replayFailedUnitOfWorkWithInvokeGuardedCallback, |
There was a problem hiding this comment.
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.
sebmarkbage
approved these changes
Mar 6, 2020
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/ReactFeatureFlagsmodule. 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 commandyarn test-www-variant.__VARIANT__is set to false by default; when runningtest-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 totrue, and once with it set tofalse. 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 importshared/ReactFeatureFlagsand 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.