Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reconsider when to clear additional .snap.new files #492

Open
max-sixty opened this issue May 20, 2024 · 0 comments
Open

Reconsider when to clear additional .snap.new files #492

max-sixty opened this issue May 20, 2024 · 0 comments

Comments

@max-sixty
Copy link
Collaborator

max-sixty commented May 20, 2024

Currently we can get the following sequence:

  • Process 1: cargo insta test --accept -- test_foo is runs on every file change (e.g. with watchexec). test_foo passes fine.
  • Process 2: cargo insta test --accept is run manually. This finds an incorrect snapshot and creates a .snap.new file
  • Process 1: triggered by the file change, it re-runs its command on test_foo, test_foo passes fine again. It doesn't accept the .snap.new file because it's for an unrelated change. But it does remove it.
  • Process 2: completes, and would accept the change in .snap.new, but the file is gone. It doesn't raise an error.

So because we have two processes running, we've managed to run cargo insta test --accept, missed an incorrect snapshot, and yet every process has reported success.

Should we instead only remove additional .snap.new files when running cargo insta review? Unlike cargo insta test --accept, that's running over every .snap.new file; it can't delete ones that relate to other tests. cargo insta test --accept can still remove .snap.new files for the tests it's running, but wouldn't for other tests.

max-sixty added a commit to max-sixty/insta that referenced this issue Oct 9, 2024
For discussion, re mitsuhiko#492.

There are tests which confirm that a passing test removes any associated pending snapshot. Also need a test that checks `--unreferenced=delete` also picks up pending snapshots; then we know that running all tests with `--unreferenced=delete` is a catch-all way of removing any pending snapshots.

One test fails, re binary snapshots, because it's on the existing behavior.

My only reservation is whether I'm missing something, otherwise the proposed behavior seems better.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant