fix --unreferenced=auto clearing new pending snapshots (#762)

closes #751

Author: max-sixty

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

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 {

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

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

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"
-    );
-}