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 snapshot assertion #1113

Merged
merged 21 commits into from Dec 2, 2016

Conversation

Projects
None yet
5 participants
@lithin
Contributor

lithin commented Nov 17, 2016

This PR introduces snapshot testing into ava, using the jest-snapshost package that has been recently rewritten to allow usage in other runners than Jest. The reason behind it is that in our new project, we'd love to use Ava as it's absolutely brilliant. However, snapshot testing is a requirement for our project, and so we need to introduce it in one way or another.

  • Introduce jest snaphost in a spike to test viability
  • Allow updating of snapshots
  • Produce filename and snapshot path dynamically
  • Enhance reporting
  • Unit testing
  • Write documentation
  • Hook up code coverage
  • Check this works with con-current tests
  • Use correct test name for snapshots
  • CRITICAL DEPENDENCY: submit pull request to jest-snapshots for minor changes that made this PR easier (see facebook/jest#2130) - uses npm link locally atm I removed the new version for jest snapshots as a dependency for now - the owner is currently on holiday for a week :x
  • Check what "expand" option in snapshot state does Turns out it expands the diff which we want by default
  • Enable making two snapshots in one test
  • Add snapshots into test/assertion planning

All ideas, suggestions, comments are very welcome!

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Nov 18, 2016

Member

👍 Makes sense to me. Lots of users need this.


Context: https://facebook.github.io/jest/blog/2016/07/27/jest-14.html

Member

sindresorhus commented Nov 18, 2016

👍 Makes sense to me. Lots of users need this.


Context: https://facebook.github.io/jest/blog/2016/07/27/jest-14.html

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Nov 18, 2016

Member

Produce filename and snapshot path dynamically

I would look at how Jest handles this.

Enhance reporting

@vdemedes is working on a new output which includes diffing, so this could simply use t.is() comparison of the snapshot, and diffing will be supported soon.

Member

sindresorhus commented Nov 18, 2016

Produce filename and snapshot path dynamically

I would look at how Jest handles this.

Enhance reporting

@vdemedes is working on a new output which includes diffing, so this could simply use t.is() comparison of the snapshot, and diffing will be supported soon.

@sindresorhus sindresorhus changed the title from Introduce jest snapshot integration to Add snapshot assertion Nov 18, 2016

@lithin

This comment has been minimized.

Show comment
Hide comment
@lithin

lithin Nov 18, 2016

Contributor

@sindresorhus thanks for such quick feedback!

Filepath & directory

I totally agree that we should look at how jest handles this sort of stuff. Is there any way in Ava to see what filename/path the current test is in?

Reporting

As for reporting, Jest snapshots generate a message for you:

screen shot 2016-11-18 at 9 00 31 am

I'm not sure how easy it would be to change this?

Snapshots directory

At the moment, Jest saves snapshots into the same folder as the current test is in. We were wondering with my colleagues if it wouldn't be better to create a __snapshots__ folder in the root folder. What do you think?

Contributor

lithin commented Nov 18, 2016

@sindresorhus thanks for such quick feedback!

Filepath & directory

I totally agree that we should look at how jest handles this sort of stuff. Is there any way in Ava to see what filename/path the current test is in?

Reporting

As for reporting, Jest snapshots generate a message for you:

screen shot 2016-11-18 at 9 00 31 am

I'm not sure how easy it would be to change this?

Snapshots directory

At the moment, Jest saves snapshots into the same folder as the current test is in. We were wondering with my colleagues if it wouldn't be better to create a __snapshots__ folder in the root folder. What do you think?

@lithin

This comment has been minimized.

Show comment
Hide comment
@lithin

lithin Nov 18, 2016

Contributor

Here are a few updates on my progress:

  • We decided to follow jest and have __snapshots__ directory next to the test files
  • Checked code coverage with nyc works as expected
  • Reporting has been slightly improved - please check the screenshot

screen shot 2016-11-18 at 2 16 55 pm

Contributor

lithin commented Nov 18, 2016

Here are a few updates on my progress:

  • We decided to follow jest and have __snapshots__ directory next to the test files
  • Checked code coverage with nyc works as expected
  • Reporting has been slightly improved - please check the screenshot

screen shot 2016-11-18 at 2 16 55 pm

@lithin

This comment has been minimized.

Show comment
Hide comment
@lithin

lithin Nov 21, 2016

Contributor

@sindresorhus I think I've finished - could you please review?

Contributor

lithin commented Nov 21, 2016

@sindresorhus I think I've finished - could you please review?

@lithin

This comment has been minimized.

Show comment
Hide comment
@lithin

lithin Nov 21, 2016

Contributor

Builds are failing because jest-snapshot is using const in build files which isn't compatible with node 0.12. Is there any chance of upgrading node on travis?

Contributor

lithin commented Nov 21, 2016

Builds are failing because jest-snapshot is using const in build files which isn't compatible with node 0.12. Is there any chance of upgrading node on travis?

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Nov 21, 2016

Member

Builds are failing because jest-snapshot is using const in build files which isn't compatible with node 0.12. Is there any chance of upgrading node on travis?

Just ignore that for now. We're dropping Node.js 0.12 support very soon.

Member

sindresorhus commented Nov 21, 2016

Builds are failing because jest-snapshot is using const in build files which isn't compatible with node 0.12. Is there any chance of upgrading node on travis?

Just ignore that for now. We're dropping Node.js 0.12 support very soon.

@lithin

This comment has been minimized.

Show comment
Hide comment
@lithin

lithin Nov 22, 2016

Contributor

@sindresorhus Does that mean we need to wait for the upgrade to be made?

We just caught one bug so I did a quick fix :) It's again ready for review.

