Repository: mitsuhiko/insta (rust)

Issue:
`--unreferenced=auto` potentially deleting snapshots after a test
### What happened?

running `cargo insta test --features=default,test-dbs --unreferenced=auto` on PRQL at 1c9a67cf7f29b551af3310bce92b29f3e3ae974b deleted the snapshots at the end of the run

(wanted to put an issue in to avoid forgetting, can't look into it atm, but will try to soon)

### Reproduction steps

_No response_

### Insta Version

1.42.2

### rustc Version

1.84.1

### What did you expect?

_No response_

Pull request description:
fix `--unreferenced=auto` clearing new pending snapshots
closes #751


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0d94072..08b2cf0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,16 +2,19 @@
 
 All notable changes to insta and cargo-insta are documented here.
 
-## Unreleased
+## 1.43.0
 
+- Add uppercase keyboard shortcuts for bulk operations in `cargo insta review`:
+  `A` to accept all, `R` to reject all, and `S` to skip all remaining snapshots.
+  #745
+- `--unreferenced=auto` (or other relevant values) no longer cleans up pending
+  snapshots. A bug where `cargo insta test --unreferenced=auto` would
+  incorrectly pass on new pending snapshots has been fixed.
 - Support specifying `cargo-nextest` bin with `INSTA_CARGO_NEXTEST_BIN`.  #721 (Louis Fruleux)
 - Allow setting `INSTA_WORKSPACE_ROOT` at compile time. This is useful for reproducible binaries
   so they don't contain references to `CARGO_MANIFEST_DIR`. #726 (Pascal Bach)
 - Qualify all references in macros to avoid name clashes. #729 (Austin Schey)
 - Remove `linked-hash-map` and `pin-project` dependencies.  #742, #741, #738
-- Add uppercase keyboard shortcuts for bulk operations in `cargo insta review`:
-  `A` to accept all, `R` to reject all, and `S` to skip all remaining snapshots.
-  #745
 - `cargo insta review` fails with a helpful error message when run in a non-TTY environment.
 
 ## 1.42.2
diff --git a/cargo-insta/src/cli.rs b/cargo-insta/src/cli.rs
index c6d5bd9..ea00e25 100644
--- a/cargo-insta/src/cli.rs
+++ b/cargo-insta/src/cli.rs
@@ -877,7 +877,7 @@ fn handle_unreferenced_snapshots(
         UnreferencedSnapshots::Ignore => return Ok(()),
     };
 
-    let files = fs::read_to_string(snapshot_ref_path)
+    let snapshot_files_from_test = fs::read_to_string(snapshot_ref_path)
         .map(|s| {
             s.lines()
                 .filter_map(|line| fs::canonicalize(line).ok())
@@ -910,19 +910,14 @@ fn handle_unreferenced_snapshots(
         .filter_map(|e| e.path().canonicalize().ok())
         // The path isn't in the list which the tests wrote to, so it's
         // unreferenced.
-        //
-        // TODO: note that this will include _all_ `.pending-snap` files,
-        // regardless of whether or not a test was run, since we don't record
-        // those in the snapshot references file. We can make that change, but
-        // also we'd like to unify file & inline snapshot handling; if we do
-        // that it'll fix this smaller issue too.
+        .filter(|path| !snapshot_files_from_test.contains(path))
+        // we don't want to delete the new or pending-snap files, partly because
+        // we use their presence to determine if a test created a snapshot and
+        // so `insta test` should fail
         .filter(|path| {
-            // We also check for the pending path
-            let pending_path = path.with_file_name(format!(
-                "{}.new",
-                path.file_name().unwrap().to_string_lossy()
-            ));
-            !files.contains(path) && !files.contains(&pending_path)
+            path.extension()
+                .map(|x| x != "new" && x != "pending-snap")
+                .unwrap_or(true)
         });
 
         for path in unreferenced_snapshots {


Test patch:
diff --git a/cargo-insta/tests/functional/binary.rs b/cargo-insta/tests/functional/binary.rs
index e099478..dfa0357 100644
--- a/cargo-insta/tests/functional/binary.rs
+++ b/cargo-insta/tests/functional/binary.rs
@@ -177,9 +177,10 @@ fn test_binary_snapshot() {
         )
         .create_project();
 
-    assert!(!&test_project
+    // create the snapshot
+    assert!(&test_project
         .insta_cmd()
-        .args(["test"])
+        .args(["test", "--accept", "--", "--nocapture"])
         .output()
         .unwrap()
         .status
diff --git a/cargo-insta/tests/functional/delete_pending.rs b/cargo-insta/tests/functional/delete_pending.rs
index 34a4d95..37869f1 100644
--- a/cargo-insta/tests/functional/delete_pending.rs
+++ b/cargo-insta/tests/functional/delete_pending.rs
@@ -24,6 +24,7 @@ fn test_snapshot_file() {
         )
         .create_project();
 
+    // initially it'll fail
     assert!(!&test_project
         .insta_cmd()
         .args(["test", "--", "--nocapture"])
@@ -32,18 +33,26 @@ fn test_snapshot_file() {
         .status
         .success());
 
+    // then it'll pass with `--accept`
+    assert!(&test_project
+        .insta_cmd()
+        .args(["test", "--accept", "--", "--nocapture"])
+        .output()
+        .unwrap()
+        .status
+        .success());
+
     assert_snapshot!(test_project.file_tree_diff(), @r"
     --- Original file tree
     +++ Updated file tree
-    @@ -1,4 +1,8 @@
+    @@ -1,4 +1,7 @@
      
     +  Cargo.lock
        Cargo.toml
        src
-    +    src/.lib.rs.pending-snap
          src/lib.rs
     +    src/snapshots
-    +      src/snapshots/delete_unreferenced__snapshot_file.snap.new
+    +      src/snapshots/delete_unreferenced__snapshot_file.snap
     ");
 
     // Now remove the tests; the pending snapshots should be deleted when
diff --git a/cargo-insta/tests/functional/main.rs b/cargo-insta/tests/functional/main.rs
index 66f75fd..5cb5346 100644
--- a/cargo-insta/tests/functional/main.rs
+++ b/cargo-insta/tests/functional/main.rs
@@ -71,6 +71,7 @@ use tempfile::TempDir;
 mod binary;
 mod delete_pending;
 mod inline;
+mod unreferenced;
 mod workspace;
 
 /// Wraps a formatting function to be used as a `Stdio`
@@ -617,92 +618,6 @@ fn foo_always_missing() {
     // Check for the name clash error message
     assert!(error_output.contains("Insta snapshot name clash detected between 'foo_always_missing' and 'test_foo_always_missing' in 'snapshot_name_clash_test'. Rename one function."));
 }
-
-#[test]
-fn test_unreferenced_delete() {
-    let test_project = TestFiles::new()
-        .add_cargo_toml("test_unreferenced_delete")
-        .add_file(
-            "src/lib.rs",
-            r#"
-#[test]
-fn test_snapshot() {
-    insta::assert_snapshot!("Hello, world!");
-}
-"#
-            .to_string(),
-        )
-        .create_project();
-
-    // Run tests to create snapshots
-    let output = test_project
-        .insta_cmd()
-        .args(["test", "--accept"])
-        .output()
-        .unwrap();
-
-    assert!(&output.status.success());
-
-    // Manually add an unreferenced snapshot
-    let unreferenced_snapshot_path = test_project
-        .workspace_dir
-        .join("src/snapshots/test_unreferenced_delete__unused_snapshot.snap");
-    std::fs::create_dir_all(unreferenced_snapshot_path.parent().unwrap()).unwrap();
-    std::fs::write(
-        &unreferenced_snapshot_path,
-        r#"---
-source: src/lib.rs
-expression: "Unused snapshot"
----
-Unused snapshot
-"#,
-    )
-    .unwrap();
-
-    assert_snapshot!(test_project.file_tree_diff(), @r"
-    --- Original file tree
-    +++ Updated file tree
-    @@ -1,4 +1,8 @@
-     
-    +  Cargo.lock
-       Cargo.toml
-       src
-         src/lib.rs
-    +    src/snapshots
-    +      src/snapshots/test_unreferenced_delete__snapshot.snap
-    +      src/snapshots/test_unreferenced_delete__unused_snapshot.snap
-    ");
-
-    // Run cargo insta test with --unreferenced=delete
-    let output = test_project
-        .insta_cmd()
-        .args([
-            "test",
-            "--unreferenced=delete",
-            "--accept",
-            "--",
-            "--nocapture",
-        ])
-        .output()
-        .unwrap();
-
-    assert!(&output.status.success());
-
-    // We should now see the unreferenced snapshot deleted
-    assert_snapshot!(test_project.file_tree_diff(), @r"
-    --- Original file tree
-    +++ Updated file tree
-    @@ -1,4 +1,7 @@
-     
-    +  Cargo.lock
-       Cargo.toml
-       src
-         src/lib.rs
-    +    src/snapshots
-    +      src/snapshots/test_unreferenced_delete__snapshot.snap
-    ");
-}
-
 #[test]
 fn test_hidden_snapshots() {
     let test_project = TestFiles::new()
@@ -889,137 +804,3 @@ src/
         stderr
     );
 }
-
-#[test]
-fn test_unreferenced_config_reject() {
-    // This test verifies that the `test.unreferenced: reject` setting in insta.yaml
-    // is respected when no command-line argument is provided.
-    //
-    // Specifically, it tests the fix for issue #757, which ensures that:
-    // 1. Config file settings are properly applied when not overridden by command-line flags
-    // 2. Error handling for unreferenced snapshots properly updates the success flag
-    let test_project = TestFiles::new()
-        .add_cargo_toml("test_unreferenced_config_reject")
-        .add_file(
-            "src/lib.rs",
-            r#"
-#[test]
-fn test_snapshot() {
-    insta::assert_snapshot!("Hello, world!");
-}
-"#
-            .to_string(),
-        )
-        .create_project();
-
-    // Run tests to create snapshots first (without the config file)
-    let output = test_project
-        .insta_cmd()
-        .args(["test", "--accept"])
-        .output()
-        .unwrap();
-
-    assert!(output.status.success());
-
-    // Now add the config file after snapshot is created
-    test_project.update_file(
-        "insta.yaml",
-        r#"
-test:
-  unreferenced: reject
-"#
-        .to_string(),
-    );
-
-    // Manually add an unreferenced snapshot
-    let unreferenced_snapshot_path = test_project
-        .workspace_dir
-        .join("src/snapshots/test_unreferenced_config_reject__unused_snapshot.snap");
-    std::fs::create_dir_all(unreferenced_snapshot_path.parent().unwrap()).unwrap();
-    std::fs::write(
-        &unreferenced_snapshot_path,
-        r#"---
-source: src/lib.rs
-expression: "Unused snapshot"
----
-Unused snapshot
-"#,
-    )
-    .unwrap();
-
-    // Verify files exist
-    let snapshot_path = test_project
-        .workspace_dir
-        .join("src/snapshots/test_unreferenced_config_reject__snapshot.snap");
-    let unreferenced_path = test_project
-        .workspace_dir
-        .join("src/snapshots/test_unreferenced_config_reject__unused_snapshot.snap");
-
-    assert!(snapshot_path.exists(), "Normal snapshot file should exist");
-    assert!(
-        unreferenced_path.exists(),
-        "Unreferenced snapshot file should exist"
-    );
-
-    // First verify explicitly passing --unreferenced=reject does fail correctly
-    let output = test_project
-        .insta_cmd()
-        .args(["test", "--unreferenced=reject", "--", "--nocapture"])
-        .stderr(Stdio::piped())
-        .output()
-        .unwrap();
-
-    // The test should fail with explicit flag
-    assert!(
-        !output.status.success(),
-        "Command should fail with explicit --unreferenced=reject flag"
-    );
-
-    let stderr = String::from_utf8_lossy(&output.stderr);
-    assert!(
-        stderr.contains("encountered unreferenced snapshots"),
-        "Expected error message about unreferenced snapshots, got: {}",
-        stderr
-    );
-
-    // Now run without flags - this should also fail due to the config file setting
-    let output = test_project
-        .insta_cmd()
-        .args(["test", "--", "--nocapture"])
-        .stderr(Stdio::piped())
-        .output()
-        .unwrap();
-
-    // The command should fail because of the config file setting
-    assert!(
-        !output.status.success(),
-        "Command should fail when config file has test.unreferenced: reject"
-    );
-
-    // Verify the error message mentions unreferenced snapshots
-    let stderr = String::from_utf8_lossy(&output.stderr);
-    assert!(
-        stderr.contains("encountered unreferenced snapshots"),
-        "Expected error message about unreferenced snapshots, got: {}",
-        stderr
-    );
-
-    // Run with --unreferenced=delete to clean up
-    let output = test_project
-        .insta_cmd()
-        .args(["test", "--unreferenced=delete", "--", "--nocapture"])
-        .output()
-        .unwrap();
-
-    assert!(output.status.success());
-
-    // Verify the unreferenced snapshot was deleted
-    assert!(
-        snapshot_path.exists(),
-        "Normal snapshot file should still exist"
-    );
-    assert!(
-        !unreferenced_path.exists(),
-        "Unreferenced snapshot file should have been deleted"
-    );
-}
diff --git a/cargo-insta/tests/functional/unreferenced.rs b/cargo-insta/tests/functional/unreferenced.rs
new file mode 100644
index 0000000..d0c3e0d
--- /dev/null
+++ b/cargo-insta/tests/functional/unreferenced.rs
@@ -0,0 +1,353 @@
+use crate::TestFiles;
+use std::process::Stdio;
+
+/// This test verifies that tests with missing snapshots should fail when
+/// using --unreferenced=auto, and unreferenced snapshots should be preserved in this case
+#[test]
+fn test_unreferenced_auto_with_missing_snapshots() {
+    // Create a test project with a test that has no snapshot yet
+    let test_project = TestFiles::new()
+        .add_cargo_toml("test_unreferenced_missing")
+        .add_file(
+            "src/lib.rs",
+            r#"
+#[test]
+fn test_with_missing_snapshot() {
+    insta::assert_snapshot!("This has no snapshot yet");
+}
+"#
+            .to_string(),
+        )
+        .create_project();
+
+    // Manually add an unreferenced snapshot
+    let unreferenced_snapshot_path = test_project
+        .workspace_dir
+        .join("src/snapshots/test_unreferenced_missing__unused_snapshot.snap");
+    std::fs::create_dir_all(unreferenced_snapshot_path.parent().unwrap()).unwrap();
+    std::fs::write(
+        &unreferenced_snapshot_path,
+        r#"---
+    source: src/lib.rs
+    expression: "\"Unused snapshot\""
+    ---
+    Unused snapshot
+    "#,
+    )
+    .unwrap();
+
+    // Run test, which should fail since the snapshot is missing
+    // initially without --unreferenced=auto
+    assert!(!test_project
+        .insta_cmd()
+        .args(["test", "--", "--nocapture"])
+        .output()
+        .unwrap()
+        .status
+        .success());
+
+    // ...now with --unreferenced=auto
+    let output = test_project
+        .insta_cmd()
+        .args(["test", "--unreferenced=auto", "--", "--nocapture"])
+        .output()
+        .unwrap();
+
+    // Verify that the test run failed
+    assert!(
+        !output.status.success(),
+        "The test run should have failed due to missing snapshot"
+    );
+
+    // verify the unreferenced snapshot was not deleted when a test fails due to
+    // missing snapshots
+    assert!(
+        std::path::Path::new(&unreferenced_snapshot_path).exists(),
+        "The unreferenced snapshot should not be deleted when tests fail due to missing snapshots"
+    );
+}
+
+/// This test verifies that inline tests with missing snapshots should fail when
+/// using --unreferenced=auto, and unreferenced snapshots should be preserved in this case
+#[test]
+fn test_unreferenced_auto_with_missing_inline_snapshots() {
+    // Create a test project with a test that has no snapshot yet
+    let test_project = TestFiles::new()
+        .add_cargo_toml("test_unreferenced_missing_inline")
+        .add_file(
+            "src/lib.rs",
+            r#"
+#[test]
+fn test_with_missing_inline_snapshot() {
+    insta::assert_snapshot!("This has no inline snapshot yet", @"");
+}
+"#
+            .to_string(),
+        )
+        .create_project();
+
+    // Manually add an unreferenced snapshot
+    let unreferenced_snapshot_path = test_project
+        .workspace_dir
+        .join("src/snapshots/test_unreferenced_missing_inline__unused_snapshot.snap");
+    std::fs::create_dir_all(unreferenced_snapshot_path.parent().unwrap()).unwrap();
+    std::fs::write(
+        &unreferenced_snapshot_path,
+        r#"---
+    source: src/lib.rs
+    expression: "\"Unused snapshot\""
+    ---
+    Unused snapshot
+    "#,
+    )
+    .unwrap();
+
+    // Run test, which should fail since the inline snapshot is incorrect
+    // initially without --unreferenced=auto
+    assert!(!test_project
+        .insta_cmd()
+        .args(["test", "--", "--nocapture"])
+        .output()
+        .unwrap()
+        .status
+        .success());
+
+    // ...now with --unreferenced=auto
+    let output = test_project
+        .insta_cmd()
+        .args(["test", "--unreferenced=auto", "--", "--nocapture"])
+        .output()
+        .unwrap();
+
+    // Verify that the test run failed
+    assert!(
+        !output.status.success(),
+        "The test run should have failed due to incorrect inline snapshot"
+    );
+
+    // verify the unreferenced snapshot was not deleted when a test fails due to
+    // missing snapshots
+    assert!(
+        std::path::Path::new(&unreferenced_snapshot_path).exists(),
+        "The unreferenced snapshot should not be deleted when tests fail due to missing snapshots"
+    );
+}
+
+#[test]
+fn test_unreferenced_delete() {
+    let test_project = TestFiles::new()
+        .add_cargo_toml("test_unreferenced_delete")
+        .add_file(
+            "src/lib.rs",
+            r#"
+#[test]
+fn test_snapshot() {
+    insta::assert_snapshot!("Hello, world!");
+}
+"#
+            .to_string(),
+        )
+        .create_project();
+
+    // Run tests to create snapshots
+    let output = test_project
+        .insta_cmd()
+        .args(["test", "--accept"])
+        .output()
+        .unwrap();
+
+    assert!(&output.status.success());
+
+    // Manually add an unreferenced snapshot
+    let unreferenced_snapshot_path = test_project
+        .workspace_dir
+        .join("src/snapshots/test_unreferenced_delete__unused_snapshot.snap");
+    std::fs::create_dir_all(unreferenced_snapshot_path.parent().unwrap()).unwrap();
+    std::fs::write(
+        &unreferenced_snapshot_path,
+        r#"---
+source: src/lib.rs
+expression: "Unused snapshot"
+---
+Unused snapshot
+"#,
+    )
+    .unwrap();
+
+    insta::assert_snapshot!(test_project.file_tree_diff(), @r"
+    --- Original file tree
+    +++ Updated file tree
+    @@ -1,4 +1,8 @@
+     
+    +  Cargo.lock
+       Cargo.toml
+       src
+         src/lib.rs
+    +    src/snapshots
+    +      src/snapshots/test_unreferenced_delete__snapshot.snap
+    +      src/snapshots/test_unreferenced_delete__unused_snapshot.snap
+    ");
+
+    // Run cargo insta test with --unreferenced=delete
+    let output = test_project
+        .insta_cmd()
+        .args([
+            "test",
+            "--unreferenced=delete",
+            "--accept",
+            "--",
+            "--nocapture",
+        ])
+        .output()
+        .unwrap();
+
+    assert!(&output.status.success());
+
+    // We should now see the unreferenced snapshot deleted
+    insta::assert_snapshot!(test_project.file_tree_diff(), @r"
+    --- Original file tree
+    +++ Updated file tree
+    @@ -1,4 +1,7 @@
+     
+    +  Cargo.lock
+       Cargo.toml
+       src
+         src/lib.rs
+    +    src/snapshots
+    +      src/snapshots/test_unreferenced_delete__snapshot.snap
+    ");
+}
+
+#[test]
+fn test_unreferenced_config_reject() {
+    // This test verifies that the `test.unreferenced: reject` setting in insta.yaml
+    // is respected when no command-line argument is provided.
+    //
+    // Specifically, it tests the fix for issue #757, which ensures that:
+    // 1. Config file settings are properly applied when not overridden by command-line flags
+    // 2. Error handling for unreferenced snapshots properly updates the success flag
+    let test_project = TestFiles::new()
+        .add_cargo_toml("test_unreferenced_config_reject")
+        .add_file(
+            "src/lib.rs",
+            r#"
+#[test]
+fn test_snapshot() {
+    insta::assert_snapshot!("Hello, world!");
+}
+"#
+            .to_string(),
+        )
+        .create_project();
+
+    // Run tests to create snapshots first (without the config file)
+    let output = test_project
+        .insta_cmd()
+        .args(["test", "--accept"])
+        .output()
+        .unwrap();
+
+    assert!(output.status.success());
+
+    // Now add the config file after snapshot is created
+    test_project.update_file(
+        "insta.yaml",
+        r#"
+test:
+  unreferenced: reject
+"#
+        .to_string(),
+    );
+
+    // Manually add an unreferenced snapshot
+    let unreferenced_snapshot_path = test_project
+        .workspace_dir
+        .join("src/snapshots/test_unreferenced_config_reject__unused_snapshot.snap");
+    std::fs::create_dir_all(unreferenced_snapshot_path.parent().unwrap()).unwrap();
+    std::fs::write(
+        &unreferenced_snapshot_path,
+        r#"---
+source: src/lib.rs
+expression: "Unused snapshot"
+---
+Unused snapshot
+"#,
+    )
+    .unwrap();
+
+    // Verify files exist
+    let snapshot_path = test_project
+        .workspace_dir
+        .join("src/snapshots/test_unreferenced_config_reject__snapshot.snap");
+    let unreferenced_path = test_project
+        .workspace_dir
+        .join("src/snapshots/test_unreferenced_config_reject__unused_snapshot.snap");
+
+    assert!(snapshot_path.exists(), "Normal snapshot file should exist");
+    assert!(
+        unreferenced_path.exists(),
+        "Unreferenced snapshot file should exist"
+    );
+
+    // First verify explicitly passing --unreferenced=reject does fail correctly
+    let output = test_project
+        .insta_cmd()
+        .args(["test", "--unreferenced=reject", "--", "--nocapture"])
+        .stderr(Stdio::piped())
+        .output()
+        .unwrap();
+
+    // The test should fail with explicit flag
+    assert!(
+        !output.status.success(),
+        "Command should fail with explicit --unreferenced=reject flag"
+    );
+
+    let stderr = String::from_utf8_lossy(&output.stderr);
+    assert!(
+        stderr.contains("encountered unreferenced snapshots"),
+        "Expected error message about unreferenced snapshots, got: {}",
+        stderr
+    );
+
+    // Now run without flags - this should also fail due to the config file setting
+    let output = test_project
+        .insta_cmd()
+        .args(["test", "--", "--nocapture"])
+        .stderr(Stdio::piped())
+        .output()
+        .unwrap();
+
+    // The command should fail because of the config file setting
+    assert!(
+        !output.status.success(),
+        "Command should fail when config file has test.unreferenced: reject"
+    );
+
+    // Verify the error message mentions unreferenced snapshots
+    let stderr = String::from_utf8_lossy(&output.stderr);
+    assert!(
+        stderr.contains("encountered unreferenced snapshots"),
+        "Expected error message about unreferenced snapshots, got: {}",
+        stderr
+    );
+
+    // Run with --unreferenced=delete to clean up
+    let output = test_project
+        .insta_cmd()
+        .args(["test", "--unreferenced=delete", "--", "--nocapture"])
+        .output()
+        .unwrap();
+
+    assert!(output.status.success());
+
+    // Verify the unreferenced snapshot was deleted
+    assert!(
+        snapshot_path.exists(),
+        "Normal snapshot file should still exist"
+    );
+    assert!(
+        !unreferenced_path.exists(),
+        "Unreferenced snapshot file should have been deleted"
+    );
+}