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

Adding --noDuplicateMockWarn cli option #6037

Closed
wants to merge 7 commits into from

Conversation

kgroat
Copy link

@kgroat kgroat commented Apr 19, 2018

Summary

This option disables the jest-haste-map: duplicate manual mock found warning as discussed in #2070. This is a temporary solution to the underlying problem, but one that will allow users to stop seeing the warning when running their test suite.

Test plan

I've ensured that the existing HasteMap test verifying that the warning is logged still succeeds, and added a test to show that, when the option is set to true, the warning is not logged.

Also tested the new cli arg.

Old output:

screen shot 2018-04-19 at 2 08 32 pm

New output in config file:

screen shot 2018-04-19 at 4 34 06 pm

New output through cli argument:

screen shot 2018-04-19 at 4 36 02 pm

This option disables the “jest-haste-map: duplicate manual mock found” warning
@codecov-io
Copy link

codecov-io commented Apr 19, 2018

Codecov Report

Merging #6037 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #6037   +/-   ##
======================================
  Coverage    64.3%   64.3%           
======================================
  Files         217     217           
  Lines        8317    8317           
  Branches        3       4    +1     
======================================
  Hits         5348    5348           
  Misses       2968    2968           
  Partials        1       1
Impacted Files Coverage Δ
packages/jest-runtime/src/index.js 72.89% <ø> (ø) ⬆️
packages/jest-config/src/index.js 36% <ø> (ø) ⬆️
packages/jest-config/src/valid_config.js 100% <ø> (ø) ⬆️
packages/jest-config/src/defaults.js 92.85% <ø> (ø) ⬆️
packages/jest-haste-map/src/index.js 96.28% <100%> (ø) ⬆️
packages/jest-config/src/normalize.js 94.14% <100%> (ø) ⬆️
... and 1 more

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 440e9c6...4c01635. Read the comment docs.

@cuvelierm
Copy link

cuvelierm commented Apr 20, 2018

Will try to fix that error this afternoon for you

@cuvelierm
Copy link

Just ran all code locally, but didn't receive the error, sorry I can't help.

@kgroat
Copy link
Author

kgroat commented Apr 20, 2018

I'm unable to reproduce the error either. What is the error saying / indicative of?

Also, is there a way to re-run the test-node-8 checks short of pushing a new change?

@kgroat
Copy link
Author

kgroat commented Apr 20, 2018

Seems like I figured out the issue 😀

@SimenB
Copy link
Member

SimenB commented Apr 23, 2018

Does this make sense as a cli flag? Seems more like config - you don't want to turn off the warnings for a single run, you either want them there or not every time

@kgroat
Copy link
Author

kgroat commented Apr 23, 2018

I also tested this as a config option, and it works there, too; see the image labeled "New output in config file" above.

If you mean that it should only be available as config, that does make sense. However, I'm not sure how to make it available as a config option but not as a flag from the cli. Is it just a matter of removing it from packages/jest-cli/src/cli/args.js, or is there more to it?

@beckend
Copy link

beckend commented Apr 27, 2018

Will there be an option i package.json as well?

@kgroat
Copy link
Author

kgroat commented Apr 30, 2018

@beckend yes, this includes a config option, which will be available in the package.json "jest" config. The discussion is whether it should only be a config option or if it should also be available as a cli flag.

@kgroat
Copy link
Author

kgroat commented May 16, 2018

Any review comments or word on if / when this can get merged?

@rickhanlonii
Copy link
Member

@cpojer what do you think about this?

@cpojer
Copy link
Member

cpojer commented May 17, 2018

I’d much rather overhaul the entire mocking system before merging an option like this tbh.

@cpojer
Copy link
Member

cpojer commented May 17, 2018

Actually, I think @rubennorte’s diff around hasteImpl will make this warning be a thing of the past.

@rubennorte
Copy link
Contributor

@cpojer the mock warning is displayed before the worker processes the file, so the change in hasteImpl won't affect this.

@cpojer
Copy link
Member

cpojer commented May 17, 2018

@rubennorte oh good point. Maybe we can change that and together with your PR it would solve all the problems.

@rubennorte
Copy link
Contributor

@cpojer sounds good. I'll do that.

@cpojer
Copy link
Member

cpojer commented May 22, 2018

We'll go that route, so I'm closing this PR here. @kgroat thanks so much for sending a PR to Jest. I hope the alternative solution we'll provide will work for you too!

@cpojer cpojer closed this May 22, 2018
@Unibozu
Copy link

Unibozu commented May 30, 2018

@cpojer where can we check the progress of the mocking system overhaul? this PR was referenced from #2070, it would be amazing to know when the solution is expected to go live

@SimenB
Copy link
Member

SimenB commented May 30, 2018

#6104

@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 12, 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.

None yet

10 participants