Alternative for deepEqual, formatting, diffs and snapshots

[novemberborn]

Latest status: #1341 (comment)

---

This PR proposes we replace lodash.isequal, @ava/pretty-format, diff and jest-snapshot with one single library.

• With t.deepEqual(), if actual and expected values are not equal then the diff we present should actually show a difference. This currently isn't always the case because lodash.isequal checks properties that may not be formatted by @ava/pretty-format

• The diff resulting from t.deepEqual() is done using diff, which compares formatted values. This diff may be too large, or it's unclear where the shown diff is located in a large tree structure

• As discussed in Snapshot recap #1275 jest-snapshot presents a diff between formatted values. This has the same issues as with our current t.deepEqual() implementation, as well as preventing us from showing colors

• What counts as equality in snapshots is different from t.deepEqual() which could be surprising

lodash.isequal has some quirks too:

• Object(1) is equal to 1
• Non-index properties on arrays are ignored for the comparison
• An arguments object can be compared to an actual object, where I'd expect to compare it to an array
• Map and Set objects are equal even if their entries are in a different order

@ava/pretty-format and jest-snapshot have other issues, like not printing properties of most non-Object objects.

I've been working on concordance to tackle these issues in one library:

• Comparing, formatting and diffing operations all follow the same rules, meaning the output is consistent
• It'll be possible to serialize snapshots such that they're both human readable and encode the value, allowing for comparisons that are consistent with not using snapshots, and customizable formatting
• It'll be possible to write plugins that can compare say React trees or perhaps testdouble explanations

A lot more work needs to be done:

