Deterministic snaphot files, declaration-ordered snapshot reports #2610

ninevra · 2020-11-23T01:50:23Z

Closes #2311
Closes #2324

Ensures that *.snap files are deterministic by sorting entry blocks by the hash of their test name or id.

Ensures that *.md snapshot report files are sorted in as close to declaration order as is reasonably possible, by

emitting a taskIndex incremental id for each test case and hook,
emitting an associatedTaskIndex for each beforeEach, afterEach, and afterEachAlways hook, indicating the associated task case,
passing these ids and the pre-existing snapshotCount through to snapshotManager, and
sorting new entries (test cases, hooks, or snapshot ids) according to the taskIndex, associatedTaskIndex, and snapshotCount of the first snapshot therein.

Caveats

*.snap files are not sorted by declaration order, only *.md files. Sorting *.snaps by declaration order is more complicated, because when running in appendOnly mode (i.e. without the -u flag) they may contain entries which are never touched during the course of the run. These entries would then have to be sorted among the others in a deterministic fashion. While this could be accomplished with a fallback to sorting by hash, it's not clear where such entries should appear in the sort order. It's also not clear that this grants any benefit, since after encoding and compressing the order of entries is not readily apparent to a human reader in any case.
In appendOnly mode, only the new report entries are sorted in declaration order, and only among themselves.
Declaration order is not a terribly useful sort order for snapshots in hooks, particularly beforeEach and afterEach hooks. Further, *Each hooks are run multiple times, so taskIndex and snapshotCount don't uniquely identify them, requiring a third sort key (associatedTaskIndex). This should only matter until the next AVA major version, given Disable snapshots and try assertions in hooks #2523.

Release concerns

As far as I can tell, this change won't affect any existing codebases' *.snap or *.md files until either AVA is run with -u or some change is made to the corresponding tests. snapshotManagers existing behavior is to not record anything, even internally, unless -u is enabled or there was no preexisting snapshot to compare to. However, this is difficult to definitively test.

No changes have been made to the snapshot file format.

Remaining issues

When AVA is run with -u, projects may see unexpected snapshot updates as their snapshots and reports are resorted. Avoiding this would require reworking snapshotManager to: load saved snapshots even when -u is present; always report success from compare() if -u is present; and write updates only if some conflict or new snapshot was encountered.

Should this be placed behind a feature flag, to reduce the incidence of spurious updates?

Produce a taskIndex for each hook and test. Sort snapshot headers (called `belongsTo`) according to the taskIndex in which their first snapshot was declared.

This should ensure that all sort keys are unique and so determinism is not affected by the stability of Array.prototype.sort().

Adds a third sort key, associatedTaskIndex, only set on Each hooks. Preserves sort determinism on node v10.

ninevra · 2020-11-23T04:13:03Z

Sorry, it seems like the testing approach I took doesn't work in CI. I'll have to rethink how to test this.

lib/snapshot-manager.js

test/snapshot-order/fixtures/report-declaration-order/test.js

test/snapshot-order/test.js

novemberborn · 2020-11-23T14:46:26Z

This looks really promising @ninevra! Let me know how I can help!

Should this be placed behind a feature flag, to reduce the incidence of spurious updates?

No, I think this is fine. As long as existing snapshot files are accepted we're good.

When ava is run in CI, new snapshots cannot be written. In normal circumstances, this ensures that snapshot assertions don't get merged without a recorded snapshot; lacking this feature, CI could appear to pass when in fact snapshot assertions were not checked. In this case, however, writing snapshots is intended.

ninevra · 2020-11-27T20:48:39Z

The remaining CI failures are also present on master when running on node v15.3.0. This appears to be unrelated to this PR, and may not actually a bug; reported in #2616.

I'd still like to add more tests to this, ideally asserting that existing snapshots aren't rewritten.

These snapshots are always recreated and their exact content is not tested, so they shouldn't be version controlled.

novemberborn

I'd still like to add more tests to this, ideally asserting that existing snapshots aren't rewritten.

The test coverage is impressively thorough already! What do you mean by not rewriting existing snapshots? FWIW if the files are deterministic I'm not overly concerned about writing unnecessarily. Not sure how to test that either other than by looking at modification dates maybe… doesn't seem worthwhile.

I left two minor comments on the tests. Nice use of the self-testing setup.

The remaining CI failures are also present on master when running on node v15.3.0. This appears to be unrelated to this PR, and may not actually a bug; reported in #2616.

Thanks, fixing in #2618.

test/snapshot-order/backwards-compatibility.js

test/snapshot-order/randomness.js

Addressed.

novemberborn · 2020-11-30T15:28:47Z

@ninevra if you pull in latest master then tests should pass.

It does contain some linter related changes so there may be a bit of churn.

…tic-snap-files

ninevra · 2020-11-30T20:38:57Z

@novemberborn

I'd still like to add more tests to this, ideally asserting that existing snapshots aren't rewritten.

I was referring to the backwards-compatibility test suite I later added in 7dcb928, which demonstrates that existing unsorted snapshots won't be overwritten until AVA is run with --update-snapshots or the tests are modified. Mostly that was just to convince myself I wasn't reading the code wrong and missing something.

The test coverage is complete now as far as I'm concerned.

(Although, the backwards-compatibility suite is not of any real value after this point, and is difficult to maintain; if the snapshot file format is changed in the future, obtaining valid & unsorted snapshots will be difficult, so the suite should probably be removed at that point. We could remove it preemptively now to reduce maintainence burden.)

Co-authored-by: Mark Wubben <[email protected]>

novemberborn