Contributor

lithin commented Nov 22, 2016

@sindresorhus Does that mean we need to wait for the upgrade to be made?

We just caught one bug so I did a quick fix :) It's again ready for review.

Show outdated Hide outdated lib/snapshot-state.js
Show outdated Hide outdated lib/snapshot-state.js
Show outdated Hide outdated test/snapshot-state.js
@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Nov 22, 2016

Member

Does that mean we need to wait for the upgrade to be made?

No. I just upgraded a big part of the code base, but made sure not to touch anything changed by this pull request. Feel free to start using ES2015 features (for your changes only). Otherwise I'll do it after merge, no problem.

Member

sindresorhus commented Nov 22, 2016

Does that mean we need to wait for the upgrade to be made?

No. I just upgraded a big part of the code base, but made sure not to touch anything changed by this pull request. Feel free to start using ES2015 features (for your changes only). Otherwise I'll do it after merge, no problem.

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Nov 22, 2016

Member

@lithin This looks very good :) I'll play with it some more tomorrow and do a more thorough review.

Does snapshot testing only work with JSX or can you use it with other things too?

You also need to add an entry to the Assertion section: https://github.com/avajs/ava#assertions

Can you fix the lint issues reported by Travis: https://travis-ci.org/avajs/ava/jobs/177985206 ?

Member

sindresorhus commented Nov 22, 2016

@lithin This looks very good :) I'll play with it some more tomorrow and do a more thorough review.

Does snapshot testing only work with JSX or can you use it with other things too?

You also need to add an entry to the Assertion section: https://github.com/avajs/ava#assertions

Can you fix the lint issues reported by Travis: https://travis-ci.org/avajs/ava/jobs/177985206 ?

Show outdated Hide outdated lib/cli.js
@@ -0,0 +1,27 @@
var jestSnapshot = require('jest-snapshot');

This comment has been minimized.

@sindresorhus

sindresorhus Nov 22, 2016

Member

Put a 'use strict'; at the top, above this.

@sindresorhus

sindresorhus Nov 22, 2016

Member

Put a 'use strict'; at the top, above this.

This comment has been minimized.

@lithin

lithin Nov 23, 2016

Contributor

👍

@lithin

lithin Nov 23, 2016

Contributor

👍

Show outdated Hide outdated readme.md
Show outdated Hide outdated readme.md
The first time you run this test, a snapshot file will be created in `__snapshots__` folder looking something like this:
```

This comment has been minimized.

@sindresorhus

sindresorhus Nov 22, 2016

Member
```js
@sindresorhus

This comment has been minimized.

@lithin

lithin Nov 23, 2016

Contributor

This one isn't js actually - it's just a snap file

@lithin

lithin Nov 23, 2016

Contributor

This one isn't js actually - it's just a snap file

