Keep ARGV only in CLI files #4012

aaronabramov · 2017-07-11T22:52:32Z

right now we're passing both ARGV and globalConfig all the way down to test runner. Also we mutate both of them as we pass it down (globalConfig is frozen, but we clone it instead of directly mutating).

This PR separates ARGV from globalObject and makes it that ARGV is only used in CLI context (ARGV only represents the actual passed arguments to CLI now).

globalObject is still mutable (frozen, but rewritten) and i believe Jest workflow suggests that it should be this way. For example, when we run --watch mode, user's interactions change globalConfig and then we re-run Jest with the new config.

cc @rogeliog for watch stuff

aaronabramov

it also greatly simplifies the argument flow of jest.
we don't need to pass testPathPattern, testNamePattern and testPattern as a string throughout the flow.
and we don't need TestSelectionConfig any more, since it was a derived data structure

aaronabramov · 2017-07-11T22:53:40Z

integration_tests/__tests__/__snapshots__/custom_reporters.test.js.snap

@@ -97,7 +77,7 @@ exports[`Custom Reporters Integration default reporters enabled 2`] = `
 Tests:       1 passed, 1 total
 Snapshots:   0 total
 Time:        <<REPLACED>>
-Ran all test suites matching \\"add.test.js\\".


as discussed in #4006 this now prints the pattern instead of mix of pattern+raw input

aaronabramov · 2017-07-11T22:55:02Z

packages/jest-cli/src/__tests__/__snapshots__/watch.test.js.snap

@@ -4,7 +4,7 @@ exports[`Watch mode flows Runs Jest once by default and shows usage 1`] = `
 Array [
  "
 Watch Usage
- › Press o to only run tests related to changed files.
+ › Press a to run all tests.


this is not the actual behavior change, rather the test change because of different default mock value

aaronabramov · 2017-07-11T22:55:20Z

packages/jest-cli/src/__tests__/__snapshots__/watch_test_name_pattern_mode.test.js.snap

@@ -228,5 +228,3 @@ exports[`Watch mode flows Results in pattern mode get truncated appropriately 2`
 [MOCK - cursorTo(12, 5)]
 [MOCK - cursorRestorePosition]"
 `;
-


i used a direct comparison here instead of snapshot

aaronabramov · 2017-07-11T22:56:40Z

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

@@ -1,73 +0,0 @@
-/**


we don't need this data structure any more, since all needed data can be taken from globalConfig now

aaronabramov · 2017-07-11T22:58:39Z

packages/jest-cli/src/run_jest.js

-        updateArgv(argv, 'watchAll', {noSCM: true});
-        testSelectionConfig = getTestSelectionConfig(argv);
-        data = await source.getTestPaths(testSelectionConfig);
+        // updateArgv(argv, 'watchAll', {noSCM: true});


this line seems to be noop now, but i wasn't 100% sure, i'll follow up.
(we might be missing a test case that tests this behavior)

I'm not sure that I understand why this is not needed anymore

aaronabramov · 2017-07-11T23:00:28Z

packages/jest-cli/src/test_path_pattern_to_regexp.js

+ * @flow
+ */
+
+module.exports = (testPathPattern: string) => new RegExp(testPathPattern, 'i');


since we serialize/deserialize patterns when we spawn workers, i chose to have a single function that can produce consistent result on both sides (probably should put it in a comment in this file)

I like this one

aaronabramov · 2017-07-11T23:00:45Z

packages/jest-cli/src/watch.js

@@ -22,7 +21,8 @@ import isValidPath from './lib/is_valid_path';
 import preRunMessage from './pre_run_message';
 import createContext from './lib/create_context';
 import runJest from './run_jest';
-import updateArgv from './lib/update_argv';
+// import updateArgv from './lib/update_argv';


TODO: remove

aaronabramov · 2017-07-11T23:01:59Z

packages/jest-config/src/index.js

      noStackTrace: options.noStackTrace,
+      nonFlagArgs: options.nonFlagArgs,


the value of ARGV._ with a normal name

aaronabramov · 2017-07-11T23:05:51Z

packages/jest-config/src/index.js

      logHeapUsage: options.logHeapUsage,
      mapCoverage: options.mapCoverage,
+      maxWorkers: options.maxWorkers,
+      noSCM: undefined,


this one is complicated, because we only know its value after we searched for tests (jest-changed-files).
currently in Jest master we modify globalConfig after we get the result and i think this is not a good pattern.
I think we should split runJest function into two and run them sequentially:

const globalConfig = readConfig(...); const {noSCM, tests} = findTests(globalConfig); globalConfig.noSCM = noSCM; globalConfig.somethingElse = ... // there's a few modifications that we're already doing in runJest Object.freeze(globalConfig); // at this point config is immutable. no hacks runJest(globalConfig);

aaronabramov · 2017-07-11T23:06:57Z

packages/jest-config/src/normalize.js

+  newOptions.nonFlagArgs = argv._;
+  newOptions.testPathPattern = buildTestPathPattern(argv);
+  newOptions.json = argv.json;
+  newOptions.lastCommit = argv.lastCommit;


i'm not sure if it's the right way to do it. Adding these values to the switch block above leaves them undefined

cpojer · 2017-07-12T09:16:14Z

I don't think I agree that this many new options have to be added to GlobalConfig. To keep this manageable, given that they all mostly belong to one or two features, I'd recommend grouping them in one or two objects within GlobalConfig instead. What do you think?

aaronabramov · 2017-07-12T16:35:15Z

@cpojer i don't think we should go this way honestly.
i don't see how flat object is less manageable than hierarchical structure.
most of our config options used only for one/two features (expand, logHeapUsage, notify, bail, etc...)
i definitely though about grouping all new options first, but then realized that this only makes things more complicated. (have to deepFreeze, no one to one mapping with CLI, have to look into object structure every time because you don't know which group it belongs to, flowtype refining magic, etc.)

if we do want to move to grouped configs, a can see it as

globalConfig: {
  testSelection: {testPathPattern: ..., testNamePattern: ..., ...},
  runnerOptions: {onlyChanged: ..., bail: ..., maxWorkers: ...},
  coverage: {threshold: ..., coverageIgnorePatterns: ...},
  // ...
}

but then you have to decide whether --coverage will be a part of runnerOptions or coverage group. or if updateSnapshot should be a runner options or have its own snapshot group, and if it's its own group.

so basically my thoughts are - if we start grouping options together, there's no good convention or rule of how to group them and it will eventually become very confusing :(

codecov-io · 2017-07-12T20:51:50Z

Codecov Report

Merging #4012 into master will increase coverage by 0.07%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##           master    #4012      +/-   ##
==========================================
+ Coverage   60.31%   60.39%   +0.07%     
==========================================
  Files         196      196              
  Lines        6761     6754       -7     
  Branches        6        6              
==========================================
+ Hits         4078     4079       +1     
+ Misses       2680     2672       -8     
  Partials        3        3

Impacted Files	Coverage Δ
packages/jest-config/src/validate_pattern.js	`100% <ø> (ø)`
packages/jest-config/src/valid_config.js	`100% <ø> (ø)`	⬆️
packages/jest-config/src/index.js	`0% <ø> (ø)`	⬆️
packages/jest-cli/src/search_source.js	`75% <0%> (-1.28%)`	⬇️
...ckages/jest-cli/src/reporters/coverage_reporter.js	`49.36% <0%> (-0.64%)`	⬇️
...ackages/jest-cli/src/reporters/summary_reporter.js	`21.31% <0%> (-1.27%)`	⬇️
packages/jest-cli/src/run_jest.js	`0% <0%> (ø)`	⬆️
packages/jest-cli/src/reporters/notify_reporter.js	`12.5% <0%> (-0.55%)`	⬇️
packages/jest-cli/src/get_changed_files_promise.js	`100% <100%> (ø)`	⬆️
...ckages/jest-cli/src/test_path_pattern_to_regexp.js	`100% <100%> (ø)`
... and 6 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 03c6814...942dde3. Read the comment docs.

thymikee · 2017-07-12T20:59:14Z

integration_tests/__tests__/__snapshots__/custom_reporters.test.js.snap

+        \\"path\\": false
+    },
+    \\"options\\": {
+        \\"Dmitrii Abramov\\": \\"Awesome\\"


probably needs to be updated too :D

Spare that for another PR :p

thymikee · 2017-07-12T21:11:05Z

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

+  globalConfig,
+  mode,
+  options,
+}: {


There's not so many options here, so maybe we can switch to function arguments instead of destructuring? We could avoid duplicating the names in type def.

it was just arguments before, but there's a lot of calls with either mode or options undefined, so it became
updateGlobalConfig(globalConfig, null, options)

actually you're right, i don't see any reason to keep it two separate arguments. i'll just merge them into

updateGlobalConfig(globalConfig: GlobalConfig, options: Options);

thymikee · 2017-07-12T21:47:32Z

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

@@ -263,7 +263,6 @@ const _run = async (

  argv.watch || argv.watchAll


should be taken from globalConfig now?

oh wow... i didn't notice this. This simplifies things even further. All of those functions don't need argv any more. (and flow didn't catch that again) :)

thymikee · 2017-07-12T21:58:46Z

packages/jest-cli/src/watch.js

-  updateArgv(argv, argv.watch ? 'watch' : 'watchAll', {
-    testNamePattern: argv.testNamePattern,
-    testPathPattern: argv.testPathPattern || (argv._ || []).join('|'),
-  });


Because of lack of this update logic, globalConfig.onlyChanged is not updated properly on the first run, resulting in tests being run, despite there's nothing changed in git.

good catch! adding it back (although updating onlyChanged implicitly seems like something we should fix in the future)

thymikee

Fix the bug with watch mode I mentioned in comments and I think this is good to go. Looks good to me for now.

cpojer · 2017-07-13T09:57:25Z

I'd like it if we could keep "testSelection" as a globalConfig param.

Just to double check, these args are not part of InitialOptions, therefore cannot be specified in a configuration file, right? I think that's the right way to go for these, there is never a time when hardcoding the test selection makes sense imho.

aaronabramov · 2017-07-13T14:20:02Z

@cpojer test selection config was completely derived data that duplicated what global config already had. with this PR we mutate global config directly, and there's no way to bring teat selection back :( teat selection config was the reason why i couldn't implement "-all". because both globalConfig and teatSelectionConfig had 'onlyChanged" property, and i couldn't mutate both

…

On Thu, Jul 13, 2017 at 2:57 AM Christoph Nakazawa ***@***.***> wrote: I'd like it if we could keep "testSelection" as a globalConfig param. Just to double check, these args are *not* part of InitialOptions, therefore cannot be specified in a configuration file, right? I think that's the right way to go for these, there is never a time when hardcoding the test selection makes sense imho. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4012 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA5YZYCl48luPNOqmczBCNh-xqdZKvgcks5sNeoGgaJpZM4OU6l0> .

cpojer · 2017-07-13T14:27:23Z

Ok, you convinced me. No mutations, though, only copying!

aaronabramov · 2017-07-13T14:28:21Z

yeah. that's what i meant :)

…

On Thu, Jul 13, 2017 at 7:27 AM Christoph Nakazawa ***@***.***> wrote: Ok, you convinced me. No mutations, though, only copying! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4012 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA5YZe5IvtjbX9ONSC1lUcPQJRKqwGLJks5sNilOgaJpZM4OU6l0> .

rogeliog

Nice clean up, watch mode looks good to me... Just some really minor comments

rogeliog · 2017-07-13T16:22:30Z

packages/jest-cli/src/__tests__/watch.test.js

@@ -22,10 +22,11 @@ jest.doMock(
  () =>
    function() {
      const args = Array.from(arguments);
+      const {onComplete} = args[0];


Super nit: couldn't this be const [onComplete] = args?

rogeliog · 2017-07-13T16:25:56Z

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

+  options?: Options,
+}): GlobalConfig => {
+  // $FlowFixMe Object.assign
+  const newConfig: GlobalConfig = Object.assign({}, globalConfig);


Nice, we are not mutating it any more!

rogeliog · 2017-07-13T16:33:55Z

packages/jest-cli/src/run_jest.js

-        updateArgv(argv, 'watchAll', {noSCM: true});
-        testSelectionConfig = getTestSelectionConfig(argv);
-        data = await source.getTestPaths(testSelectionConfig);
+        // updateArgv(argv, 'watchAll', {noSCM: true});


I'm not sure that I understand why this is not needed anymore

rogeliog · 2017-07-13T16:36:40Z

packages/jest-cli/src/test_path_pattern_to_regexp.js

+ * @flow
+ */
+
+module.exports = (testPathPattern: string) => new RegExp(testPathPattern, 'i');


I like this one

aaronabramov · 2017-07-14T00:23:23Z

ok. seems like i addressed them all!

github-actions · 2021-05-13T10:07:09Z

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 Jul 11, 2017

aaronabramov force-pushed the test_path_pattern branch from 6b28cf2 to 90ab5f1 Compare July 11, 2017 22:52

aaronabramov commented Jul 11, 2017

View reviewed changes

This was referenced Jul 12, 2017

move maxWorkers to globalConfig #4005

Merged

Add support for custom summary reporter #3828

Closed

aaronabramov force-pushed the test_path_pattern branch from 6ad5483 to 6e56c6e Compare July 12, 2017 19:36

aaronabramov mentioned this pull request Jul 12, 2017

jest --all #4020

Merged

thymikee reviewed Jul 12, 2017

View reviewed changes

thymikee requested changes Jul 13, 2017

View reviewed changes

rogeliog approved these changes Jul 13, 2017

View reviewed changes

Keep ARGV only in CLI files

942dde3

aaronabramov force-pushed the test_path_pattern branch from 6e56c6e to 942dde3 Compare July 13, 2017 23:55

aaronabramov merged commit 7bf328d into jestjs:master Jul 14, 2017

aaronabramov deleted the test_path_pattern branch July 14, 2017 00:40

tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017

Keep ARGV only in CLI files (jestjs#4012)

7921b03

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

		noStackTrace: options.noStackTrace,
		nonFlagArgs: options.nonFlagArgs,

		@@ -263,7 +263,6 @@ const _run = async (

		argv.watch \|\| argv.watchAll

Keep ARGV only in CLI files #4012

Keep ARGV only in CLI files #4012

Conversation

aaronabramov commented Jul 11, 2017

aaronabramov left a 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

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 Jul 12, 2017

aaronabramov commented Jul 12, 2017

codecov-io commented Jul 12, 2017 • edited Loading

Codecov Report

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 Jul 12, 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

thymikee left a comment

Choose a reason for hiding this comment

cpojer commented Jul 13, 2017

aaronabramov commented Jul 13, 2017 via email

cpojer commented Jul 13, 2017

aaronabramov commented Jul 13, 2017 via email

rogeliog left a 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

aaronabramov commented Jul 14, 2017

github-actions bot commented May 13, 2021

codecov-io commented Jul 12, 2017 •

edited

Loading

thymikee Jul 12, 2017 •

edited

Loading