Although, the backwards-compatibility suite is not of any real value after this point, and is difficult to maintain; if the snapshot file format is changed in the future, obtaining valid & unsorted snapshots will be difficult, so the suite should probably be removed at that point. We could remove it preemptively now to reduce maintainence burden.

Yes let's do that.

There's a few more places where we could write the in-memory buffers. Other than that I think this is good to go!

If you wouldn't mind touching this up I'll try and get it out in a release soon.

test/snapshot-order/backwards-compatibility.js

ninevra · 2020-12-01T21:37:54Z

Removed the backwards-compatibility test suite per #2610 (review). This incidentally removes the remaining instances of copyFileSync (sorry for missing those in the first place). Hopefully I'm not misunderstanding your request?

novemberborn · 2020-12-06T17:57:20Z

Awesome, thanks @ninevra!

novemberborn · 2020-12-07T09:09:32Z

3.14 is live with these changes.

fisker · 2020-12-07T09:34:08Z

Wow, maybe I can remove this hack sindresorhus/eslint-plugin-unicorn#842 👍

ninevra added 12 commits November 21, 2020 19:00

Test .snap is independent of test resolution order

47a0015

Sort snapshots before encoding them

6d6e211

Move intertest-order fixture to subdirectory

bec3485

Minor changes to intertest-order test

2b22330

Test that reports are sorted in declaration order

c2b7dff

Test report order of snapshots in hooks

93bf925

Sort reports by approximate declaration order

342ec6b

Produce a taskIndex for each hook and test. Sort snapshot headers (called `belongsTo`) according to the taskIndex in which their first snapshot was declared.

Pass through snapshotCount as a fallback sort key

69f4d7f

This should ensure that all sort keys are unique and so determinism is not affected by the stability of Array.prototype.sort().

Refactor to include ordering data in entries map

52b4b29

Polyfill matchAll()

4585a11

Stabilize sort of *Each* hook snapshot reports

cd29d35

Adds a third sort key, associatedTaskIndex, only set on Each hooks. Preserves sort determinism on node v10.

Test more *Each hooks

d6b25db

ninevra force-pushed the deterministic-snap-files branch from 624a47c to d6b25db Compare November 23, 2020 01:51

Update package.json

72d78bb

sindresorhus previously requested changes Nov 23, 2020

View reviewed changes

ninevra added 5 commits November 27, 2020 00:34

Update package.json

5b78ddf

Expand abbreviations

8fe455b

Prefer destructuring

f15a7a6

Fix capitalization

244cdfc

ninevra added 5 commits November 28, 2020 00:38

Test behavior with existing unsorted snapshots

7dcb928

Clean up transient snapshots after tests

67bb529

These snapshots are always recreated and their exact content is not tested, so they shouldn't be version controlled.

Use t.teardown() for cleanup

dcd84dc

Add a larger test case

5afacb4

Organize tests into separate files

241b170

ninevra marked this pull request as ready for review November 28, 2020 22:48

novemberborn reviewed Nov 30, 2020

View reviewed changes

test/snapshot-order/backwards-compatibility.js Outdated Show resolved Hide resolved

test/snapshot-order/randomness.js Outdated Show resolved Hide resolved

ninevra added 2 commits November 30, 2020 12:01

Merge branch 'master' of https://github.com/avajs/ava into determinis…

ce77f43

…tic-snap-files

Prefer writing over copying when files are in memory

9f05271

Add a snapshot message for clarity

c2153e9

Co-authored-by: Mark Wubben <[email protected]>

novemberborn reviewed Dec 1, 2020

View reviewed changes

test/snapshot-order/backwards-compatibility.js Outdated Show resolved Hide resolved

test/snapshot-order/backwards-compatibility.js Outdated Show resolved Hide resolved

Remove unmaintainable backwards-compatibility tests

49ff55f

novemberborn merged commit e66b54c into avajs:master Dec 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Deterministic snaphot files, declaration-ordered snapshot reports #2610

Deterministic snaphot files, declaration-ordered snapshot reports #2610

Uh oh!

ninevra commented Nov 23, 2020 •

edited by novemberborn

Loading

Uh oh!

ninevra commented Nov 23, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

novemberborn commented Nov 23, 2020

Uh oh!

ninevra commented Nov 27, 2020

Uh oh!

novemberborn left a comment

Uh oh!

Uh oh!

Uh oh!

novemberborn commented Nov 30, 2020

Uh oh!

ninevra commented Nov 30, 2020

Uh oh!

novemberborn left a comment

Uh oh!

Uh oh!

Uh oh!

ninevra commented Dec 1, 2020

Uh oh!

novemberborn commented Dec 6, 2020

Uh oh!

novemberborn commented Dec 7, 2020

Uh oh!

fisker commented Dec 7, 2020

Uh oh!

Uh oh!

Deterministic snaphot files, declaration-ordered snapshot reports #2610

Deterministic snaphot files, declaration-ordered snapshot reports #2610

Uh oh!

Conversation

ninevra commented Nov 23, 2020 • edited by novemberborn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Caveats

Release concerns

Remaining issues

Uh oh!

ninevra commented Nov 23, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

novemberborn commented Nov 23, 2020

Uh oh!

ninevra commented Nov 27, 2020

Uh oh!

novemberborn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

novemberborn commented Nov 30, 2020

Uh oh!

ninevra commented Nov 30, 2020

Uh oh!

novemberborn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ninevra commented Dec 1, 2020

Uh oh!

novemberborn commented Dec 6, 2020

Uh oh!

novemberborn commented Dec 7, 2020

Uh oh!

fisker commented Dec 7, 2020

Uh oh!

Uh oh!

ninevra commented Nov 23, 2020 •

edited by novemberborn

Loading