Show outdated Hide outdated readme.md
var x = module.exports;
x.get = function (initializeState, globalsOptions) {

This comment has been minimized.

@sindresorhus

sindresorhus Nov 22, 2016

Member

x.get => exports.get and drop the module.exports

@sindresorhus

sindresorhus Nov 22, 2016

Member

x.get => exports.get and drop the module.exports

This comment has been minimized.

@lithin

lithin Nov 24, 2016

Contributor

Had to return this back to make it work with .state

@lithin

lithin Nov 24, 2016

Contributor

Had to return this back to make it work with .state

Show outdated Hide outdated lib/assert.js
Show outdated Hide outdated readme.md
Show outdated Hide outdated test/assert.js
Show outdated Hide outdated test/assert.js
Show outdated Hide outdated test/assert.js
@@ -0,0 +1,45 @@
var test = require('tap').test;

This comment has been minimized.

@vadimdemedes

vadimdemedes Nov 22, 2016

Member

'use strict'; is missing here.

@vadimdemedes

vadimdemedes Nov 22, 2016

Member

'use strict'; is missing here.

This comment has been minimized.

@lithin

lithin Nov 23, 2016

Contributor

👍

@lithin

lithin Nov 23, 2016

Contributor

👍

@vadimdemedes

This comment has been minimized.

Show comment
Hide comment
@vadimdemedes

vadimdemedes Nov 22, 2016

Member

Really well done, @lithin! Nicely organized and clean PR, thanks a lot for working on this!

Member

vadimdemedes commented Nov 22, 2016

Really well done, @lithin! Nicely organized and clean PR, thanks a lot for working on this!

Show outdated Hide outdated readme.md
@lithin

Does snapshot testing only work with JSX or can you use it with other things too?

Yes - I added it to the readme

Show outdated Hide outdated lib/assert.js
@@ -0,0 +1,27 @@
var jestSnapshot = require('jest-snapshot');

This comment has been minimized.

@lithin

lithin Nov 23, 2016

Contributor

👍

@lithin

lithin Nov 23, 2016

Contributor

👍

The first time you run this test, a snapshot file will be created in `__snapshots__` folder looking something like this:
```

This comment has been minimized.

@lithin

lithin Nov 23, 2016

Contributor

This one isn't js actually - it's just a snap file

@lithin

lithin Nov 23, 2016

Contributor

This one isn't js actually - it's just a snap file

@@ -0,0 +1,45 @@
var test = require('tap').test;

This comment has been minimized.

@lithin

lithin Nov 23, 2016

Contributor

👍

@lithin

lithin Nov 23, 2016

Contributor

👍

@lithin

This comment has been minimized.

Show comment
Hide comment
@lithin

lithin Nov 23, 2016

Contributor

Thanks @vdemedes and @sindresorhus for the feedback - hopefully I didn't miss anything :)

Contributor

lithin commented Nov 23, 2016

Thanks @vdemedes and @sindresorhus for the feedback - hopefully I didn't miss anything :)

@lithin

This comment has been minimized.

Show comment
Hide comment
@lithin

lithin Nov 24, 2016

Contributor

@sindresorhus it seems one of my comments got lost in all the changes - how do you propose to use the x.snapshot vs x._snapshot? I wasn't quite sure how to implement that.

Contributor

lithin commented Nov 24, 2016

@sindresorhus it seems one of my comments got lost in all the changes - how do you propose to use the x.snapshot vs x._snapshot? I wasn't quite sure how to implement that.

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Nov 24, 2016

Member

I was thinking something like this:

x._snapshot = function (tree, optionalMessage, match, snapshotStateGetter) {
	// actual implementation here
};

x.snapshot = function (tree, optionalMessage) {
	x._snapshot(tree, optionalMessage);
};

That way the user (which uses x.snapshot) can never accidentally supply those internal arguments, and we can still easily test it using x._snapshot.

Member

sindresorhus commented Nov 24, 2016

I was thinking something like this:

x._snapshot = function (tree, optionalMessage, match, snapshotStateGetter) {
	// actual implementation here
};

x.snapshot = function (tree, optionalMessage) {
	x._snapshot(tree, optionalMessage);
};

That way the user (which uses x.snapshot) can never accidentally supply those internal arguments, and we can still easily test it using x._snapshot.

Show outdated Hide outdated lib/enhance-assert.js
@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Nov 25, 2016

Member

I noticed if create a snapshot, rename the test, and run ava -u, the previous test is still in the snapshot file. Is this intentional?


I'm not getting the colorized diff output like you have:

screen shot 2016-11-25 at 14 15 23


The generated output should also be indented so it's vertically aligned with the test title in the output. You can use indent-string for this.

Member

sindresorhus commented Nov 25, 2016

I noticed if create a snapshot, rename the test, and run ava -u, the previous test is still in the snapshot file. Is this intentional?


I'm not getting the colorized diff output like you have:

screen shot 2016-11-25 at 14 15 23


The generated output should also be indented so it's vertically aligned with the test title in the output. You can use indent-string for this.

Show outdated Hide outdated lib/assert.js
@lithin

This comment has been minimized.

Show comment
Hide comment
@lithin

lithin Nov 28, 2016

Contributor

@sindresorhus All fixed :)

As for the report output, I'm not sure what happened there :x I found out that no matter how I use chalk from within jest or ava, it wouldn't colour anything. I thought it might be cause by the reporter but now I realised it used to work... Very confusing! Any ideas what this might be caused by?

I haven't seen anywhere that the test would appear twice after updating the snapshot. It could happen if you've changed the name of the test though?

Contributor

lithin commented Nov 28, 2016

@sindresorhus All fixed :)

As for the report output, I'm not sure what happened there :x I found out that no matter how I use chalk from within jest or ava, it wouldn't colour anything. I thought it might be cause by the reporter but now I realised it used to work... Very confusing! Any ideas what this might be caused by?

I haven't seen anywhere that the test would appear twice after updating the snapshot. It could happen if you've changed the name of the test though?

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Nov 29, 2016

Member

The indentation is not correct. As commented earlier, it should be vertically aligned with the test title. So you need to add 2 space indent. It seems the first line in the output is already indented by the assertion output, so we just indent the following text. Like this:

var message = 'Please check your code or --update-snapshots\n\n';

if (optionalMessage) {
	message += indentString(optionalMessage, 2);
}

if (typeof result.message === 'function') {
	message += indentString(result.message(), 2);
}

state.save();

test(result.pass, create(result, false, 'snapshot', message, x.snap));
Member

sindresorhus commented Nov 29, 2016

The indentation is not correct. As commented earlier, it should be vertically aligned with the test title. So you need to add 2 space indent. It seems the first line in the output is already indented by the assertion output, so we just indent the following text. Like this:

var message = 'Please check your code or --update-snapshots\n\n';

if (optionalMessage) {
	message += indentString(optionalMessage, 2);
}

if (typeof result.message === 'function') {
	message += indentString(result.message(), 2);
}

state.save();

test(result.pass, create(result, false, 'snapshot', message, x.snap));
@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Nov 29, 2016

Member

Any ideas what this might be caused by?

Yes. The error is colored red by our reporter. I'm more confused by how you actually got a diff color there in the first place. But we can ignore it for this pull request, as we're very close to a rewrite of the reporter output, where this can be handled.

Member

sindresorhus commented Nov 29, 2016

Any ideas what this might be caused by?

Yes. The error is colored red by our reporter. I'm more confused by how you actually got a diff color there in the first place. But we can ignore it for this pull request, as we're very close to a rewrite of the reporter output, where this can be handled.

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Nov 29, 2016

Member

I haven't seen anywhere that the test would appear twice after updating the snapshot. It could happen if you've changed the name of the test though?

Yes, that's what I wrote. I renamed the test and then ran ava -u. Shouldn't that remove old tests?

Member

sindresorhus commented Nov 29, 2016

I haven't seen anywhere that the test would appear twice after updating the snapshot. It could happen if you've changed the name of the test though?

Yes, that's what I wrote. I renamed the test and then ran ava -u. Shouldn't that remove old tests?

@lithin

This comment has been minimized.

Show comment
Hide comment
@lithin

lithin Nov 29, 2016

Contributor

The error is colored red by our reporter. I'm more confused by how you actually got a diff color there in the first place.

I'm really confused about that as well. The colouring coming from jest diff used to work fine. Now, even if I remove the test(... message ...) in the snapshot assertion and just do console.log(message), the message isn't coloured.

I agree it shouldn't be in scope of this pull request though.

Shouldn't that remove old tests?

Jest identifies tests by their name - if you rename a test, it's as if you were running a new one. It has no way of telling if a test of another name should be identified with the renamed one. In other words, I don't think it can remove old tests from snapshots - it has to be done manually.

Contributor

lithin commented Nov 29, 2016

The error is colored red by our reporter. I'm more confused by how you actually got a diff color there in the first place.

I'm really confused about that as well. The colouring coming from jest diff used to work fine. Now, even if I remove the test(... message ...) in the snapshot assertion and just do console.log(message), the message isn't coloured.

I agree it shouldn't be in scope of this pull request though.

Shouldn't that remove old tests?

Jest identifies tests by their name - if you rename a test, it's as if you were running a new one. It has no way of telling if a test of another name should be identified with the renamed one. In other words, I don't think it can remove old tests from snapshots - it has to be done manually.

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Nov 29, 2016

Member

Jest identifies tests by their name - if you rename a test, it's as if you were running a new one. It has no way of telling if a test of another name should be identified with the renamed one. In other words, I don't think it can remove old tests from snapshots - it has to be done manually.

Yes, that's desirable between runs, but when running $ ava --update-snapshots, I expected it to update the snapshot to the reflect the current state of the tests file, removing non-existing/renamed tests. Is there any benefit to keeping non-existing tests around after updating the snapshot? I don't really see it, as we already have git, but I might be missing something fundamental about how it works.

Member

sindresorhus commented Nov 29, 2016

Jest identifies tests by their name - if you rename a test, it's as if you were running a new one. It has no way of telling if a test of another name should be identified with the renamed one. In other words, I don't think it can remove old tests from snapshots - it has to be done manually.

Yes, that's desirable between runs, but when running $ ava --update-snapshots, I expected it to update the snapshot to the reflect the current state of the tests file, removing non-existing/renamed tests. Is there any benefit to keeping non-existing tests around after updating the snapshot? I don't really see it, as we already have git, but I might be missing something fundamental about how it works.

@lithin

This comment has been minimized.

Show comment
Hide comment
@lithin

lithin Nov 30, 2016

Contributor

@sindresorhus I think you're absolutely right :) Might be a good suggestion to the jest repo but I'm afraid I can't change that through my implementation of it.

Contributor

lithin commented Nov 30, 2016

@sindresorhus I think you're absolutely right :) Might be a good suggestion to the jest repo but I'm afraid I can't change that through my implementation of it.

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Dec 1, 2016

Member

@lithin Alright. I'll open an issue on Jest when I have some time :) This looks ready to me, but the tests are failing on Windows: https://ci.appveyor.com/project/ava/ava/build/1.0.462/job/jnx6609ubg58gfcq Mind taking a look?

Member

sindresorhus commented Dec 1, 2016

@lithin Alright. I'll open an issue on Jest when I have some time :) This looks ready to me, but the tests are failing on Windows: https://ci.appveyor.com/project/ava/ava/build/1.0.462/job/jnx6609ubg58gfcq Mind taking a look?

@lithin

This comment has been minimized.

Show comment
Hide comment
@lithin

lithin Dec 2, 2016

Contributor

Tests passed! :)

Contributor

lithin commented Dec 2, 2016

Tests passed! :)

It's awesome now

@sindresorhus sindresorhus merged commit ee65b6d into avajs:master Dec 2, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.06%) to 96.93%
Details
@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Dec 2, 2016

Member

Yay! Super excited about this. Thank you so much for working hard on this @lithin. We really appreciate it. 🎉🦄🚀🌸

Member

sindresorhus commented Dec 2, 2016

Yay! Super excited about this. Thank you so much for working hard on this @lithin. We really appreciate it. 🎉🦄🚀🌸

sindresorhus added a commit that referenced this pull request Dec 2, 2016

@lpil

This comment has been minimized.

Show comment
Hide comment
@lpil

lpil Dec 2, 2016

Great work, thanks @lithin & @sindresorhus

lpil commented Dec 2, 2016

Great work, thanks @lithin & @sindresorhus

@vadimdemedes

This comment has been minimized.

Show comment
Hide comment
@vadimdemedes

vadimdemedes Dec 3, 2016

Member

This is amazing, thank you, Anna!

giphy 8

P.S. I post this gif only on really cool stuff :)

Member

vadimdemedes commented Dec 3, 2016

This is amazing, thank you, Anna!

giphy 8

P.S. I post this gif only on really cool stuff :)

@lithin

This comment has been minimized.

Show comment
Hide comment
@lithin

lithin Dec 5, 2016

Contributor

Hahaha thank you @vadimdemedes ! It was a pleasure to contribute to a project like Ava :)

@sindresorhus there are definitely things to improve so I might make a few more pull requests if I get around working on them ;)

Contributor

lithin commented Dec 5, 2016

Hahaha thank you @vadimdemedes ! It was a pleasure to contribute to a project like Ava :)

@sindresorhus there are definitely things to improve so I might make a few more pull requests if I get around working on them ;)

@novemberborn novemberborn referenced this pull request Feb 17, 2017

Closed

Snapshot recap #1275

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment