Various issues with assert.snapshot()

[tniessen]

Copied from #44429 (comment):

---

• I guess that the made-up serialization format was supposed to be human readable, but it really does not seem appropriate for assertions. I don't expect it to be reliable in the presence of things such as

  { [util.inspect.custom]() { return '#*#*#*#*#*#*#*#*#*#*#*#'; } }

• util.inspect() potentially omits information, leading to false negatives:

  const a = new ArrayBuffer(101);
  const snapshot1 = util.inspect(a);
  new Uint8Array(a)[a.byteLength - 1] = 1;
  const snapshot2 = util.inspect(a);
  assert.strictEqual(snapshot1, snapshot2);
  // passes even though the inspected value changed!

• util.inspect() is affected by util.inspect.defaultOptions, which means that two snapshots of the same value can be different, leading to false positives.

• Looking at the change history for util.inspect(), it seems that the default options changed between versions. This would potentially also break existing .snapshot files, leading to false positives.

• The documentation of util.inspect() literally says:

  The util.inspect() method returns a string representation of object that is intended for debugging. The output of util.inspect may change at any time and should not be depended upon programmatically.

• Reading and potentially writing a file at some opinionated location during a call to an assert API seems very unusual to me, especially because it causes the API to become async. It also seems like this could easily lead to unexpected conflicts.

  This is particularly problematic when the main module is not the actual test file (e.g., when using mocha or other test runners).

• I understand its purpose, but the addition of a CLI option to "refresh" snapshots (during API calls within assert) seems unusual to me (but maybe that's common practice somewhere).

---

Because this is an experimental feature, we can still revert it or modify it to at least address some of these issues.

[BridgeAR]

A couple of the issues can be overcome by settings the options. I believe serializing with our v8 serializer would indeed be safer?

[benjamingr]

Copying my comment from the other issue:

I think we generally just missed this in the original PR and hadn't considered the point you're making here and I think that after I've read your point it I agree with you. We should absolutely change the serialization to not always use util.inspect because of the false negatives/positives that might produce.

---

So, I consider this a bug in the current implementation (since the results aren't always correct) and I think we should modify this.

[benjamingr]

FWIW Jest also has a addSnapshotSerializer API

[MoLow]

A couple of the issues can be overcome by settings the options. I believe serializing with ourv8 serializer would indeed be safer?

@BridgeAR are you referring to https://nodejs.org/api/v8.html#new-serializer ?

[tniessen]

v8.serialize() is generally safe, however, the output may also depend on the version of V8 and thus on the version of Node.js being used. I believe it is backward compatible, so a newer version of V8 will usually be able to deserialize data produced by an earlier version of V8, but the converse might not be true: data serialized by a newer version of V8 might not be readable by older versions of V8. If snapshots are supposed to be committed to git repositories, this might be a problem.

[MoLow]

another approach would be to only accept strings, and escape strings that match snapshot delimiters, then we have no serialization

[benjamingr]

v8.serialize() is generally safe, however, the output may also depend on the version of V8 and thus on the version of Node.js being used. I believe it is backward compatible, so a newer version of V8 will usually be able to deserialize data produced by an earlier version of V8, but the converse might not be true: data serialized by a newer version of V8 might not be readable by older versions of V8. If snapshots are supposed to be committed to git repositories, this might be a problem.

I think "snapshots generated in newer versions of Node only work in these newer versions" is a fair approach (and if you need to change versions just regenerate the snapshots).

A different alternative would be for us to use Jest's serializer (or one of the other userland ones)

[tniessen]

another approach would be to only accept strings, and escape strings that match snapshot delimiters, then we have no serialization

Aside from how restrictive this limitation would be, why does assert.snapshot() use some unreliable made-up format instead of JSON or some other well-defined format? (Or even JavaScript files, which is what jest seems to do... not that I necessarily like the comparison.)

I think "snapshots generated in newer versions of Node only work in these newer versions" is a fair approach (and if you need to change versions just regenerate the snapshots).

I am not sure. My understanding of assert.snapshot() is that it is supposed to replace explicit assertions with .snapshot files that are committed to the repository. If someone creates said .snapshot file with version X of Node.js and commits it to some repository and then someone else checks out the repository with version Y < X, it might unexpectedly fail, even if both X and Y are non-EOL versions of Node.js and both support assert.snapshot().

---

This feature appears sufficiently complicated while easy to implement in userland that I am not sure including it in core is a good idea.

[BridgeAR]

why does assert.snapshot() use some unreliable made-up format instead of JSON

We would be limited to only accept input that is fully compliant to JSON and that contains no values that are stripped by JSON. Ideally, we serialize as much information as possible to support a wide range of inputs.

[tniessen]

@BridgeAR My comment was in response to the suggestion to only serialize key-value pairs of strings, in which case JSON would be more than enough :)

[benjamingr]

I played a bit with moving serialization to the v8.serializer/deserialize API and making it synchronous.

I think that:

• Making it synchronous is nice and simplifies code - so I am in favor of that change especially since JS doesn't have async stacks (only traces) - so this is an improvement and I'm in favor.
• Using v8.serialize/deserialize kind of sucks because it's a binary serialization format and it makes one of the most useful aspects of snapshot testing (being able to look at the diff during code review and seeing how behavior change) impossible - you basically end up with binary blocks.

Some thoughts:

• We can save the snapshot twice (one binary with 100% fidelity using the v8.serialize API and one textual with inspect with infinite depth for DX)
• We can JSON.stringify with a reasonable reviver which is a pretty common approach and support customSerializer/deserializer and that can take v8.serialize/deserialize for sacrificing developer experience for more supported cases.

[BridgeAR]

Having a sync API is tricky for big snapshots but we could accept that and just document it so that everyone is aware not to save huge files in a snapshot.

It is possible to prohibit custom inspect and define all options so that the user's custom settings won't impact the output (we should define the options anyway to guarantee the value is properly stored by inspecting hidden values etc.). So the first three mentioned issues above can all easily be resolved. The main issue I see with inspect is that the output may change at any point of time and it would not be backwards compatible. We could improve that by adding the Node.js version to the output and to allow to pass that through to a duplicate implementation that contains the implementation of each version that changed something on the output format. Not nice but it would work.
It would at least still be possible to diff the code during code reviews that way.

We can save the snapshot twice

This might not be sufficient as users might be confused in case the output format changed while switching versions and they just refreshed the snapshot and now there seem to be changes even though that's not actually the case.

[tniessen]

We can save the snapshot twice (one binary with 100% fidelity using the v8.serialize API and one textual with inspect with infinite depth for DX)

Data redundancy leads to inconsistency, which is something an assert API should avoid IMHO.

So the first three mentioned issues above can all easily be resolved. The main issue I see with inspect is that the output may change at any point of time and it would not be backwards compatible. We could improve that by adding the Node.js version to the output and to allow to pass that through to a duplicate implementation that contains the implementation of each version that changed something on the output format. Not nice but it would work.
It would at least still be possible to diff the code during code reviews that way.

It seems to me that util.inspect() was not at all designed for this purpose, and using it to reliably compare potentially complex data structures still seems like a hacky approach to me.

Finding a simple serialization format suitable for snapshots might be difficult. Jest seems to use CJS files as snapshots, which export the values.

---

The current implementation appears to unconditionally store the snapshot based on the main module path. Isn't this problematic when using test runners such as mocha, where the main module is not the user's test file?

If this API is going to be complex (custom serializers, where to store snapshots, tooling such as --update-assert-snapshot, matching using asymmetric matchers etc.), does it need to live in core?