• There is no color support yet Support (color) themes concordancejs/concordance#7
• Formatting depth cannot be controlled (used when showing power-assert statements) Allow formatting depth to be controlled concordancejs/concordance#11
• We should be able to control how many properties / items are shown, which can be used when showing power-assert statements, and in diffs Allow number of properties / items shown to be controlled concordancejs/concordance#12
• Snapshots are not yet supported (and we'd have to fully replace jest-snapshot) Support snapshots concordancejs/concordance#9
• We'd need to support plugins, e.g. for React Support plugins concordancejs/concordance#10
• There's other bugs https://github.com/concordancejs/concordance/issues?q=is%3Aopen+is%3Aissue+label%3Abug

Regardless, I'm excited about the consistency and user experience this provides, and how it helps AVA be a more agnostic test runner.

If you're interested in contributing to this effort there are lots of opportunities to help out. Please come find me in our Gitter channel or consult the concordance issue list.

[novemberborn]

I've added serialization support to kathryn, and pushed a commit here which uses kathryn for snapshots.

Perhaps controversially, Kathryn serializes to a binary format. Consequently so would AVA with this implementation. I'm writing a second test-file.readable.snap file which contains a readable version that can be diffed using version control as a way of verifying the new snapshot. We could consider combining the binary data with the readable data to have a single file, but that would make it harder for us to parse the snapshot file, and for users to consume the version control diff.

To keep things simple, snapshots are only updated when --update-snapshots is passed. This means we don't automatically prune snapshots as tests change. We could consider warning the user when a snapshot is unused, though I'm not sure if it's worth it.

There are some to-do items left:

• We don't prune snapshot files even if --update-snapshots is passed (that is, the file exist, but the tests no longer uses snapshots)
• If a test is removed I don't think we can know what snapshot files to remove
• The directory is still __snapshots__. As discussed in [WIP] Replace jest-snapshot #1223 we'd want it to only be __snapshots__ if inside a __tests__ directory, otherwise snapshots
• The README hasn't yet been updated
• The formatted sections of the readable snapshot could be surrounded with code fences so the snapshot can be viewed as Markdown

@vadimdemedes @sindresorhus what do you think?

[sindresorhus]

This looks very promising!

---

Perhaps controversially, Kathryn serializes to a binary format.

Can you elaborate on why this is needed? I can see the benefit of having two files, as we can make the readable one even more readable. I'm just curious why binary.

[sindresorhus]

Just some nitpick:

      Object {
        foo: Object {
    -     bar: Date 2017-04-19T06:58:10.047Z,
    +     bar: Date 2017-04-19T06:58:30.166Z,
        },
      }

Since we control the output now. I don't like the trailing commas. And I think we should drop the type for Object and Array as their type is already known with {} and []. I also think we could make the date output nicer: 2017-04-19T06:58:30.166Z => 2017-04-19 06:58:30 166ms Z.

---

I think the readable snapshot should be the main one, so test.js.readable.snap => test.js.snap and we could do another one: test.js.binary.snap for the binary one.

[sindresorhus]

Can you document the binary format? Why not use something like Protocol Buffers?

My biggest concerns with a binary format is debuggability and it not being diffable in git, so each snapshot update will take the whole size of the snapshot. This can have big impact of projects with lots of large snapshots.

[sindresorhus]

So let's go with 16 bit from the start then? Then we don't have to document such limitation and never have to worry about it.

[sindresorhus]

Nitpick: Use template literals

[novemberborn]

Perhaps controversially, Kathryn serializes to a binary format.

Can you elaborate on why this is needed

It needs to serialize to some intermediate format. It can't really be JSON, because of dates, buffers, and certain number serializations.

Can you document the binary format?

Yes, eventually. I'm still proving out the feasibility of it all.

Why not use something like Protocol Buffers?

I can look into that. There isn't a lot to the current encoding though (at least in AVA).

My biggest concerns with a binary format is debuggability and it not being diffable in git, so each snapshot update will take the whole size of the snapshot. This can have big impact of projects with lots of large snapshots.

At least the snapshot files correspond to test files. I don't know how outrageously large they would get in practice.

I don't think there is much value in the serialization being readable. Even if it's JSON it wouldn't be pretty formatted, and a single line diff is just as useless as a binary diff. If we do pretty format it would tempt people to make changes, and that's likely to break the snapshot. With the binary format we discourage all that and we get to use compression. I think that strikes the right balance.

I think the readable snapshot should be the main one, so test.js.readable.snap => test.js.snap and we could do another one: test.js.binary.snap for the binary one.

Maybe, yea. The readable snapshot isn't actually used though, it's just there so you can verify changes.

---

Since we control the output now. I don't like the trailing commas

For the last property you mean? It'd be a bit more work to track which item / property / map entry is the last one, flag it, and then prevent the comma. Though it would be possible.

Thing is, this output isn't necessarily JavaScript. It just looks a lot like it. I like the consistency of always ending a property.

I think we should drop the type for Object and Array as their type is already known with {} and [].

[] are determined by the presence of an integer-value .length property. We could make an exception for true Objects and Arrays but I'm not convinced it's worthwhile.

I also think we could make the date output nicer: 2017-04-19T06:58:30.166Z => 2017-04-19 06:58:30 166ms Z.

Sure. concordancejs/concordance#15

---

I'm hoping to land theme support today, and I'm also doing a pass through this PR to revisit the assert integration and snapshot implementation.

[sindresorhus]

I can look into that. There isn't a lot to the current encoding though (at least in AVA).

With Protocol Buffers we don't really have to care about the binary part. We just define the schema and automatically get a encoder/decoder. Instead of how we have lots of custom code to encode/decode now.

We could make an exception for true Objects and Arrays but I'm not convinced it's worthwhile.

True Objects and Arrays are the most common output, and simplifying that simplifies the 95%. I think it's very much worth it.

I don't think there is much value in the serialization being readable. Even if it's JSON it wouldn't be pretty formatted, and a single line diff is just as useless as a binary diff. If we do pretty format it would tempt people to make changes, and that's likely to break the snapshot. With the binary format we discourage all that and we get to use compression. I think that strikes the right balance.

Good point. I'm warming up to the idea.

For the last property you mean? It'd be a bit more work to track which item / property / map entry is the last one, flag it, and then prevent the comma. Though it would be possible.

Yes. That's how JS is usually presented, like with util.inspect(). It's also how most JS is written. Having a trailing comma is distracting and makes the output more noisy. Same reason I'd like the Object/Array type names gone.

[novemberborn]

True Objects and Arrays are the most common output, and simplifying that simplifies the 95%. I think it's very much worth it.

concordancejs/concordance#17

Having a trailing comma is distracting and makes the output more noisy.

concordancejs/concordance#18

---

I've forced-pushed some updates:

• Actual / expected / difference values are no longer indented in the logger output
• More direct usage of kathryn (more changes coming with theming)
• I'm now generating a "Snapshot report" in Markdown format. This is the readable snapshot output. It even includes the assertion message (t.snapshot(obj, 'this message here')).

Lastly there is a commit that switches to protocol buffers. It uses https://www.npmjs.com/package/protobufjs/. I'm vendoring a minimal implementation to avoid users having to install the full package, which seems to come to 13MB!

[sindresorhus]

The snapshot version should be defined in the schema too.

[novemberborn]

Why?

[sindresorhus]

I was thinking so we could warn about different versions, but seems like the best practise is not to version them, so I guess having it in the Snapshot field makes more sense: http://stackoverflow.com/questions/8519381/how-does-protocol-buffer-handle-versioning

[sindresorhus]

Lastly there is a commit that switches to protocol buffers.

How do you like it? Do you think it's worth using or do you prefer the custom binary handling?

---

If we go for this, I think we should just publish vendor/protobufjs/minimal.js as a module instead of vendoring.

[novemberborn]

How do you like it? Do you think it's worth using or do you prefer the custom binary handling?

I'm not sure. On the one hand it's nice to not have to write the binary logic, on the other hand it's not that complicated. But it's quite likely that's just me 😉 There's overhead in managing the tooling too, and I don't know whether having the tooling makes it easier for others who end up having to deal with this code.

I'm not convinced it'll help with kathryn either, mostly because kathryn tries to be very generic, plugins included, and having to write protobufs for plugins just adds more authoring complexity.

What do you think, given the diff?

If we go for this, I think we should just publish vendor/protobufjs/minimal.js as a module instead of vendoring.

Yea. And then we can use Greenkeeper for updates too 😉 protobufjs is not semver compatible so abstracting it in a separate module is appealing.

[novemberborn]

To keep things simple, snapshots are only updated when --update-snapshots is passed. This means we don't automatically prune snapshots as tests change. We could consider warning the user when a snapshot is unused, though I'm not sure if it's worth it.

I wanted to come back to this. I'm not sure about the pruning behavior, either with the current AVA or with Jest itself. The more useful thing we have currently is that new snapshots are saved the first time they're asserted. This is nice with watch mode since you can just keep typing. On the other hand it seems strange that AVA would actually write files without being told to do so. The resulting snapshots aren't deterministic either. Files churn when run with --update-snapshots.

I'm leaning towards only updating when --update-snapshots is passed. However in watch mode it'd be neat if you can type u and it reruns all tests, updating snapshots. We can even suggest this to users when a snapshot is missing.

[novemberborn]

I've pushed color support. Update dependencies and use node cli.js test/fixture/formatting.js to see examples.

[sindresorhus]

What do you think, given the diff?

I'm gonna say it's up to you. I'm slightly in favor of Protocol Buffers, but it does add some overhead in tooling and I'm not seeing as much use for it as I had hoped, and you're right that the manual binary handling is not that advanced. For example, I would have thought that Protocol Buffers would handle the whole binary thing, so I'm curious why you're adding a header and version manually:

ava/lib/snapshot.js

Lines 91 to 92 in 1695c44

	BINARY_HEADER,
	VERSION_HEADER,

? Manual binary handling have the downside of boilerplate code and high chance of off-by-one errors.

[sindresorhus]

On the other hand it seems strange that AVA would actually write files without being told to do so.

It is being told so though, kinda. The user is explicitly writing a t.snapshot() assertion. I don't think it's very user-friendly to require a --update-snapshot every time the user creates a new t.snapshot(). That command is meant only for updating existing snapshots.

[sindresorhus]

I tried latest now with my existing snapshot and got:

  [anonymous]
  /Users/sindresorhus/dev/private/ava-playground/test.js:4

   3: test(t => {
   4:   t.snapshot({foo: {
   5:     bar: new Date()

  Error thrown in test

  Error:

  Error {
    message: 'Snapshot version is v7937, can only handle v1',
  }

Ah never mind, probably because the protocol buffer change.

---

Sidenote: I also think the error output could be better here. We're saying Error 3 times. Ideally it would be:

Error thrown in test:
Snapshot version is v7937, can only handle v1

[sindresorhus]

I think Contains 1 snapshot from 1 test. See test.js.snap for the actual snapshot. should be one separate lines so it will diff better. Only the first part is dynamic.

[sindresorhus]

See `test.js.snap` for the actual snapshot.

You can't really see the snapshot in that file, so I would rather say:

The actual snapshot is in test.js.snap.

Or something similar.

[novemberborn]

I'm curious why you're adding a header and version manually:

ava/lib/snapshot.js

Lines 91 to 92 in 1695c44

	BINARY_HEADER,
	VERSION_HEADER,

?

The header is so that people can see what generated the file. The version so that eventually, older AVA versions can detect a newer snapshot and not even try to decode it. Currently it's compressed and then within that there's the encoded index. If we change the compression then older AVA versions would just crash. If we change how the version is encoded inside the decompressed binary blob (easy to accidentally do with protobufs) then again older AVA versions would crash. Hence leaving it outside, which makes it easier to guarantee we never (accidentally) change it.

I'm slightly in favor of Protocol Buffers, but it does add some overhead in tooling and I'm not seeing as much use for it as I had hoped, and you're right that the manual binary handling is not that advanced.

Also, a big use case for protobufs is when you need to share data between different programs. You can write a definition once and then generate parsers / generators in different languages. Here it's just AVA reading its own output.

I tried latest now with my existing snapshot and got

Yes because I'm changing formats without regard for versioning in this work-in-progress PR.

I also think the error output could be better here. We're saying Error 3 times. Ideally it would be:

Error thrown in test:
Snapshot version is v7937, can only handle v1

Oh good observation! We should be able to remove the Error: line and just show the formatted error.

• Remove unnecessary Error: heading before formatted errors

I think Contains 1 snapshot from 1 test. See `test.js.snap` for the actual snapshot. should be one separate lines so it will diff better. Only the first part is dynamic.

• Place snapshot file reference on its own line

You can't really see the snapshot in that file, so I would rather say:

The actual snapshot is in `test.js.snap`.

Or something similar.

• Update snapshot file reference copy

On the other hand it seems strange that AVA would actually write files without being told to do so.

It is being told so though, kinda. The user is explicitly writing a t.snapshot() assertion. I don't think it's very user-friendly to require a --update-snapshot every time the user creates a new t.snapshot(). That command is meant only for updating existing snapshots.

That's a good interpretation.

• Automatically add new snapshots to existing file and report

[sindresorhus]

I like the idea of using Markdown for the readable version.

This is how I would design the report:

# Snapshot report for `test.js`

Contains 1 snapshot from 1 test.

The actual snapshot is saved in `test.js.snap`.

Generated by [AVA](https://ava.li). 

## Test title

**Snapshot 1**

\```
Object {
  foo: Object {
    bar: Date 2017-04-19T16:11:13.136Z {},
  },
}
\```

Ignore the \ of course.

[novemberborn]

@sindresorhus yea I can update to that. I'm using the indented code blocks though since there is no way for the formatted value to accidentally escape it.

🙊🙈🙉

[novemberborn]

🎉

[sindresorhus]

Yay! I'm super excited about this. 🙌

[vadimdemedes]