-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Run ReactElementJSX-test against bundles #18301
Conversation
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 ea3cccd:
|
global.Symbol = undefined; | ||
|
||
ReactFeatureFlags = require('shared/ReactFeatureFlags'); | ||
ReactFeatureFlags.warnAboutSpreadingKeyToJSX = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already true in the test-www-variant
run, so instead of moving this to a new file, you can wrap the test in a condition:
if (require('shared/ReactFeatureFlags').warnAboutSpreadingKeyToJSX) {
// test goes here
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See here:
react/packages/shared/forks/ReactFeatureFlags.www-dynamic.js
Lines 10 to 18 in 885ed46
// 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__; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh smart
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍Assuming tests pass
It would be nice to have some way to ensure that a test isn’t completely ignored by all runs because we forgot about giving it a variant. |
Yeah that would be nice. Though this is probably only an issue for existing tests, since it'd be weird to write a test that you never run yourself. Also, if we remove all the hardcoded overrides here: react/packages/shared/forks/ReactFeatureFlags.www-dynamic.js Lines 25 to 32 in 885ed46
...and here: react/scripts/jest/setupTests.www.js Lines 13 to 22 in 885ed46
then if a flag is shipped to any of our open source or www channels, it will definitely be tested. So I'm not too worried about this. |
Put another way, it's not that big a deal if we neglect to test a configuration that doesn't actually exist in one of our release channels. It's not ideal, because there could be hidden bugs that don't get discovered until we turn the flag on somewhere. But it's more important to test against every possible configuration that actually gets shipped. |
This is an important test. We should run it on bundles, not just source. See #18299 (comment) for the kind of issue it could catch.
I'm removing the internal suffix and moving the only test that actually depends on feature flags by now into its own file. That one stays internal.