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

Move all default values into jest-config #9924

Merged
merged 1 commit into from May 16, 2021
Merged

Conversation

ghostd
Copy link
Contributor

@ghostd ghostd commented Apr 29, 2020

Fixes #8826

@codecov-io
Copy link

Codecov Report

Merging #9924 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9924   +/-   ##
=======================================
  Coverage   64.23%   64.23%           
=======================================
  Files         292      292           
  Lines       12466    12466           
  Branches     3075     3077    +2     
=======================================
  Hits         8007     8007           
  Misses       3810     3810           
  Partials      649      649           
Impacted Files Coverage Δ
packages/jest-config/src/Defaults.ts 100.00% <ø> (ø)
packages/jest-config/src/ValidConfig.ts 100.00% <ø> (ø)

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 c5f2fd7...0f16f56. Read the comment docs.

@ghostd ghostd force-pushed the fix-gh-8826 branch 4 times, most recently from 1097f87 to 386d2c1 Compare April 29, 2020 20:45
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

This makes sense to me, thanks! Is this safe to change now, or should we wait for the next major?

@jeysal @thymikee

@ghostd
Copy link
Contributor Author

ghostd commented Apr 30, 2020

I dont know why a test fails only for the windows platform. I dont have any Windows machine to check that :-/
Could it be related to the 'is-ci' feature?

@SimenB
Copy link
Member

SimenB commented Apr 30, 2020

@ghostd don't worry about that one, it seems it's a bit flaky. It passed for all other versions 🙂

clearMocks: false,
collectCoverage: false,
coveragePathIgnorePatterns: [NODE_MODULES_REGEXP],
coverageProvider: 'babel',
coverageReporters: ['json', 'text', 'lcov', 'clover'],
detectLeaks: false,
detectOpenHandles: false,
Copy link
Collaborator

@thymikee thymikee Apr 30, 2020

Choose a reason for hiding this comment

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

I'm fine with removing the defaults from args, but we left some flags like these out of user config on purpose, so they can only be set as, well, CLI flags. Don't we want to do that anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, i didn't know that. I can update my PR in this way. Let me know what fits the project needs.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we wanna keep that

Copy link
Collaborator

@thymikee thymikee May 2, 2020

Choose a reason for hiding this comment

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

@SimenB ICYMI: #6747

Copy link
Member

@SimenB SimenB May 2, 2020

Choose a reason for hiding this comment

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

yeah, I don't want that one 😀 let's just keep it open though, I'll go through more old PRs later and chime in on them

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's close it then :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, to summarize, i need to update my PR to keep the detectOpenHandles default value in CLI options. Any other?

Copy link
Member

Choose a reason for hiding this comment

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

wait, I don't think it matters if the default is here? As longs as it's false I mean. We just don't wanna read it from the user's config file.

@ghostd it's all the new ones added here - we want people to be able to set them from the CLI but not from config (at least not yet, we might change that)

Copy link
Contributor Author

@ghostd ghostd May 4, 2020

Choose a reason for hiding this comment

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

jest-cli already depends on jest-config, so does it make sense to use defaultOptions to init the CLI options object? Hence we'll have the default values in jest-config and all parameters having a default value in the yarg object will be not overrided by the user config.

@SimenB SimenB added this to the Jest 26 milestone May 2, 2020
Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

👍

@cpojer cpojer modified the milestones: Jest 26, Jest 27 May 4, 2020
@jeysal
Copy link
Contributor

jeysal commented May 4, 2020

This makes sense to me, thanks! Is this safe to change now, or should we wait for the next major?

I think the problem in relation to the open discussion may be dropping the implied 'you can import these defaults and stick any of their key value pairs into the Jest config' contract. Maybe jest-config should have an explicit 'not allowed on CLI' handling for some option, but only warn and ignore if people do specify that because they {...defaults} or something.

@SimenB
Copy link
Member

SimenB commented May 4, 2020

something like that should work, yeah. a blacklist should be fine, there are only a few flags we don't want people to stick in their config and forget about

@SimenB
Copy link
Member

SimenB commented Dec 5, 2020

@ghostd would you be up for rebasing this? 🙏

@ghostd
Copy link
Contributor Author

ghostd commented Dec 6, 2020

@SimenB done :)

@cpojer
Copy link
Member

cpojer commented May 15, 2021

Sorry for the long wait, people have been really busy. @ghostd, do you mind rebasing this PR one more time?

@ghostd ghostd force-pushed the fix-gh-8826 branch 2 times, most recently from 66743ea to 1d93971 Compare May 16, 2021 09:43
@ghostd
Copy link
Contributor Author

ghostd commented May 16, 2021

Should be fine now.

Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

👍

@cpojer cpojer merged commit bdd6282 into jestjs:master May 16, 2021
@ghostd ghostd deleted the fix-gh-8826 branch May 17, 2021 06:39
@Hypnosphi
Copy link

Looks like this breaks autodetection of CI environment, because normalize.ts takes the value from argv: https://github.com/facebook/jest/blob/0a902e10e0a5550b114340b87bd31764a7638729/packages/jest-config/src/normalize.ts#L1109

Other cases when something reads from argv may also be affected

@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 Jun 27, 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.

notifyMode is ignored when used at jest.config.js
8 participants