From 1d9ddeaf3b8fda5e0c3417072b23b59e9d976603 Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Sat, 4 Jan 2025 15:06:43 -0800 Subject: [PATCH] Don't clear unrelated pending snapshots (#651) For discussion, re #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 | 6 +- cargo-insta/src/cli.rs | 71 +++-- cargo-insta/tests/functional/binary.rs | 20 +- .../tests/functional/delete_pending.rs | 253 ++++++++++++++++++ cargo-insta/tests/functional/main.rs | 1 + 5 files changed, 316 insertions(+), 35 deletions(-) create mode 100644 cargo-insta/tests/functional/delete_pending.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 8786cb2a..4305e18c 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/cargo-insta/src/cli.rs b/cargo-insta/src/cli.rs index e43ff03c..62c92fc1 100644 --- a/cargo-insta/src/cli.rs +++ b/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 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 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,7 +849,7 @@ 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, @@ -855,18 +857,23 @@ fn handle_unreferenced_snapshots( ) .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(); + } } } } diff --git a/cargo-insta/tests/functional/binary.rs b/cargo-insta/tests/functional/binary.rs index 5440c79b..e099478c 100644 --- a/cargo-insta/tests/functional/binary.rs +++ b/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 diff --git a/cargo-insta/tests/functional/delete_pending.rs b/cargo-insta/tests/functional/delete_pending.rs new file mode 100644 index 00000000..34a4d95a --- /dev/null +++ b/cargo-insta/tests/functional/delete_pending.rs @@ -0,0 +1,253 @@ +use insta::assert_snapshot; + +use crate::TestFiles; + +/// `--unreferenced=delete` should delete pending snapshots +#[test] +fn delete_unreferenced() { + let test_project = TestFiles::new() + .add_cargo_toml("delete_unreferenced") + .add_file( + "src/lib.rs", + r#" +#[test] +fn test_snapshot() { + insta::assert_snapshot!("Hello, inline!", @"Hello!"); +} + +#[test] +fn test_snapshot_file() { + insta::assert_snapshot!("Hello, world!"); +} +"# + .to_string(), + ) + .create_project(); + + assert!(!&test_project + .insta_cmd() + .args(["test", "--", "--nocapture"]) + .output() + .unwrap() + .status + .success()); + + 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.pending-snap + src/lib.rs + + src/snapshots + + src/snapshots/delete_unreferenced__snapshot_file.snap.new + "); + + // Now remove the tests; the pending snapshots should be deleted when + // passing `--unreferenced=delete` + test_project.update_file("src/lib.rs", "".to_string()); + + assert!(&test_project + .insta_cmd() + .args(["test", "--unreferenced=delete", "--", "--nocapture"]) + .output() + .unwrap() + .status + .success()); + + assert_snapshot!(test_project.file_tree_diff(), @r" + --- Original file tree + +++ Updated file tree + @@ -1,4 +1,6 @@ + + + Cargo.lock + Cargo.toml + src + src/lib.rs + + src/snapshots + "); +} + +#[test] +fn test_pending_snapshot_deletion() { + let test_project = TestFiles::new() + .add_cargo_toml("test_combined_snapshot_deletion") + .add_file( + "src/lib.rs", + r#" +use insta::assert_snapshot; + +#[test] +fn test_inline_snapshot() { + insta::assert_snapshot!("Hello, world!", @"Hello!"); +} + +#[test] +fn test_file_snapshot() { + assert_snapshot!("hello world"); +} +"# + .to_string(), + ) + .create_project(); + + // Run the test with `--accept` to create correct snapshots + assert!(test_project + .insta_cmd() + .args(["test", "--accept", "--", "--nocapture"]) + .output() + .unwrap() + .status + .success()); + + // Verify the file tree has a `.snap` file + 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_combined_snapshot_deletion__file_snapshot.snap + "); + + // Modify the tests to make them fail + test_project.update_file( + "src/lib.rs", + r#" +use insta::assert_snapshot; + +#[test] +fn test_inline_snapshot() { + insta::assert_snapshot!("Hello WORLD!", @"Hello, world!"); +} + +#[test] +fn test_file_snapshot() { + assert_snapshot!("hello WORLD"); +} +"# + .to_string(), + ); + + // Run the tests to create pending snapshots + assert!(!test_project + .insta_cmd() + .args(["test"]) + .output() + .unwrap() + .status + .success()); + + // Verify pending snapshots exist + assert_snapshot!(test_project.file_tree_diff(), @r" + --- Original file tree + +++ Updated file tree + @@ -1,4 +1,9 @@ + + + Cargo.lock + Cargo.toml + src + + src/.lib.rs.pending-snap + src/lib.rs + + src/snapshots + + src/snapshots/test_combined_snapshot_deletion__file_snapshot.snap + + src/snapshots/test_combined_snapshot_deletion__file_snapshot.snap.new + "); + + // Run `cargo insta reject` to delete pending snapshots + assert!(test_project + .insta_cmd() + .args(["reject"]) + .output() + .unwrap() + .status + .success()); + + // Pending snapshots should be 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_combined_snapshot_deletion__file_snapshot.snap + "); + + // Run the tests again to create pending snapshots + assert!(!test_project + .insta_cmd() + .args(["test"]) + .output() + .unwrap() + .status + .success()); + + // They should be back... + assert_snapshot!(test_project.file_tree_diff(), @r" + --- Original file tree + +++ Updated file tree + @@ -1,4 +1,9 @@ + + + Cargo.lock + Cargo.toml + src + + src/.lib.rs.pending-snap + src/lib.rs + + src/snapshots + + src/snapshots/test_combined_snapshot_deletion__file_snapshot.snap + + src/snapshots/test_combined_snapshot_deletion__file_snapshot.snap.new + "); + + // Modify the test back so they pass + test_project.update_file( + "src/lib.rs", + r#" +use insta::assert_snapshot; + +#[test] +fn test_inline_snapshot() { + insta::assert_snapshot!("Hello, world!", @"Hello, world!"); +} + +#[test] +fn test_file_snapshot() { + assert_snapshot!("hello world"); +} +"# + .to_string(), + ); + + // Run the tests with `--unreferenced=delete` to delete pending snapshots + assert!(test_project + .insta_cmd() + .args(["test", "--unreferenced=delete"]) + .output() + .unwrap() + .status + .success()); + + // Verify the pending snapshots are 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_combined_snapshot_deletion__file_snapshot.snap + "); +} diff --git a/cargo-insta/tests/functional/main.rs b/cargo-insta/tests/functional/main.rs index 59856fc5..ccb43206 100644 --- a/cargo-insta/tests/functional/main.rs +++ b/cargo-insta/tests/functional/main.rs @@ -60,6 +60,7 @@ use similar::udiff::unified_diff; use tempfile::TempDir; mod binary; +mod delete_pending; mod inline; mod workspace;