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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add command for updating snapshots in watch mode #1413

Merged
merged 4 commits into from
Jun 25, 2017
Merged

Add command for updating snapshots in watch mode #1413

merged 4 commits into from
Jun 25, 2017

Conversation

efegurkan
Copy link
Contributor

@efegurkan efegurkan commented Jun 12, 2017

This is my humble implementation for pressing 'u' in watch mode to update snapshots.

fixes #1387

lib/watcher.js Outdated
@@ -79,7 +79,7 @@ class Watcher {

this.clearLogOnNextRun = true;
this.runVector = 0;
this.run = specificFiles => {
this.run = (specificFiles, options = {}) => {
Copy link
Member

Choose a reason for hiding this comment

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

You can't use default arguments. We still support Node.js 4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that explains the failing tests indeed 👍

test/watcher.js Outdated
@@ -212,7 +212,7 @@ group('chokidar', (beforeEach, test, group) => {
t.ok(logger.reset.notCalled);
t.ok(logger.start.notCalled);
t.ok(api.run.calledOnce);
t.strictDeepEqual(api.run.firstCall.args, [files, {runOnlyExclusive: false}]);
t.strictDeepEqual(api.run.firstCall.args, [files, {runOnlyExclusive: false, updateSnapshots: false}]);
Copy link
Member

Choose a reason for hiding this comment

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

I would extract {runOnlyExclusive: false, updateSnapshots: false} into a variable and use it in all these asserts. Would make it easier to add additional options in the future.

@sindresorhus
Copy link
Member

The feature makes sense to me :) @avajs/core Thoughts?

It also needs to be documented.

@sindresorhus sindresorhus changed the title [WIP] Add u for updating snapshots on watch mode [WIP] Add command for updating snapshots in watch mode Jun 13, 2017
@efegurkan efegurkan changed the title [WIP] Add command for updating snapshots in watch mode Add command for updating snapshots in watch mode Jun 15, 2017
Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

@efegurkan awesome, thanks!

Just a small typo, and I'm wondering whether this should rerun all tests or just the tests from the previous run.

@@ -91,6 +91,10 @@ If you run AVA in your CI with watch mode, the execution will exit with an error

You can quickly rerun all tests by typing <kbd>r</kbd> on the console, followed by <kbd>Enter</kbd>.

## Updating snapshots

You can update failing snapshots by typing<kbd>u</kbd> on the console, followed by <kbd>Enter</kbd>.
Copy link
Member

Choose a reason for hiding this comment

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

Needs a space between typing and <kbd>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops 🤷‍

lib/watcher.js Outdated
});
});
}
rerunAll() {
this.dirtyStates = {};
this.run();
}
rerunWithUpdate() {
this.dirtyStates = {};
this.run(undefined, {updateSnapshots: true});
Copy link
Member

Choose a reason for hiding this comment

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

Should this update snapshots in all tests, or just those tests that just ran? I'm leaning towards the latter. Do you know what Jest settled on?

@lukescott?

Choose a reason for hiding this comment

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

I believe it only updates snapshots of tests that were just ran.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for jest it updates only for running tests.

Copy link
Member

Choose a reason for hiding this comment

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

Let's do that here as well then. Storing the specificFiles || files argument here should be sufficient.

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

@efegurkan good work with this. I found two issues in the code (see comments below). Since we're including a major snapshot overhaul in the next release I really want to land this, so I took the liberty of addressing these issues and making u rerun the previous tests, rather than all tests. Will push those changes imminently.

(This will be a force-push since I had to rebase this on master.)

api.js Outdated
@@ -59,7 +59,7 @@ class Api extends EventEmitter {
const resolvedfpath = fs.realpathSync(file);
precompiled[resolvedfpath] = hash;

const options = Object.assign({}, this.options, {precompiled});
const options = Object.assign({}, this.options, {precompiled, updateSnapshots: runStatus.updateSnapshots});
Copy link
Member

Choose a reason for hiding this comment

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

This breaks the --update-snapshots option, since runStatus.updateSnapshots defaults to false.

test/watcher.js Outdated
@@ -89,7 +90,8 @@ group('chokidar', (beforeEach, test, group) => {
runStatus = {
failCount: 0,
rejectionCount: 0,
exceptionCount: 0
exceptionCount: 0,
updateSnapshots: 0
Copy link
Member

Choose a reason for hiding this comment

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

This should be false rather than 0 (it's not a counter).

@novemberborn novemberborn merged commit f507e36 into avajs:master Jun 25, 2017
novemberborn added a commit that referenced this pull request Jul 13, 2017
novemberborn added a commit that referenced this pull request Jul 13, 2017
kevva pushed a commit that referenced this pull request Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Press 'u' in watch mode to update snapshots
4 participants