Refactor and improve watch mode with testPathPattern and testNamePattern #2568
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2568 +/- ##
==========================================
+ Coverage 67.12% 67.74% +0.62%
==========================================
Files 142 146 +4
Lines 5126 5237 +111
==========================================
+ Hits 3441 3548 +107
- Misses 1685 1689 +4
Continue to review full report at Codecov.
|
| onlyChanged: false, | ||
| watch: false, | ||
| watchAll: true, | ||
| }); | ||
| }); | ||
|
|
||
| it('Pressing "T" enters testname pattern mode', () => { |
There was a problem hiding this comment.
Maybe we can update these tests to look similar to https://github.com/facebook/jest/blob/master/packages/jest-cli/src/__tests__/watch-pattern-mode-test.js#L86 in those tests we heavily use snapshots and they give us some nice insight of how the state updates over time :)
| delete argv.testPathPattern; | ||
| delete argv.testNamePattern; | ||
|
|
||
| if (options && options.onlyChanged) { |
There was a problem hiding this comment.
I think we can use a the default param syntax to prevent all of the options &&
packages/jest-cli/src/runJest.js
Outdated
| @@ -37,7 +37,7 @@ const getTestSummary = ( | |||
| : ''; | |||
|
|
|||
| const nameInfo = argv.testNamePattern | |||
| ? chalk.dim(' with tests matching ') + `"${argv.testNamePattern}"` | |||
| ? chalk.dim(' with tests matching ') + `"/${argv.testNamePattern}/"` | |||
There was a problem hiding this comment.
Why these '/' is so that it looks like a RegExp?
packages/jest-cli/src/watch.js
Outdated
| }); | ||
| let currentPattern = ''; | ||
|
|
||
| let isEnteringString = false; |
There was a problem hiding this comment.
As pattern mode becomes more and more complex it seems that we need to start extracting all of this logic out of watch.js.. so that the pattern mode and typeahead stuff can scale and be maintained in the future.
There was a problem hiding this comment.
yay extract more. Love that this code is getting so solid.
|
@zouxuoz are you getting back to this? |
|
@cpojer yes, I will do at the weekend |
|
TO DO
|
|
@zouxuoz Sorry that I haven't reviewed your latest changes, I've been really busy but I'll try to get to it as soon as possible 😄 |
|
@rogeliog no problem. I'm ping you when PR is ready for review 😉 |
|
Hey! What's the status of this? @zouxuoz is this ready for review? I'd like to get it in for Jest 19. Also, as a follow-up, it would be cool to cache the test names after the first run so we can also use pattern mode for this after the initial run! |
| pipe: stream$Writable | tty$WriteStream, | ||
| promptController: PromptController, | ||
| ) => { | ||
| class TestPathPatternModeController { |
There was a problem hiding this comment.
Why not just exporting the class directly
| pipe.write(ansiEscapes.cursorShow); | ||
|
|
||
| promptController.prompt( | ||
| this.onChange.bind(this), |
There was a problem hiding this comment.
We can bind this in the constructor or use class property syntax so that we don't have to re-bind it every time that it runs.
bind in constructor
constructor() {
this.onChange = this.onChange.bind(this);
}class property
onChange = () => {
// ...content
}|
|
||
| promptController.prompt(jest.fn(), onSuccess, jest.fn()); | ||
|
|
||
| promptController.put('74'); |
There was a problem hiding this comment.
I think we could extract this to a KEYS object at the top level of this file, this should make the tests easier to follow in the future :)
let {KEYS} = require('../../constants');
KEYS = Object.assign({}, KEYS, {
E: '65',
S: '73',
T: '74',
}promptController.put(KEYS.T);
|
This is kickass. |
|
OMG this is exactly what I was thinking of. Can we maybe have the name of the test file there as well? and if there is ambiguity (two files with the same name), do: what do you think? We have a way to shorten path names, we can simply re-use it. |
packages/jest-cli/src/watch.js
Outdated
| ? chalk.dim(' \u203A Press ') + 'o' + chalk.dim(' to only run tests related to changed files.') | ||
| : null, | ||
| snapshotFailure | ||
| ? chalk.dim(' \u203A Press ') + 'u' + chalk.dim(' to update failing snapshots.') | ||
| : null, | ||
| chalk.dim(' \u203A Press ') + 'p' + chalk.dim(' to filter by a filename regex pattern.'), | ||
| chalk.dim(' \u203A Press ') + 't' + chalk.dim(' to filter by a testname regex pattern.'), |
There was a problem hiding this comment.
can we do "test name regex pattern"?
packages/jest-cli/src/watch.js
Outdated
| @@ -167,21 +150,54 @@ const watch = ( | |||
| startRun({updateSnapshot: true}); | |||
| break; | |||
| case KEYS.A: | |||
| setWatchMode(argv, 'watchAll'); | |||
| setWatchMode(argv, 'watchAll', { | |||
There was a problem hiding this comment.
Shall we rename this function to "setWatchState", or given that it is inside of "watch.js", simply call it "setState"? I think that makes more sense.
|
@cpojer it's a good suggestion, but what it will look like if a single file has many of tests? Filenames occupy a lot of space, may be we can group test names by filename? |
|
Hmm.. yeah you are right. How does this feel:
? |
packages/jest-cli/src/watch.js
Outdated
| pipe.write(ansiEscapes.clearScreen); | ||
| pipe.write(usage(argv, hasSnapshotFailure)); | ||
| pipe.write(ansiEscapes.cursorShow); | ||
| }, |
There was a problem hiding this comment.
can we factor this function out? We have some duplication here.
packages/jest-cli/src/watch.js
Outdated
| testPathPattern: argv.testPathPattern | ||
| || argv._ instanceof Array | ||
| ? argv._.join('|') | ||
| : '', |
There was a problem hiding this comment.
nit, code style:
testPathPattern: argv.testPathPattern || Array.isArray(argv._)
? argv._.join('|')
: '',
do you also want to check for .length of the array? I guess it produces the same output here.
Don't use instanceof Array, Array.isArray is much safer.
| @@ -14,7 +14,7 @@ const buildTestPathPatternInfo = require('./buildTestPathPatternInfo'); | |||
| const setWatchMode = ( | |||
There was a problem hiding this comment.
As said, let's rename this file to setState.
| @@ -14,7 +14,7 @@ const buildTestPathPatternInfo = require('./buildTestPathPatternInfo'); | |||
| const setWatchMode = ( | |||
| argv: Object, | |||
| mode: 'watch' | 'watchAll', | |||
| options?: Object, | |||
| options?: Object = {}, | |||
There was a problem hiding this comment.
this won't work on all versions of node 4, let's not use default args please :)
| expect(onSuccess).toHaveBeenCalledWith('test'); | ||
| }); | ||
|
|
||
| it('should calls handler on cancel prompt', () => { |
| this.value.slice(0, -1) | ||
| : | ||
| this.value + char | ||
| ; |
There was a problem hiding this comment.
code style:
this.value = key === KEYS.BACKSPACE
? this.value.slice(0, -1)
: this.value + char;
| } | ||
|
|
||
| put( | ||
| key: string |
|
|
||
| const {KEYS} = require('../constants'); | ||
|
|
||
| class PromptController { |
There was a problem hiding this comment.
I really don't like the word "Controller" in code. Can we simply call this "Prompt" here and everywhere else?
| const {KEYS} = require('../constants'); | ||
|
|
||
| class PromptController { | ||
| entering: boolean; |
There was a problem hiding this comment.
Let's call this "_isEntering". Please add a _ to all fields that are supposed to be private.
| onSuccess: Function; | ||
| onCancel: Function; | ||
|
|
||
| prompt( |
|
@cpojer ready for review |
| const colorize = require('./colorize'); | ||
|
|
||
| const DOTS = '...'; | ||
| const ENTER = '⏎'; |
There was a problem hiding this comment.
Do we also need to check for stuff like \n or \r\n?
There was a problem hiding this comment.
and \r 🙄 I've fixed it.
|
I'm seeing an issue where the the pattern matches tests but they don't execute
|
|
I'm not sure why but I'm getting a
|
|
@rogeliog I've fixed the first issue and reproduced the second issue, but it's bug from the master branch, not this PR. I'm trying to fix it, but I think we can do it in another PR. |
|
This is awesome work @zouxuoz. There are definitely ways we can make this even better but I think it is ready to be merged. We can do small follow-ups to make this really solid – looking forward to more of your contributions. If you don't mind, please send me your email address: cpojer@fb.com so I can invite you to our jest-core chat. |
…ern (jestjs#2568) * Separate prompt patterns logic and watch mode logic * Separate pathname pattern mode logic and watch mode * Using testPathPattern for path pattern mode instead of _ args * Add initial testname pattern mode * Cache testnames for pattern mode * fix direct exporting * fix re-bind for on change handler * fix keys constants in tests * fix testname whitespace * rename setWatchMode to setState * refactor cancel pattern mode function * fix code style * rid of default params in functions * rid of should in tests * fix code style * fix comma * rename prompt files * fix private fields * rename prompt to enter * fix comma * fix comma and default param * fix code style * fix testname whitespace * refactor cache test names * Add tests for test name pattern mode * Add prefix with test file path for test name pattern mode * Move colorize to separate file * Update prompt on resize terminal * Better output for test name pattern mode * Ignore case for testNamePattern * Replace all types of the line breaks for test names in watch pattern mode
…ern (jestjs#2568) * Separate prompt patterns logic and watch mode logic * Separate pathname pattern mode logic and watch mode * Using testPathPattern for path pattern mode instead of _ args * Add initial testname pattern mode * Cache testnames for pattern mode * fix direct exporting * fix re-bind for on change handler * fix keys constants in tests * fix testname whitespace * rename setWatchMode to setState * refactor cancel pattern mode function * fix code style * rid of default params in functions * rid of should in tests * fix code style * fix comma * rename prompt files * fix private fields * rename prompt to enter * fix comma * fix comma and default param * fix code style * fix testname whitespace * refactor cache test names * Add tests for test name pattern mode * Add prefix with test file path for test name pattern mode * Move colorize to separate file * Update prompt on resize terminal * Better output for test name pattern mode * Ignore case for testNamePattern * Replace all types of the line breaks for test names in watch pattern mode
|
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. |
Summary
Resolve #1860 #2546
Tasks
args.testPathPatterninstead of 'args._' in watch mode;args.testNamePatternin watch mode;Test plan
Works on local machine and added unit tests are passed.
Preview