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

add globalMocks, remove mocksPattern #2705

Closed
wants to merge 44 commits into from

Conversation

ColCh
Copy link
Contributor

@ColCh ColCh commented Jan 26, 2017

Summary

this is a temporary solution - it's a LOT OF things to be done to adapt "HasteMaps" data structures to understand conception of local/global mocks

but it solves duplicate mock error:

jest-haste-map: duplicate manual mock found:
  Module name: results
  Duplicate Mock path: /project/src/redux/thunks/__mocks__/results.js
This warning is caused by two manual mock files with the same file name.
Jest will use the mock file found in: 
/project/src/redux/thunks/__mocks__/results.js
 Please delete one of the following two files: 
 /project/src/redux/__mocks__/results.js
/project/src/redux/thunks/__mocks__/results.js

I've implemented thing discussed here

#2070 (comment)

#2070 (comment)

Test plan

  • tested jest itself with yarn test
  • tested our projects with forked version (yarn link). Duplicate mock warning is gone, all our tests have passed. We have mocks for app modules as well as mocks for node_modules

merging this PR should close #2070

@ColCh ColCh changed the title 2070 global mocks add globalMocks Jan 26, 2017
@jest-bot
Copy link
Contributor

jest-bot commented Jan 26, 2017

Warnings
⚠️ Changes were made to package.json, but not to yarn.lock - Perhaps you need to run `yarn install`?

Generated by 🚫 dangerJS

@codecov-io
Copy link

codecov-io commented Jan 26, 2017

Codecov Report

Merging #2705 into master will increase coverage by 0.08%.

@@            Coverage Diff             @@
##           master    #2705      +/-   ##
==========================================
+ Coverage    67.9%   67.98%   +0.08%     
==========================================
  Files         140      140              
  Lines        5057     5067      +10     
==========================================
+ Hits         3434     3445      +11     
+ Misses       1623     1622       -1
Impacted Files Coverage Δ
packages/jest-config/src/defaults.js 100% <ø> (ø)
packages/jest-config/src/validConfig.js 100% <ø> (ø)
packages/jest-runtime/src/transform.js 87.87% <ø> (ø)
packages/jest-runtime/src/index.js 86% <ø> (-0.06%)
packages/jest-config/src/normalize.js 86.01% <100%> (+0.4%)
packages/jest-haste-map/src/index.js 95.49% <100%> (+0.12%)
packages/jest-runtime/src/shouldInstrument.js 68.42% <100%> (+1.75%)
packages/pretty-format/src/index.js 98.48% <ø> (+0.5%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 852ac99...1daf077. Read the comment docs.

@cpojer
Copy link
Member

cpojer commented Jan 27, 2017

This is fantastic. Thanks for doing this. I honestly don't think it's that big of a deal!

Here are two suggestions that I think we should do to take it over the finish line:

  • Remove "mocksPattern" from everywhere in Jest. It shouldn't be configurable. Make it a constant in jest-haste-map and it should point to "mocks". It was never supposed to be configurable and it just adds unnecessary complexity. If people don't like it, I'm ok if they stop using Jest over it.
  • If the mock doesn't match globalMocks, I don't think you need to add it to the haste map at all. jest-runtime/jest-resolve should be able to look for the local mocks already. Can you just try that to confirm my suspicion? I think the jest-haste-map mocks are only really needed at FB.

@cpojer
Copy link
Member

cpojer commented Jan 27, 2017

Finally, I believe at Facebook we will need to keep allowing all mocks that are currently in the system. Can you think of a way to make it so it can work the same way as it did before? Otherwise we'd have to add hundreds of folders to this config at Facebook.

@ColCh
Copy link
Contributor Author

ColCh commented Jan 27, 2017

I honestly don't think it's that big of a deal!

In case hashmaps structures are used for local mocks too, of course... but they are not :) so it's relatively easy

I don't think you need to add it to the haste map at all. jest-runtime/jest-resolve should be able to look for the local mocks already. Can you just try that to confirm my suspicion?

You're right. Checked against jest tests and our projects tests

Finally, I believe at Facebook we will need to keep allowing all mocks that are currently in the system. Can you think of a way to make it so it can work the same way as it did before? Otherwise we'd have to add hundreds of folders to this config at Facebook.

what about globalMocks: true?

@ColCh
Copy link
Contributor Author

ColCh commented Jan 30, 2017

@cpojer ready to check again. I've added a special undocumented value to force every mock as global:

globalMocks: ['*']

which internally gets translated to globalMocks: true after normalize step.

AFAIK It's not possible to use globalMocks: true in config, because jest-validate simply asserts types of passed in example config, not allowing more than one type for one config entry. Please point, if I'm wrong at this.

@thymikee
Copy link
Collaborator

That's right, you can have just one type for user provided config in jest-validate. But there are number of places (e.g. transform) where user config is translated into completely different type. You just need to provide a valid example in jest-config/src/validConfig.js

@ColCh ColCh changed the title add globalMocks add globalMocks, remove mocksPattern Feb 1, 2017
@peterjacobson
Copy link

peterjacobson commented Apr 5, 2017

Hi, I notice it's been two months since this was touched.
My tests are logging warnings:

jest-haste-map: duplicate manual mock found:
  Module name: ErrorUtils
  Duplicate Mock path: /Users/pete/projects/fitadvisor_app/node_modules/react-native/node_modules/fbjs/lib/__mocks__/ErrorUtils.js
This warning is caused by two manual mock files with the same file name.
Jest will use the mock file found in:
/Users/pete/projects/fitadvisor_app/node_modules/react-native/node_modules/fbjs/lib/__mocks__/ErrorUtils.js
 Please delete one of the following two files:
 /Users/pete/projects/fitadvisor_app/node_modules/react-native/Libraries/Core/__mocks__/ErrorUtils.js
/Users/pete/projects/fitadvisor_app/node_modules/react-native/node_modules/fbjs/lib/__mocks__/ErrorUtils.js

Does this PR solve this bug, or has there been another solution in the meantime?

Thanks for all your work @ColCh, and appreciating the clear, kind and constructive way you host this community @cpojer 🥇

@cpojer
Copy link
Member

cpojer commented May 11, 2017

Ok, I think we need to find a different solution to this and we should be looking at it, but haven't had enough time recently to do so. I'll close this PR for now because it is quite outdated at this point.

@cpojer cpojer closed this May 11, 2017
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] duplicate manual mock found in separate directories
7 participants