-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Description
A summary of snapshot related issues. See #1218, #1220, #1254 and #1223.
Fundamentally, jest-snapshot
is string based. The snapshots contain human-readable representations of expected values, which are then compared against another representation of the actual value when tests are run.
Snapshot testing is useful when code generates complex tree structures that are tedious to verify by writing assertions, and that may change significantly as part of day-to-day development. Humans manually verify that the snapshot is as expected the first time it's generated. To do this, the snapshot needs to be readable. Then, when the actual value changes and the snapshot is regenerated, source control diffing makes it easy to see what's actually changed and if that is as expected.
The original snapshot PR (#1113) used jest-snapshot
as-is (thanks @lithin!). Here, jest-snapshot
was also responsible for the error message:
expect(value).toMatchSnapshot()
Received value does not match stored snapshot 1.
- Snapshot
+ Received
Object {
- "foo": "bar",
+ "foo": "bar!",
}
Soon after this landed @vadimdemedes started working on improving our error output, specifically showing diffs for t.deepEqual()
and t.is()
assertions (#1154). This work also included t.snapshot()
. However, to generate these diffs we need actual objects, not the error message that jest-snapshot
returns. (Plus, that expect(value).toMatchSnapshot()
line is a bit jarring.)
The solution was to send a JSON string into jest-snapshot
, with a workaround for React trees. This means though that the snapshot contains a fairly unreadable JSON string (#1254). Also, whereas jest-snapshot
normalizes key order in Object
objects, AVA can no longer rely on that because we're now just comparing JSON strings. Then when we generate a diff it comes up empty since our diffing logic does not care about key order (#1220).
@vadimdemedes started a PR (#1223) to replace jest-snapshot
with our own implementation, using t.deepEqual()
for the comparisons. The advantage of this approach is that diffs look the same for all our (diffing) assertions. Unfortunately, because it uses JSON serialization it limits the kinds of object structures we can safely snapshot:
undefined
,Symbol()
and function property values are droppedundefined
,Symbol()
and function array items are replaced bynull
- regular expression property values and array items are replaced by
{}
- no support for built-ins such as
Map
andSet
- no support for custom classes: instances are reduced to plain objects
- we're effectively using
toJSON()
for comparisons, which we don't do fort.deepEqual()
We need to decide whether we want error output for t.snapshot()
to be the same as that for an equivalent error from t.deepEqual()
. I don't think we can do this while still using jest-snapshot
without hurting snapshot readability.
The current JSON proposal severely limits the kinds of snapshots that can be created. This should only be a short-term solution, but as this issue establishes we have some critical issues with our current snapshot implementation right now, so I'd rather we ship something that works real soon. We can then expand the functionality going forward.
If we go this route we should enforce that snapshots cannot contain any values incompatible with JSON serialization. We can do this when adding a snapshot by comparing the snapshot against the value being added.
On the other hand using jest-snapshot
provides more immediate user value. We could go that route and try and improve the diff output later. At that point though a reduction in functionality would not be acceptable.
If you've made is this far I thank you for your dedication. Please respond with your feedback 😄