Refactor CLI #3862
Refactor CLI #3862
Conversation
| @@ -33,7 +33,7 @@ type Options = {| | |||
| withAncestor?: boolean, | |||
| |}; | |||
|
|
|||
| export type PathPattern = {| | |||
| export type PathPatternObject = {| | |||
There was a problem hiding this comment.
we already have testNamePattern and testPathPattern, which is confusing. Also this object seem to have more info than just a pattern
There was a problem hiding this comment.
I don't like adding "Object" because everything in JS is an object, hence why it was called PathPattern. How about TestSelectionConfig or TestSelectionPattern because that is what it actually is?
Note: Previous names of this were PathPatternInfo and PathInfo or things like it. It's a tough one.
There was a problem hiding this comment.
i like it (TestSelectionConfig), let's make it that
There was a problem hiding this comment.
I like the TestSelectionConfig too. I'll stamp as soon as you rename it
| import watch from '../watch'; | ||
| import yargs from 'yargs'; | ||
|
|
||
| function run(maybeArgv?: Argv, project?: Path) { |
There was a problem hiding this comment.
run and runCLI are two main functions that get exported and both of them are just a few lines now (all data massaging/parsing is modularized and extracted into separate functions).
ideally i would name run as findJestAndRun, but those are part of public interface
| @@ -16,7 +16,7 @@ import {version as VERSION} from '../../package.json'; | |||
| const logDebugMessages = ( | |||
| globalConfig: GlobalConfig, | |||
| config: ProjectConfig, | |||
| pipe: stream$Writable | tty$WriteStream, | |||
There was a problem hiding this comment.
i wasn't sure what exactly pipe object was until i saw the usage of it. I thought outputStream would be a more descriptive name
Codecov Report
@@ Coverage Diff @@
## master #3862 +/- ##
==========================================
- Coverage 57.63% 57.58% -0.06%
==========================================
Files 194 195 +1
Lines 6782 6792 +10
Branches 6 6
==========================================
+ Hits 3909 3911 +2
- Misses 2870 2878 +8
Partials 3 3
Continue to review full report at Codecov.
|
packages/jest-cli/src/cli/index.js
Outdated
| _patchGlobalFSModule(); | ||
|
|
||
| // If we output a JSON object, we can't write anything to stdout, since | ||
| // it'll break the JSON stracture and it won't be valide. |
There was a problem hiding this comment.
stracture -> structure
valide -> valid
packages/jest-cli/src/cli/index.js
Outdated
|
|
||
| exports.run = run; | ||
| exports.runCLI = runCLI; | ||
| const parsedConfig = readConfig(argv, projects[0]); |
There was a problem hiding this comment.
Why is readConfig(argv, projects[0]) called twice?
There was a problem hiding this comment.
cause i can't even read my own code haha 😪
packages/jest-cli/src/TestRunner.js
Outdated
| addResult, | ||
| buildFailureTestResult, | ||
| makeEmptyTestResult, | ||
| } from './test_result_utils'; |
There was a problem hiding this comment.
@cpojer won't be happy haha. Maybe we can name it test_result_helpers. I like that we're reusing makeEmptyTestResult – how about renaming it to makeEmptyAggregatedResult, just like we call its type?
There was a problem hiding this comment.
it can't be makeEmptyAggregatedResult cause it has other utility functions in it (make test result, add test to aggregated result).
I'll rename it to helpers, but to me it's just another word for utilities :)
There was a problem hiding this comment.
I was referring just to rename makeEmptyTestResult -> makeEmptyAggregatedResult if that makes sense
There was a problem hiding this comment.
oh! yeah, that makes total sense!
packages/jest-cli/src/cli/index.js
Outdated
| let projects = []; | ||
| if (argv.projects) { | ||
| projects = argv.projects; | ||
| } |
There was a problem hiding this comment.
How about
const projects = argv.projects ? argv.projects : [];We're doing condition check and assignments anyway.
aaronabramov
force-pushed
the
refactor-CLI
branch
3 times, most recently
from
52580c4
to
5259bf9
Compare
| @@ -72,9 +72,6 @@ exports[`jest --showConfig outputs config info and exits 1`] = ` | |||
| "mapCoverage": false, | |||
| "noStackTrace": false, | |||
| "notify": false, | |||
| "projects": [ | |||
| "/mocked/root/path/jest/integration_tests/verbose_reporter" | |||
| ], | |||
There was a problem hiding this comment.
this was an intermediate process.cwd() value that was attached to a config in the process of parsing other configs. I change the code to pass it directly
|
I'm on board with this, feel free to merge as soon as @thymikee gives the ok. I won't have time to review this in full, but any simplification here makes a lot of sense. Make sure not to break the world, please. |
aaronabramov
force-pushed
the
refactor-CLI
branch
from
5259bf9
to
93d32ef
Compare
|
Node 4 is green locally 👍. |
|
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. |
I'm working on updating internal fb runner to work with the latest version of Jest, and unfortunately it's very tightly coupled to CLI implementation of Jest open source project.
This PR doesn't add/change any functionality, but rather renames/moves things around.
my main confusion was around the runner modules (
bin/jest,run(),runCLI(),execute(),runJest(),TestRunner,runTests()) which have similar names and seem to do the same thing.and configuration objects that are shared between the runner functions (Yarn args object,
argv,DefaultOptions,InitialObjects,ProjectConfig,GlobalConfig,testNamePattern,testPathPattern,maxWorkers,pattern, etc.). Most of them seem to be intended to be immutable, but unfortunately get mutated throughout the flow (updateArgv,globalConfig = Object.freeze(Object.assign({}, globalConfig, {newValue: true}))).we talked with @thymikee and @cpojer and came up with a plan to merge most of the configuration options into
globalConfigand stop passingargvright after we parse the initial config value.This PR renames some of the object/variable/function names, and splits up/modularizes the running pipeline for jest (rather than having everything in one big function)