(Re)generate snapshot report from snapshot file

[ninevra]

It seems like several issues (#2635, #2634, #1768; #2099 is tangentially related) might be easier to address if the .snap files contained enough information fo the snapshot report to be generated solely from the snapshot file.

There are three pieces of information I can think of that are missing from the v2 .snap file:

1. Test titles
2. Snapshot labels
3. Snapshot declaration order

I think that, if all of this information were present in the .snap file, then it would be possible to allow using test.skip(), test.only(), t.snapshot.skip(), --match et al when running --update-snapshots. The snapshot manager would load existing snapshots from disk at the start of the run, and when a skipped test or snapshot was declared, it would record a "blank space" (or to put it another way, copy over the old snapshot(s)). This would address #2635, by allowing AVA's existing tools to be used to "partially" update a file's snapshots, and #2634, by removing the need for the "Could not update snapshots" message entirely.

One caveat with the above: snapshot blocks which actually go entirely untouched (for instance because a test or id'd snapshot assertion was removed entirely, not just skip()ed) can't be ordered relative to new snapshot blocks. This has two effects:

• Snapshots with ids could no longer be sorted in declaration order, because they might only be touched in skip()ed tests. Probably they would have to be sorted to the end of the report file in alphabetical order. Maybe there's a magic solution involving associating them with the test titles that use them.
• AVA would have to either preemtively prune removed snapshots & re-order moved tests/snapshots even when running without --update-snapshots, or refrain from recording new snapshots until run with --update-snapshots. Either of these would be a substantial change to the workflow.
  • The former sounds scary, but I haven't actually thought of any major issues it would cause, and having reports always in the right order would be a much nicer experience than AVA's current append-only behavior.
  • The latter would work well with the --commit-snapshots behavior described in Consider not updating snapshots automatically #2099.

#1768 suggested that this added information would be useful or necessary for writing tools that operate on AVA's snapshot files, because otherwise consumers would need to additionally parse the report file, which isn't designed to be machine-readable (and isn't necessarily governed by the same versioning scheme as .snap files).

This could also be an opportunity to add additional functionality to the .snap file format, such as:

List of long-shot suggestions

• attaching arbitrary annotations to snapshots, as envisioned in Exposing snapshot manager logic as a library #1768

• recording any concordance plugins used

  I'm not sure if this is practical or useful. I'm imagining recording the npm package name and version of the plugin, so that consumers (incl. AVA itself, if it ever comes to support customization of concordance plugins) can at least know if that plugin is available to them and not accidentally deserialize with an incompatible concordance configuration. Potentially this would be better handled in concordance, by encoding the plugins used in the descriptor serialization itself (for all I know, this is already done).

• adopting a more general binary format, e.g. bson, to make future changes to the snapshot format somewhat easier and potentially not require a major version bump

  Using a tagged-type, named-field format like bson would allow adding fields to the snapshot data structures while retaining the ability to parse old snapshot files (without duplicating the entire serialize/deserialize code). New features could then be opted-into on a per-file or per-snapshot basis, though there would be some code duplication involved there, as the codepath for handling snapshots without the new feature would have to be retained. (This is probably wishful thinking, it might turn out to be impossible to ship such features in a user-intelligible way, and there might never be any such features in any case.)

@novemberborn is this worth exploring? I'd be happy to implement this, especially if there's any chance it's in-scope for [email protected]

[novemberborn]

is this worth exploring? I'd be happy to implement this, especially if there's any chance it's in-scope for [email protected]

Yea!

And as long as it's backwards compatible with the v2 format, it wouldn't even need a major release.

• recording any concordance plugins used […] (for all I know, this is already done).

I think it is, but it's been a few years since I've looked at that code.

• adopting a more general binary format, e.g. bson, to make future changes to the snapshot format somewhat easier and potentially not require a major version bump

This might not be worthwhile… I have not looked at bson much, but working with Protobufs in the day job I wouldn't want to load all that complexity into AVA.

[ninevra]

BSON is a lot closer to JSON than to Protocol Buffers; each field is tagged with its name and type, so serializing/deserializing doesn't require any schema, config, or IDL. I was put in mind of bson by your suggestion in #1768 (comment) of attaching annotations to snapshots; the annotations field would essentially be encoding arbitrary user-provided data, which would require something similar to json or bson in any case.

I've not personally used either Protobufs or bson, so I'm not exactly speaking from experience here.

[novemberborn]

I'd be fine with BSON, though I'm surprised the package is 3MB! https://www.npmjs.com/package/bson I need to look into this a bit more.

[novemberborn]

Turns out it contains a lot of bundled files for various targets.

I think https://github.com/mcollina/msgpack5 may be closer to what we're doing. What do you think?

[ninevra]

msgpack definitely looks preferable to bson. Looks like it's a lot closer to js than bson is; bson made some strange choices like using null-terminated strings and not supporting non-string map keys.

There's like half a dozen msgpack implementations to choose from.

There's also the option of keeping the current ad-hoc snapshot format (plus trailers) and using json to serialize the annotations.

[ninevra]

And as long as it's backwards compatible with the v2 format, it wouldn't even need a major release.

Doing this backwards-compatibly is probably possible, but seems somewhat impractical and maybe not beneficial. The main issues I'm seeing are declaration-ordering and release timing.

Currently, when snapshots are added or changed, AVA appends them to the report and rewrites the .snap. The report then consists of one or more segments, each internally declaration-ordered. The .snap file is in an arbitrary, deterministic order.

Putting the .snap file into declaration order isn't strictly possible with the current workflow, because it can contain retained old snapshots from tests that no longer exist and therefore have no ordering. I see two options:

1. Saving the report order is probably possible; one would add a trailer associating a segment number and an ordering key with each snapshot. This would allow regenerating the report in its multiply-appended form.
2. Alternatively, an arbitrary decision could be made about how to order any snapshots that weren't touched in the last run, perhaps shunting them to the end of the order. This would allow regenerating the report in a form that had only one segment and was declaration-ordered among all snapshots touched in the last run (which should be all snapshots except those for tests that no longer exist). (I prefer this option; I think it generalizes better.)

The second issue is release timing. If new features are added that rely on being able to regenerate the snapshot report, then they would need to either:

1. insist on updating snapshots. This would look strange to a user, as there would be no version bump or other visible change. Further, if multiple developers on a project used AVA versions stradling the change, then it could happen repeatedly, as one developer's AVA rewrote snapshots with the old format and another's AVA insisted on the new format; this is behavior consistent with a semver-major change.
2. be prepared to handle either old or new snapshots, enabling or disabling the feature on a per-file basis. This would be a long-term maintainability challenge and also make the workflow of such features a lot more confusing for the user.

[novemberborn]

Two quick points, but I'll get back to you later.

We can make breaking changes now, provided we get it all into AVA 4.

The author of https://github.com/mcollina/msgpack5 writes good software I rely on in production, so I'd be happy to pick that implementation. It'll likely be easier to grok than a custom format.

[ninevra]

Looking into msgpack5, it's got some potentially problematic issues. Most significantly, it encodes both plain js objects and js Maps as msgpack maps, and decodes msgpack maps as js objects iff they have only string-valued keys, otherwise as js Maps. String-keyed Maps therefore don't round-trip through serialization, and their iteration order is lost (replaced with the js object traversal order). There is no way to configure this behavior; I tried several things using msgpack ext types, but unfortunately the default encoding of Maps takes precedence over registered extensions. Less signficantly, msgpack5 throws when encoding NaN and infinities, despite msgpack supporting them it and msgpack5 itself decoding such values just fine.

(I'm probably going to keep using an ad-hoc format while prototyping this, and defer worrying about msgpack/annotations until a bit later.)

[novemberborn]

Ah that's too bad on msgpack5.

Putting the .snap file into declaration order isn't strictly possible with the current workflow, because it can contain retained old snapshots from tests that no longer exist and therefore have no ordering.

Wouldn't these get cleared up?

[ninevra]

Retained old snapshots are cleaned up on --update-snapshots runs, yes. Under the current workflow, though, new snapshots can be added even without --update-snapshots. If snapshot assertions are added in one test, another test is removed, and AVA is run without --update-snapshots, the report gets rewritten, but the snapshots from the removed test are retained.

Currently, the issue is dealt with by appending the changes to the existing report. The appended changes are sorted in declaration order. But, reconstructing the multiply-appended report requires a lot more data than is otherwise relevant.

The workflow changes I hypothesized in the original post are intended to sidestep this issue, by ensuring that any time snapshots are added, old snapshots can also be removed. That could mean either clearing up snapshots even in non--u runs, or not adding snapshots except in -u runs.

(I think there may be some value, though, in defining what happens in this scenario. It lets the changes to the snapshot format be considered [mostly] in isolation from the hypothetical changes to the workflow.) (Also, if AVA gets a "review-snapshots" tool it would be natural for that tool to offer the user a choice to retain old snapshots, bringing this problem up again. I'm excited about that possibility, but it's admittedly quite a remote concern.)

[novemberborn]

Yes happy to decouple format changes from workflow changes, especially since these workflow changes can be made in minor releases.

Whatever is the simplest option then. I think that's your second suggestion:

2. Alternatively, an arbitrary decision could be made about how to order any snapshots that weren't touched in the last run, perhaps shunting them to the end of the order. This would allow regenerating the report in a form that had only one segment and was declaration-ordered among all snapshots touched in the last run (which should be all snapshots except those for tests that no longer exist). (I prefer this option; I think it generalizes better.)

[ninevra]

@novemberborn
How would you feel about using cbor? cbor and msgpack are very similar formats, but cbor's more precisely documented, supports more of javascript's data model (notably bigints and undefined), somewhat simpler in structure, and more extensible. I think either msgpack5 or the npm cbor package would work just fine.msgpack5 has some issues indicative of a lack of maintainer time, however (a breaking change was recently accidentally shipped in a minor version, and the prebuilt browser releases are not kept up-to-date), and I get the impression that the npm cbor package has something closer to an ecosystem consensus.

I propose to use either cbor or msgpack5 and representing snapshot data as an array of blocks, where each block has a title and an array of snapshots:

[
  {
    "title": "a test",
    "snapshots": [
      {
        "label": "a snapshot",
        "buffer": /* serialized descriptor */
      },
      ...
    ]
  },
  ...
]

An annotations field could then be added backwards-compatibly at a later date if desired, as described in #1768. label would be optional, and default to "Snapshot <index>" when absent.

---

I think that, if all of this information were present in the .snap file, then it would be possible to allow using test.skip(), test.only(), t.snapshot.skip(), --match et al when running --update-snapshots. The snapshot manager would load existing snapshots from disk at the start of the run, and when a skipped test or snapshot was declared, it would record a "blank space" (or to put it another way, copy over the old snapshot(s)).

Found one issue with this: if the old snapshots exist but are unreadable (most likely because the snapshot format version was just incremented) then copying forward snapshot data naturally doesn't work. Options:

• continue failing whenever a subset of snapshots are selected during --update-snapshots
• fail only in the intersection case, where --update-snapshots was passed, a subset of snapshots was selected, and the old snapshots exist and are unreadable
• don't record any data for the skipped snapshots, allowing it to be filled in later

What do you think?

[novemberborn]

How would you feel about using cbor?

Ohhhhh, and it comes with an RFC! Plus, that package is tested using AVA 😉 I'd love to look at this in more detail but but it sounds good to me at this stage.

---

if the old snapshots exist but are unreadable (most likely because the snapshot format version was just incremented) then copying forward snapshot data naturally doesn't work

I think the analogy is where the snapshot file does not exist. In that case, which option would you prefer?

[ninevra]

I'd prefer "don't record any data for the skipped snapshots".

This case should only occur occasionally, and the advised solution is to --update-snapshots, so updating snapshots should actually work and resolve the issue. (Also, it's not like any data is actually lost if the user accidentally only updates a subset of snapshots at this stage; the old snapshots were, after all, unreadable.)

I think it would be better if the resulting "empty spaces" compared unequal to everything until manually approved, but that's #2099 again, not a new issue.

[novemberborn]

I'd prefer "don't record any data for the skipped snapshots".

This case should only occur occasionally, and the advised solution is to --update-snapshots, so updating snapshots should actually work and resolve the issue.

To make sure I understand correctly, AVA fails the assertions, because it can't process the snapshot file. It then says to update snapshots. We'll create a new file with all the snapshots that were created during the test run. Any skipped ones are missing.

I agree that's fine.

Though if you then try to update snapshots again, because AVA does understand the snapshot file, it refuses, since you're skipping some. Correct?

I think it would be better if the resulting "empty spaces" compared unequal to everything until manually approved, but that's #2099 again, not a new issue.

How much do you want to tackle? 😉 We could try and get all those changes into AVA 4. Just have to be careful and make sure we can ship the intermediate steps.

---

CBOR is great, by the way. Nice find.

[ninevra]

To make sure I understand correctly, AVA fails the assertions, because it can't process the snapshot file. It then says to update snapshots. We'll create a new file with all the snapshots that were created during the test run. Any skipped ones are missing.

I agree that's fine.

Though if you then try to update snapshots again, because AVA does understand the snapshot file, it refuses, since you're skipping some. Correct?

Correct except for the last part; part of (my) goal was to "allow using test.skip(), test.only(), t.snapshot.skip(), --match et al when running --update-snapshots." So, --update-snapshots will always allow using test/snapshot selection mechanisms. When there's old data for the skipped test/snapshot, that data is copied forward. When there isn't data, or the data's unreadable, nothing is recorded. In your example, the second run of --update-snapshots still records nothing for the skipped tests/snapshots. When next those tests/assertions are actually executed, new data is recorded, filling in the blanks. (Filling in blanks doesn't require --update-snapshots, as it's identical to adding new tests/snapshot assertions, which doesn't currently require --update-snapshots.)

This behavior is separable from the proposed new snapshot format, but it's the part that allows closing #2634 and #2635.

[novemberborn]

Great!