Remove snapshot files when a test file stops using snapshots

[ninevra]

Causes --update-snapshots to remove snapshot and report files if a test file stopped using snapshots.

The removal of snapshot and report files does not trigger --watch mode to rerun tests.

Snapshot & report files will not be removed if test.skip(), test.only(), t.snapshot.skip(), --match, or line number selection were used. --match and line number selection throw when used with --update-snapshots (as they did before), and the other cases print Could not update snapshots for ....

Causes --update-snapshots not to update snapshots if t.snapshot.skip() was used. This is in line with the already documented behavior. Previously, the t.snapshot.skip() assertion would fail, but no special report would be issued (likely allowing the failure message to be suppressed by a previous assertion failure) and snapshots would be updated anyway. Not technically a breaking change. Could theoretically break workflows that programmatically run --update-snapshots and skip assertions.

Promotes [email protected], previously a transitive dependency, to a top-level devDependency.

Possible future work (issues which came up in this process but are out-of-scope here):

• Could improve the Could not update snapshots for ... message
  • show the reason why snapshots could not be updated
  • only show for files where there was some actual snapshot update to perform
• Could attempt to improve the --update-snapshot interface and edge cases
  • better snapshot update interface for .failing() tests? (see Remove snapshot files when a test file stops using snapshots #2623 (comment))
  • harmonize snapshot comparison behavior (--update-snapshots can currently produce different comparison behavior in different test files in the same run; see Remove snapshot files when a test file stops using snapshots #2623 (comment))

Original post from draft pull request

Removes snapshot and report files if a test file stops using snapshots.

The removal of snapshot and report files should not trigger watch mode to rerun tests.

Will close #1424 when complete.

Temporarily blocked on avajs/eslint-plugin-ava#315.

---

So, I may have gone a little overboard with this. In particular:

What's tricky though is if tests are skipped, or if snapshot assertions are skipped, we should not remove the snapshot files.

I read the intent here as: snapshot files should not be removed if for some reason this test run is not representative, i.e. the user might not have been intending to stop using snapshots or might be intending to use them again soon.

With that understanding, I identified several more cases in which snapshot files maybe shouldn't be removed:

• a hook was .skip()ed (since hooks can contain snapshots)
• an assertion failed (since failing tests skip afterEach hooks)
• a test threw an exception (since execution could have stopped before a t.snapshot() assertion)
• the test file threw an exception during test declaration
• a test was marked failing() (since failing tests can throw exceptions) (*)
• tests were selected with .only(), --match, or line number selection.

(Currently, I've implemented and tested all of these checks, but probably some or all of them should be removed.)

However, this behavior (not updating snapshots on unrepresentative runs) is inconsistent with the existing behavior of --update-snapshots. --update-snapshots currently shares only the test.skip() restriction and the last restriction on the above list. In the normal case where --update-snapshots was passed, some snapshot assertions were called, and some were .skip()ed, --update-snapshots will happily delete the snaphots that were .skip()ed.

It seems odd that --update-snapshots, when given 9 t.snapshot.skip() and 1 t.snapshot(), would delete 9 snapshots, but when given 10 t.snapshot.skip() would delete none.

• Am I misunderstanding the intent here?
• Should the existing --update-snapshots behavior be changed to incorporate some or all of the above checks?
• Should any or all of the above checks be dropped?

---

(*): the t.failing() check is probably too coarse. Tests might be marked failing for a long time, so they probably shouldn't interfere with updating other tests' snapshots. Not sure how to refine this yet. If we don't care about snapshots in hooks, then the failing assertions check can be dropped and the t.failing() check can be refined to only block updating if an exception is thrown in the test.

---

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

• #1424: Remove snapshot files when a test file stops using snapshots

---

[novemberborn]

• Am I misunderstanding the intent here?

No. Non-representative is a good way of putting it.

• Should the existing --update-snapshots behavior be changed to incorporate some or all of the above checks?

Yea, sounds good. Nice spot!

• Should any or all of the above checks be dropped?

I don't care about hook behavior, that'll go away within the next few months.

[ninevra]

Rethinking this, I believe I was mistaken (in some cases dangerously so) about most of the conditions I listed. In particular,

• tests throwing exceptions is not separable from tests failing, since AVA supports outside exception-based assertion libraries.
• the interaction with tests marked .failing() is more complicated than I initially understood, and not germane to this issue.
  • If a test starts failing at a snapshot assertion (perhaps due to a regression) and is then marked .failing(), then later, unrelated runs with --update-snapshots will update the snapshot, causing the test to pass, even if the underlying issue wasn't fixed.
• the interaction between snapshots and errors thrown during test declaration is not limited to --update-snaphots, and so not germane to this issue.
• preventing snapshot updates when tests fail is a) not clearly helpful, b) not limited to --update-snapshots, and c) not clearly definable given the existence of test.failing().

