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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: update yargs to fix CLI flag overriding #9519

Merged
merged 6 commits into from Mar 1, 2020

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Feb 5, 2020

Summary

Fixes #9517. @jeysal we really need to add io-ts or something to our config as we've talked about multiple times 馃槵

Test plan

Test added. Without rolling back yargs it fails.
image

@codecov-io
Copy link

codecov-io commented Feb 5, 2020

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #9519   +/-   ##
======================================
  Coverage      65%     65%           
======================================
  Files         283     283           
  Lines       12151   12151           
  Branches     3010    3010           
======================================
  Hits         7899    7899           
  Misses       3608    3608           
  Partials      644     644

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 e376f4a...3361486. Read the comment docs.

@thymikee
Copy link
Collaborator

thymikee commented Feb 5, 2020

I never knew overriding flags like this was supported. Before rolling back, I'd rather like to consult with Yargs team if this is expected behavior or a bug. Can you forward mentioned issue to them?

@SimenB
Copy link
Member Author

SimenB commented Feb 5, 2020

I've pinged Ben privately

@jeysal
Copy link
Contributor

jeysal commented Feb 5, 2020

I could add introducing io-ts to jest-config to my todo list, just not sure if worth it before the config refactor?
Perhaps it's not a super big effort actually, plus it'll demonstrate how we can use it and might make it easier for people who don't know it involved in the config refactor.
Also as you probably know my todo list is not being processed super quickly 馃槄

@SimenB
Copy link
Member Author

SimenB commented Feb 5, 2020

@thymikee regardless, this is either a bug or an oversight in our config normalization - we pass config from file through jest-validate, but not from argv.

https://github.com/facebook/jest/blob/c4ae594d1f09fe37542da0597ad979dacdf27345/packages/jest-config/src/normalize.ts#L460-L484


@jeysal if not a big effort I'd welcome it 馃榾 io-ts specifically might be a bit to FP, not sure? If you have the time to explore though, that'd be sweet

@thymikee
Copy link
Collaborator

thymikee commented Feb 5, 2020

@SimenB
Copy link
Member Author

SimenB commented Feb 5, 2020

That just checks for deprecated entries and typos, it doesn't validate the shape of the object's values.

An alternative is to merge initialOptions and argv before passing through validate.

diff --git i/packages/jest-config/src/normalize.ts w/packages/jest-config/src/normalize.ts
index a2454c373..4e86339c6 100644
--- i/packages/jest-config/src/normalize.ts
+++ w/packages/jest-config/src/normalize.ts
@@ -457,7 +457,9 @@ export default function normalize(
   hasDeprecationWarnings: boolean;
   options: AllOptions;
 } {
-  const {hasDeprecationWarnings} = validate(initialOptions, {
+  const argvAndInitialOptions = setFromArgv(initialOptions, argv);
+
+  const {hasDeprecationWarnings} = validate(argvAndInitialOptions, {
     comment: DOCUMENTATION_NOTE,
     deprecatedConfig: DEPRECATED_CONFIG,
     exampleConfig: VALID_CONFIG,
@@ -476,7 +478,7 @@ export default function normalize(
   let options = normalizePreprocessor(
     normalizeReporters(
       normalizeMissingOptions(
-        normalizeRootDir(setFromArgv(initialOptions, argv)),
+        normalizeRootDir(argvAndInitialOptions),
         configPath,
         projectIndex,
       ),

That explodes for the different options we have that's CLI-only though, as those are included in the argv object, but not considered valid.

@SimenB
Copy link
Member Author

SimenB commented Feb 5, 2020

That somehow broke coverage reports? Seems like it collapsed collectCoverageOnlyFrom into a single one as well

@SimenB
Copy link
Member Author

SimenB commented Feb 12, 2020

Will also be fixed by yargs@16: yargs/yargs#1556

@drew-gross

This comment has been minimized.

@SimenB SimenB changed the title fix: rollback yargs to fix CLI flag overriding fix: update yargs to fix CLI flag overriding Mar 1, 2020
@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9519   +/-   ##
=======================================
  Coverage   65.09%   65.09%           
=======================================
  Files         287      287           
  Lines       12145    12145           
  Branches     3007     3007           
=======================================
  Hits         7906     7906           
  Misses       3604     3604           
  Partials      635      635

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 003bd50...023c8d8. Read the comment docs.

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

jest --silent --no-silent no longer negates silent flag
6 participants