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

Simplify Jest-specific tests #11243

Merged
merged 2 commits into from Oct 17, 2017
Merged

Simplify Jest-specific tests #11243

merged 2 commits into from Oct 17, 2017

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Oct 16, 2017

It was a bit hard to say what's being tested there.
This was relevant in the past but today the only special case we have for Jest is here:

if (instance.render._isMockFunction) {
// We allow auto-mocks to proceed as if they're returning null.
break;

and here:

if (child === undefined && inst.render._isMockFunction) {
// This is probably bad practice. Consider warning here and
// deprecating this convenience.
child = null;

So I just test those specifically instead now.

The automock system is also legacy, and has been disabled by default on www this year. The other things these tests were testing are related to Jest mocking itself but it doesn’t make sense for me that we should test Jest. Especially since we don’t necessarily use the same version as www. If Jest breaks any of this behavior, they will notice it during their www sync so it shouldn’t be on our plate.

See individual commits for details.

They were added after facebook#2576, but were only important when React.createElement was introduced as a migration path.
Now that elements are used consistently, these tests shouldn't be necessary.

I created a separate test specifically for scryRenderedComponentsWithType() though because that was the only one.
Today, the only remaining special behavior for Jest mocks is we let them render undefined.

We don't plan to introduce any other special behavior for them in the future.
(In fact, we already decided against replicating this special behavior for functional components.)

Therefore, we can remove dependency on Jest automocking mechanism in these tests completely,
and just explicitly mock the render method which is the only one for which we have special behavior.

For clarity, we add an explicit test for mockComponent() API (whose naming is a bit of a lie).
Copy link
Contributor

@trueadm trueadm left a comment

Choose a reason for hiding this comment

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

This looks good to me, as it removes much of the testing of Jest auto-mocking that I never really understand was in our codebase. :)

@gaearon gaearon merged commit 043d369 into facebook:master Oct 17, 2017
@gaearon gaearon deleted the jest-mocks-wat branch October 17, 2017 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants