Refactor setting config from argv #3424

thymikee · 2017-04-30T16:45:24Z

Summary

This PR aims to refactor how we translate config from CLI.

Notes:

Set default CLI boolean options to undefined, so yargs doesn't default them to false (defaults are handled in defaults.js so I put watchman and cache there)
Thanks to that setFromArgv is now more generic, translating any value (with some exceptions) to new config before normalizing (makes some weird hacks around config/argv unnecessary).
Renamed setState within jest-cli to updateArgv because that's what it does.
Types within normalize are a bit loosened now (they didn't really work fully, which we need to update in a followup)

Fixes #3403.

Test plan

jest

codecov-io · 2017-04-30T17:13:55Z

Codecov Report

Merging #3424 into master will increase coverage by 0.57%.
The diff coverage is 89.65%.

@@            Coverage Diff             @@
##           master    #3424      +/-   ##
==========================================
+ Coverage    63.8%   64.38%   +0.57%     
==========================================
  Files         179      179              
  Lines        6648     6620      -28     
  Branches        5        5              
==========================================
+ Hits         4242     4262      +20     
+ Misses       2404     2356      -48     
  Partials        2        2

Impacted Files	Coverage Δ
packages/jest-config/src/defaults.js	`100% <ø> (ø)`	⬆️
packages/jest-cli/src/runJest.js	`0% <0%> (ø)`	⬆️
packages/jest-cli/src/watch.js	`76.11% <100%> (ø)`	⬆️
packages/jest-cli/src/lib/updateArgv.js	`95.23% <100%> (ø)`
packages/jest-config/src/utils.js	`95.91% <100%> (+0.36%)`	⬆️
packages/jest-util/src/validateCLIOptions.js	`100% <100%> (ø)`	⬆️
packages/jest-config/src/index.js	`30% <50%> (+3.91%)`	⬆️
packages/jest-config/src/normalize.js	`89.15% <88.88%> (+2.22%)`	⬆️
packages/jest-config/src/setFromArgv.js	`95.65% <95.45%> (+93.21%)`	⬆️
... 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 4a99428...8345460. Read the comment docs.

SimenB · 2017-05-01T11:39:05Z

I tested this branch, and it doesn't work for me (yes, I ran yarn build this time).

$ yarn jest-coverage -- --testPathPattern eslint --coverage-reporters html
yarn jest-coverage v0.23.3
$ yarn run jest --silent -- --coverage --testPathPattern eslint --coverage-reporters html
/Users/simen/Development/jest/packages/jest-util/build/validateCLIOptions.js:62
    throw createCLIValidationError(unrecognizedOptions, allowedOptions);
    ^

● Unrecognized CLI Parameters:

  Following options were not recognized:
  ["coverage-reporters", "coverageReporters"]

  CLI Options Documentation:
  http://facebook.github.io/jest/docs/cli.html

error Command failed with exit code 1.
error Command failed with exit code 1.

If I'm misunderstanding the issue this is supposed to fix I can open up a separate issue for it 😄

thymikee · 2017-05-01T13:13:46Z

I'm still planning more testing for this tomorrow, forgot to mention that. Right now I just wanted to have feature parity. Thanks for looking into it!

cpojer · 2017-05-01T16:26:01Z

@thymikee this is super awesome, I didn't think we'd get this done before Jest 20. I really appreciate you taking this on and will give a more thorough review tomorrow.

One thing that I like about all of the rewrites we did for Jest 20 is that the entire codebase became a lot more approachable and everything makes much more sense. We now have a clear flow from:

DefaultOptions + User specified options + Argv -> InitialOptions -> ProjectConfig & GlobalConfig

That's beautiful.

cpojer · 2017-05-02T13:16:45Z

packages/jest-cli/src/cli/args.js

@@ -51,11 +51,12 @@ const docs = 'Documentation: https://facebook.github.io/jest/';
 const options = {
  bail: {
    alias: 'b',
+    default: undefined,


what's the default in yargs if you don't specify this line in this file?

For booleans it's false, otherwise it's undefined

cpojer · 2017-05-02T13:17:57Z

packages/jest-cli/src/lib/updateArgv.js

@@ -9,9 +9,18 @@
 */
 'use strict';

+import type {Argv} from 'types/Config';
+type Options = {


Should this be an exact type?

cpojer · 2017-05-02T13:18:13Z

packages/jest-cli/src/lib/updateArgv.js

@@ -9,9 +9,18 @@
 */
 'use strict';


I like that you renamed this file!

cpojer · 2017-05-02T13:19:14Z

packages/jest-config/src/index.js

@@ -40,7 +39,7 @@ async function readConfig(
 const parseConfig = argv => {
  if (argv.config && typeof argv.config === 'string') {
    // If the passed in value looks like JSON, treat it as an object.
-    if (argv.config[0] === '{' && argv.config[argv.config.length - 1] === '}') {
+    if (argv.config.startsWith('{') && argv.config.endsWith('}')) {


oh! Didn't realize this was part of node 4.

cpojer · 2017-05-02T13:20:09Z

packages/jest-config/src/setFromArgv.js

+  let configFromArgv;
+  const argvToConfig = Object.keys(argv)
+    .filter(key => argv[key] !== undefined && specialArgs.indexOf(key) === -1)
+    .reduce((acc: Object, key) => {


can we call this "options" instead of "acc"?

cpojer · 2017-05-02T13:20:33Z

packages/jest-config/src/setFromArgv.js

+    .reduce((acc: Object, key) => {
+      switch (key) {
+        case 'coverage':
+          acc['collectCoverage'] = argv[key];


I'd prefer to not use string keys. Did you do this to make flow happy? If so, you could put the keys in the default value that you pass to the reduce call.

Oh, pardon moi, I should just use dot notation 👍

cpojer · 2017-05-02T13:21:43Z

types/Config.js

@@ -181,3 +183,74 @@ export type ProjectConfig = {|
  transformIgnorePatterns: Array<Glob>,
  unmockedModulePathPatterns: ?Array<string>,
 |};
+
+export type Argv = {


Do you mind reordering this file in terms of how it is flowing through Jest? Something like this:

Argv
DefaultOptions
InitialOptions
GlobalConfig
ProjectConfig

I think that makes it a bit more readable. We could also consider having a separate types/Options file for Argv and DefaultOptions/InitialOptions.

thymikee · 2017-05-02T19:50:25Z

@cpojer Addressed all the requests
@SimenB can you test it now?

SimenB · 2017-05-02T20:30:07Z

It does work now! Should support kebab in addition to camel cased, though 😄

$ yarn jest-coverage -- --testPathPattern eslint --coverage-reporters html
yarn jest-coverage v0.23.3
$ yarn run jest --silent -- --coverage --testPathPattern eslint --coverage-reporters html
/Users/simen/Development/jest/packages/jest-util/build/validateCLIOptions.js:63
    throw createCLIValidationError(unrecognizedOptions, allowedOptions);
    ^

● Unrecognized CLI Parameter:

  Unrecognized option "coverage-reporters". Did you mean "coverageReporters"?

  CLI Options Documentation:
  http://facebook.github.io/jest/docs/cli.html

error Command failed with exit code 1.
error Command failed with exit code 1.

Especially since I got this before:

  Following options were not recognized:
  ["coverage-reporters", "coverageReporters"]

cpojer · 2017-05-02T21:25:21Z

types/Argv.js

+ */
+'use strict';
+
+export type Argv = {


such a beautiful type.

cpojer · 2017-05-02T21:26:19Z

packages/jest-config/src/utils.js

@@ -59,27 +59,29 @@ const _replaceRootDirInPath = (rootDir: string, filePath: Path): string => {
  );
 };

+const _replaceRootDirInObject = (rootDir: string, config: any): Object => {
+  if (config instanceof RegExp) {


I actually feel like we may be able to remove this in a follow-up PR. It can't be a regex at this point, can it?

cpojer · 2017-05-02T21:31:55Z

packages/jest-config/src/setFromArgv.js

+      return options;
+    }, {});
+
+  if (isJSON(argv.config)) {


I think this needs to be moved to the top of this function, it needs to happen earlier.

Changed the order of merging the returned object, --watch will now take precedence over --config="{"watch": false}"

cpojer · 2017-05-02T23:52:20Z

YES. Great work @thymikee.

* Rename setState to updateArgv * Add Argv type * Remove unnecessary argv.help check * Refactor setting config from argv * Rename acc to options * Use dot notation instead of string keys * Make Options of updateArgv an exact type * Move Argv type to it's own file * Update showConfig snapshot with 'projects' * Add missing options to args * Update setFromArgv and Argv types * Extract and reuse isJSON * Update updateArgv.js * Update validateCLIOptions.js * Move regex check out of _replaceRootDirInObject * Make explicit flags override those from --config * Rename isJSON -> isJSONString * Add reporters to args

github-actions · 2021-05-11T22:07:10Z

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.

facebook-github-bot added the CLA Signed ✔️ label Apr 30, 2017

cpojer reviewed May 2, 2017

View reviewed changes

packages/jest-cli/src/lib/updateArgv.js

@@ -9,9 +9,18 @@

*/

'use strict';

Copy link

Member

cpojer May 2, 2017

Choose a reason for hiding this comment

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

I like that you renamed this file!

cpojer reviewed May 2, 2017

View reviewed changes

thymikee force-pushed the overhaul-argv branch from 1d8dacf to 6d9d797 Compare May 2, 2017 19:49

cpojer added this to the Jest 20 milestone May 2, 2017

cpojer reviewed May 2, 2017

View reviewed changes

types/Argv.js

*/

'use strict';

export type Argv = {

Copy link

Member

cpojer May 2, 2017

Choose a reason for hiding this comment

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

such a beautiful type.

cpojer reviewed May 2, 2017

View reviewed changes

thymikee added 11 commits May 3, 2017 01:39

Rename setState to updateArgv

d20854a

Add Argv type

ab8f391

Remove unnecessary argv.help check

f481a0a

Refactor setting config from argv

594bcb8

Rename acc to options

251d014

Use dot notation instead of string keys

173fc16

Make Options of updateArgv an exact type

b4f4388

Move Argv type to it's own file

373cfec

Update showConfig snapshot with 'projects'

018bc27

Add missing options to args

8a2cc9f

Update setFromArgv and Argv types

9f88773

thymikee and others added 7 commits May 3, 2017 01:42

Extract and reuse isJSON

e3bf2da

Update updateArgv.js

6122274

Update validateCLIOptions.js

0dca09e

Move regex check out of _replaceRootDirInObject

c9e6bd7

Make explicit flags override those from --config

0014b2f

Rename isJSON -> isJSONString

14561fb

Add reporters to args

8345460

thymikee force-pushed the overhaul-argv branch from 6c85ee6 to 8345460 Compare May 2, 2017 23:46

cpojer merged commit 062edb0 into jestjs:master May 2, 2017

thymikee deleted the overhaul-argv branch March 16, 2019 10:55

github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor setting config from argv #3424

Refactor setting config from argv #3424

thymikee commented Apr 30, 2017 •

edited

Loading

codecov-io commented Apr 30, 2017 •

edited

Loading

SimenB commented May 1, 2017 •

edited

Loading

thymikee commented May 1, 2017

cpojer commented May 1, 2017

cpojer May 2, 2017

thymikee May 2, 2017

cpojer May 2, 2017

cpojer May 2, 2017

cpojer May 2, 2017

cpojer May 2, 2017

thymikee May 2, 2017

cpojer May 2, 2017

thymikee May 2, 2017

cpojer May 2, 2017

thymikee commented May 2, 2017

SimenB commented May 2, 2017 •

edited

Loading

cpojer May 2, 2017

cpojer May 2, 2017

cpojer May 2, 2017

thymikee May 2, 2017

cpojer commented May 2, 2017

github-actions bot commented May 11, 2021

Refactor setting config from argv #3424

Refactor setting config from argv #3424

Conversation

thymikee commented Apr 30, 2017 • edited Loading

codecov-io commented Apr 30, 2017 • edited Loading

Codecov Report

SimenB commented May 1, 2017 • edited Loading

thymikee commented May 1, 2017

cpojer commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thymikee commented May 2, 2017

SimenB commented May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented May 2, 2017

github-actions bot commented May 11, 2021

thymikee commented Apr 30, 2017 •

edited

Loading

codecov-io commented Apr 30, 2017 •

edited

Loading

SimenB commented May 1, 2017 •

edited

Loading

SimenB commented May 2, 2017 •

edited

Loading