-
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
Use public API in tests wherever possible #9954
Conversation
ReactCurrentOwner = require('ReactCurrentOwner'); | ||
ReactTestUtils = require('ReactTestUtils'); | ||
ReactCurrentOwner = require('react') | ||
.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactCurrentOwner; |
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 seems fair to me—it's what renderers do too.
@@ -20,7 +20,7 @@ describe('ReactIncremental', () => { | |||
beforeEach(() => { | |||
jest.resetModules(); | |||
React = require('react'); | |||
ReactNoop = require('ReactNoop'); | |||
ReactNoop = require('react-noop-renderer'); |
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 seemed to make sense to me, I already used it for debugger in the past, and it would be nice to run tests on a Fiber bundle even for non-DOM specific tests.
Not all of these required packages have forwarding in https://github.com/facebook/react/tree/master/src/node_modules so how can you require them from jest in a clean checkout? |
Hmm, didn't I add them in this PR? |
Oh I guess they're gitignored by default so I didn't push. |
Should be good now. |
@@ -13,17 +13,15 @@ | |||
|
|||
describe('onlyChild', () => { | |||
var React; | |||
var onlyChild; | |||
var WrapComponent; | |||
|
|||
beforeEach(() => { | |||
React = require('react'); |
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.
@gaearon How does this require
work right now? I couldn't find any jest mocks for 'react'
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.
Check src/node_modules/react
This moves us closer to a world where we can test both DEV and PROD flat bundles (and thus more safely experiment with PROD mangling). We already have
expectDev()
which will be very handy for this.It turned out we're already doing a good job in many places, the biggest violators are feature flags and event tests.
For now, I am changing to use public npm-like public API imports wherever possible. I left a few Stack files untouched as they'll go away. I also didn't touch feature flags which will fall away organically soon.
In places where it seems like we might want public API tests, I left comments instilling skepticism into the reader. The more tests we can run on bundles in the future, the better.
I think that once Stack and feature flags are gone, we can start enforcing that some files "opt into" testing on bundles and can't introduce internal dependencies. This is future work and doesn't block this PR which just converts most cases.