Ava: Snapshot feature will modify existing snapshots without passing `--update-snapshots` flag

[wyze]

Description

The results with snapshots are inconsistent when ran multiple times with no code changes. I've provided a repository with an example below.

Upon cloning the repo and running npm test it will pass the tests but leave the snapshots modified (See output of 2nd pass below). If you run tests again, it will throw an error (See 3rd pass output below).

For what it's worth, I was using the master branch after the snapshot feature first landed and had no issues. It seems like the bug was introduced when the magic assert feature was introduced. I even tried deleting the snapshot folder and getting a fresh run, but it still produces the same results.

Error Message & Stack Trace

# 2nd pass
> ava

  4 passed

~/Projects/ava-snapshot-issue master*

# 3rd pass
> ava

  3 passed
  1 failed

  renders custom width
  test.js:9

   8:
   9:   t.snapshot(tree)
   10: }

  Error: Unexpected token m in JSON at position 28

  Test.snapshot (test.js:9:5)

npm ERR! Test failed.  See above for more details.

Config

Copy the relevant section from package.json:

{
  "ava": {
    "babel": "inherit",
    "require": "babel-register"
  }
}

Command-Line Arguments

Copy your npm build scripts or the ava command used:

npm test

Relevant Links

See reproducable repo here: https://github.com/wyze/ava-snapshot-issue

Environment

Node.js v7.1.0
darwin 16.1.0
npm 4.0.3
AVA 0.18

[vadimdemedes]

Thank you for a detailed bug report, I just reproduced it and looking into it.

[wyze]

I think the problem lies with JSON.stringifying the serializedTree here: lib/assert#L171

If I remove the JSON.stringify and make it just const result = toMatchSnapshot.call(context, serializedTree);, it works every time.

[vadimdemedes]

Hm yes, this makes the issue gone indeed. I've been debugging this issue inside jest-snapshot, and the problem lies in saving snapshot file. For some reason, on the second run, it reserializes/reencodes the last item in the snapshot.

[wyze]

I tried to see if for some reason the macro was causing the problem, and broke out the tests into long form, and it still produced the same result.

Then, I wanted to check to see if it was jest-snapshot, so I pushed a jest branch up and it does not result in the behavior described above.

You can see the jest branch here if needed: https://github.com/wyze/ava-snapshot-issue/tree/jest

[vadimdemedes]

Found this issue, seems to be related, from what I've observed - https://github.com/facebook/jest/issues/2478.

If you keep a snapshot open in the editor, you'll see how it gets modified on the 2nd run.

[wyze]

Cool, thanks for digging in. It looks like this was fixed with https://github.com/facebook/jest/pull/2482 and we just need to wait for the next release to be cut.

[vadimdemedes]

Just opened https://github.com/avajs/ava/pull/1223 - a dead-simple snapshot implementation, I hope it will eliminate such bugs and you can use t.snapshot with almost any kind of input ;)

[vadimdemedes]

By the way, I would really appreciate it, if you could try this branch on your project and see if it solves existing/introduces new issues, if you've got some free time. No worries if not, though!

[wyze]

Looks great @vadimdemedes, thanks! I pushed a new branch, wyze/ava-snapshot-issue#ava-1223, in the repo that works without issue now.

I've also tested this in my real project and it works as well. But it does create snapshots, even if there is no snapshot assertion in that test file. So, in the project with more tests, there are __snapshots__/index.js.snap that just contain {}.

[vadimdemedes]

This is a good catch, will fix. Thank you a lot for taking the time to test it out!

[pho3nixf1re]

Is there any chance of getting a 'quick fix' for this while y'all pull together #1223 ? The current state makes snapshots unusable. Is the issue that jest-snapshot is still waiting for the fix to be merged?

[novemberborn]

Is the issue that jest-snapshot is still waiting for the fix to be merged?

@pho3nixf1re It's merged, but not released.

[novemberborn]

Actually, the jest-snapshot bug is exacerbated by AVA's implementation.

jest-snapshot always modifies the internal snapshot data when an assertion passes.

We're always saving the snapshot. This should be fine, because jest-snapshot tracks whether the existing snapshot is dirty, and those flags are not modified unless we tell it we're updating.

However, as it turns out we're saving snapshots from inside the t.snapshot() assertion. This means there are unchecked keys in the snapshot, and the save actually completes.

Instead we should save the snapshots after all tests in the test file have completed, but before the worker exits.

[novemberborn]

The reason we should save even without --update-snapshots is that this allows new snapshots to be automatically saved.

[CrOrc]

Actually, we should not save snapshot if we are running within CI server.
E.g. snapshots should be tested against data committed to source control.