Using snapshot assertions with nondeterministic test execution (whether thrown exceptions, concurrent try()s, concurrent promises, branching on the results of assertions, etc) does not work in the general case, and AVA generally has to trust the user not to do this. Trying to catch cases of nondeterministic test execution was an error & an overreach on my part.

tl;dr: I'm going to pare this PR back to essentially the original spec (skip()ed tests, skip()ed assertions, --match, .only(), line number selection). I apologize for the misguided digression.

[novemberborn]

@ninevra there might be some things worth exploring, even as a future work.

What if we didn't automatically write new snapshots when you add a test? And per test decide whether to write the result, e.g. if a failing test is passing, then we don't update its snapshot. Or we don't write any snapshots when there is a test failure (in this case, a failing test passing is a failure).

[ninevra]

There was a CI-dependent test failure. I believe I've resolved it, but it revealed some interesting edge-case behavior. It''s not important, just kind of odd/cool.

When running a test file with skipped tests in --update-snapshots mode, the snapshot manager is initialized with updating: false. This can be done because whether or not tests are skipped is known before any snapshot assertion could run. This means that in

test('the time', t => {
  t.snapshot(Date.now());
}
test('the time again', t => {
  t.snapshot(Date.now());
}

both snapshots pass in --update-snapshots mode, but in

test('the time', t => {
  t.snapshot(Date.now());
}
test.skip('the time again', t => {
  t.snapshot(Date.now());
}

the first snapshot fails. The manager in the second case isn't in updating mode, and so loads the existing snapshot from disk and compares against it, even though --update-snapshots was passed.

The new check for t.snapshot.skip() assertions can't imitate this behavior, because whether snapshot assertions are skipped isn't always knowable by the time the snapshot manager is initialized. So,

test('the time', t => {
  t.snapshot(Date.now());
}
test('the time again', t => {
  t.snapshot.skip(Date.now());
  t.pass();
}

the first snapshot passes (in --update-snapshots mode).

(In CI, the third example fails also, because snapshot assertions that would record new snapshots in CI instead fail. Thus the CI-dependent test failure.)

[ninevra]

@novemberborn
There are some intermittent test failures:

• One run on ubuntu + node v14 failed the shared-workers/worker-protocol test test workers are released when they exit. I can't reproduce this failure locally & can't see any way the diff could affect this test.
• One run on windows + node v12 timed out with the snapshot-removal and snapshot-updates tests pending.

Other than that, this should be ready for review. I've updated the header post to reflect the current state of the PR.

[novemberborn]

@ninevra thank you for your work on this, I'm hoping to take a look at it within the next few days.

[novemberborn]

The manager in the second case isn't in updating mode, and so loads the existing snapshot from disk and compares against it, even though --update-snapshots was passed.

Is this a bug then?

Possible future work (issues which came up in this process but are out-of-scope here):

Could you open issues so we can discuss these further?

[novemberborn]

One more thought on the updating problem. If we stored the test titles in the .snap files we should be able to regenerate the Markdown file. So we could add snapshots and keep the Markdown file ordered — but it won't be deterministic until you do a clean update.

[ninevra]

The manager in the second case isn't in updating mode, and so loads the existing snapshot from disk and compares against it, even though --update-snapshots was passed.

Is this a bug then?

I wouldn't consider this a bug, it has no practical effect and doesn't contradict any documentation.

Possible future work (issues which came up in this process but are out-of-scope here):

Could you open issues so we can discuss these further?

Opened #2634, #2635, #2636.

[novemberborn]

Awesome work, thank you @ninevra and happy new year 🎆