Use testing builds for our own tests #17985
Conversation
facebook-github-bot
added
CLA Signed
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 fe0c945:
|
Details of bundled changes.Comparing: 256d78d...fe0c945 react-dom
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (stable) |
Details of bundled changes.Comparing: 256d78d...fe0c945 react-dom
ReactDOM: size: 0.0%, gzip: -0.1% Size changes (experimental) |
threepointone
force-pushed
the
use-testing-build
branch
from
f4b49da to
70579f1
Compare
threepointone
force-pushed
the
use-testing-build
branch
8 times, most recently
from
fe3bb48 to
f39963f
Compare
There was a problem hiding this comment.
I understand why using a global is easier. But we need to figure out a solution to Jest not respecting the forks used by the build script. It's one of the main blockers to being able to get rid of the .internal suffix. This is going to become increasingly important as we start maintaining multiple builds of React (modern, legacy, etc).
In the meantime, instead of using a global, could you keep the feature flag and use a jest mock?
jest.mock('shared/ReactFeatureFlags', () =>
require.requireActual('shared/forks/ReactFeatureFlags.testing.js')
);This means when running the tests against source (i.e. not the bundle tests), all renderers will get the testing feature flags instead of the ones specific to their fork. But that's actually no worse than what already happens today, where they get the default feature flags. And it's no worse than using a global, which is compiled away during build.
Then, whenever we get around to solving the forking problem, it will also solve this one.
threepointone
force-pushed
the
use-testing-build
branch
2 times, most recently
from
e25d4e1 to
331ab49
Compare
Following up from facebook#17915 where we generated testing builds for `react-dom`, this PR uses those builds for our own tests. - fixes `ReactFeatureFlags.testing` to use all the default flags - changes `ReactFeatureFlags.readonly` to return `isTestEnvironment: true` (this might not strictly be needed) - with jest, mocks `react-dom` with the testing version, both for source and builds - uses `ReactDOM.act` in one of these tests to verify it actually works
threepointone
force-pushed
the
use-testing-build
branch
from
331ab49 to
fe0c945
Compare
|
Updated the PR and description. Copy pasting a specific concern - I'll write a post before I land this, but this setup has an annoyance. If you add a new feature flag, and run your tests, your test will probably fail, since it's likely you've not added it to the |
|
You can reexport one from the other: // shared/forks/ReactFeatureFlags.www.testing.js
export {
flags,
that,
are,
the,
same
} from './ReactFeatureFlags.www';
export const differentFlag = 'forked'; |
|
I tried this and it failed because it kept referring to itself and failing the build. Maybe I didn’t try it hard enough, I’ll try again. |
| @@ -30,6 +30,7 @@ | |||
| "README.md", | |||
| "build-info.json", | |||
| "index.js", | |||
| "testing.js", | |||
There was a problem hiding this comment.
Putting this here means it will go into the next public release. If you can't get rid of it because the bundle tests rely on it, maybe prefix with unstable?
| @@ -9,4 +9,4 @@ | |||
| // It lets us determine whether we're running in Fire mode without making tests internal. | |||
| const ReactFeatureFlags = require('../ReactFeatureFlags'); | |||
| // Forbid writes because this wouldn't work with bundle tests. | |||
| module.exports = Object.freeze({...ReactFeatureFlags}); | |||
| module.exports = Object.freeze({...ReactFeatureFlags, isTestEnvironment: true}); | |||
There was a problem hiding this comment.
Don't need this anymore, right?
You can move the default feature flags to their own file, |
|
abandoning this for #18196 |
Following up from #17915 where we generated testing builds for
react-dom, this PR uses those builds for our own tests.isTestEnvironment:true.fbfork for thetesting.jsentrypointisTestEnvironment: truetoReactFeatureFlags.readonlyreact-domwith the testing version, both for source and buildsReactDOM.actin one of these tests to verify it actually worksThis also meant I had to add
testing.jstoreact-dom'spackage.json(so we can still runyarn test-build). If you feel strongly about not exposing this yet, maybe I could rename it totesting_DO_NOT_USE.js?I'll write a post before I land this, but this setup has an annoyance. If you add a new feature flag, and run your tests, your test will probably fail, since it's likely you've not added it to the
.testingforks. There's nothing to warn this except for flow. which might not be running. Open to ideas for syncing these forks.