Don't clear unrelated pending snapshots (mitsuhiko#651)

For discussion, re mitsuhiko#492.

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

My only reservation is whether I'm missing something? Otherwise the
proposed behavior seems better.

CHANGELOG.md

@@ -2,8 +2,12 @@
 
 All notable changes to insta and cargo-insta are documented here.
 
-## Unreleased
+## [Unreleased]
 
+- Pending snapshots are no longer removed throughout the workspace by
+  `cargo-insta` before running tests.  Instead, running a test will overwrite or
+  remove its own pending snapshot.  To remove all pending snapshots, use `cargo
+  insta reject` or run tests with `--unreferenced=delete`.  #651
 - `insta::internals::SettingsBindDropGuard` (returned from
   `Settings::bind_to_scope`) no longer implements `Send`. This was an error and
   any tests relying on this behavior where not working properly.

cargo-insta/src/cli.rs

@@ -190,8 +190,8 @@ struct TestCommand {
     /// Accept all new (previously unseen).
     #[arg(long)]
     accept_unseen: bool,
-    /// Do not reject pending snapshots before run.
-    #[arg(long)]
+    /// Do not reject pending snapshots before run (deprecated).
+    #[arg(long, hide = true)]
     keep_pending: bool,
     /// Update all snapshots even if they are still matching; implies `--accept`.
     #[arg(long)]
@@ -684,6 +684,12 @@ fn test_run(mut cmd: TestCommand, color: ColorWhen) -> Result<(), Box<dyn Error>
             style("warning").bold().yellow()
         )
     }
+    if cmd.keep_pending {
+        eprintln!(
+            "{}: `--keep-pending` is deprecated; its behavior is implied: pending snapshots are never removed before a test run.",
+            style("warning").bold().yellow()
+        )
+    }
 
     // the tool config can also indicate that --accept-unseen should be picked
     // automatically unless instructed otherwise.
@@ -714,10 +720,6 @@ fn test_run(mut cmd: TestCommand, color: ColorWhen) -> Result<(), Box<dyn Error>
     let (mut proc, snapshot_ref_file, prevents_doc_run) =
         prepare_test_runner(&cmd, test_runner, color, &[], None, &loc)?;
 
-    if !cmd.keep_pending {
-        process_snapshots(true, None, &loc, Some(Operation::Reject))?;
-    }
-
     if let Some(workspace_root) = &cmd.target_args.workspace_root {
         proc.current_dir(workspace_root);
     }
@@ -847,26 +849,31 @@ fn handle_unreferenced_snapshots(
     for package in loc.packages.clone() {
         let unreferenced_snapshots = make_snapshot_walker(
             package.manifest_path.parent().unwrap().as_std_path(),
-            &["snap"],
+            &loc.exts,
             FindFlags {
                 include_ignored: true,
                 include_hidden: true,
             },
         )
         .filter_map(|e| e.ok())
         .filter(|e| e.file_type().map(|ft| ft.is_file()).unwrap_or(false))
-        .filter(|e| {
-            e.file_name()
-                .to_str()
-                .map(|name| {
-                    loc.exts
-                        .iter()
-                        .any(|ext| name.ends_with(&format!(".{}", ext)))
-                })
-                .unwrap_or(false)
-        })
         .filter_map(|e| e.path().canonicalize().ok())
-        .filter(|path| !files.contains(path));
+        // 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| {
+            // 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)
+        });
 
         for path in unreferenced_snapshots {
             if !encountered_any {
@@ -885,19 +892,27 @@ fn handle_unreferenced_snapshots(
             }
             eprintln!("  {}", path.display());
             if matches!(action, Action::Delete) {
-                let snapshot = match Snapshot::from_file(&path) {
-                    Ok(snapshot) => snapshot,
-                    Err(e) => {
-                        eprintln!("Error loading snapshot at {:?}: {}", &path, e);
-                        continue;
+                // If it's an inline pending snapshot, then don't attempt to
+                // load it, since these are in a different format; just delete
+                if path.extension() == Some(std::ffi::OsStr::new("pending-snap")) {
+                    if let Err(e) = fs::remove_file(path) {
+                        eprintln!("Failed to remove file: {}", e);
                     }
-                };
+                } else {
+                    let snapshot = match Snapshot::from_file(&path) {
+                        Ok(snapshot) => snapshot,
+                        Err(e) => {
+                            eprintln!("Error loading snapshot at {:?}: {}", &path, e);
+                            continue;
+                        }
+                    };
 
-                if let Some(binary_path) = snapshot.build_binary_path(&path) {
-                    fs::remove_file(&binary_path).ok();
-                }
+                    if let Some(binary_path) = snapshot.build_binary_path(&path) {
+                        fs::remove_file(&binary_path).ok();
+                    }
 
-                fs::remove_file(&path).ok();
+                    fs::remove_file(&path).ok();
+                }
             }
         }
     }

cargo-insta/tests/functional/binary.rs

@@ -177,15 +177,23 @@ fn test_binary_snapshot() {
         )
         .create_project();
 
-    let output = test_project.insta_cmd().args(["test"]).output().unwrap();
-
-    assert!(!&output.status.success());
+    assert!(!&test_project
+        .insta_cmd()
+        .args(["test"])
+        .output()
+        .unwrap()
+        .status
+        .success());
 
     test_project.update_file("src/lib.rs", "".to_string());
 
-    let output = test_project.insta_cmd().args(["test"]).output().unwrap();
-
-    assert!(&output.status.success());
+    assert!(&test_project
+        .insta_cmd()
+        .args(["test", "--unreferenced=delete"])
+        .output()
+        .unwrap()
+        .status
+        .success());
 
     assert_snapshot!(test_project.file_tree_diff(), @r"
     --- Original file tree