Repository: mitsuhiko/insta (rust)

Issue:
Insta 1.41.0 finds spurious duplicate snapshots from tests in package root
### What happened?

When insta is used in tests located in the package root (specified via the `[[test]]` Cargo.toml key), the CLI will find each generated snapshot twice, even though only one file is generated per snapshot.

### Reproduction steps

The following is a minimal workspace setup to reproduce the issue:

```sh
$ ls
Cargo.toml test.rs
```
`Cargo.toml`:
```toml
[package]
name = "insta-testing"
version = "0.1.0"
edition = "2021"

[dev-dependencies]
insta = "=1.41.0"

[[test]]
name = "test"
path = "test.rs"
```

`test.rs`:
```rust
#[test]
fn some_test() {
    insta::assert_snapshot!("name", "content");
}
```

`cargo insta test` yields the following output and resulting workspace state:

```sh
$ cargo insta test
    Finished `test` profile [unoptimized + debuginfo] target(s) in 1.40s
     Running test.rs (target/debug/deps/test-68ebf84566e4320f)

running 1 test
stored new snapshot /home/julia/code/personal/insta-testing/snapshots/test__name.snap.new
test some_test ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.04s

info: 2 snapshots to review
use `cargo insta review` to review snapshots
$ tree -I target
.
├── Cargo.lock
├── Cargo.toml
├── snapshots
│   └── test__name.snap.new
└── test.rs

2 directories, 4 files
```

Running `cargo insta review` at this point will present the same snapshot twice. `cargo insta pending-snapshots` will list the same snap.new file twice. Running `cargo insta accept` will complain (once) about the snap.new file having been removed by another process, presumably upon failing to find it again after accepting and removing it the first time.

### Insta Version

1.41.0

### rustc Version

1.82.0

### What did you expect?

I would have expected the snapshot to be found by the CLI only once; only reported as one snapshot from `insta test` and `insta pending-snapshots`, only presented once by `insta review`, only removed once by `insta accept`, etc.

Prior versions of insta display the expected behavior; setting the insta dependency version to `=1.40.0` and installing `cargo-insta@1.40.0` yields the expected behavior.

As far as I've been able to tell, this behavior only occurs when the test is in the package root directory. For instance, it does _not_ occur (i.e. the expected behavior occurs) if `test.rs` is in the default `tests` directory, or if it's in a different subdirectory specified via the `[[test]]` key. However, changing the snapshot directory via `with_settings!` (or manually via `Settings::bind`) does not affect the behavior; it seems to depend only on the location of the test itself.

Pull request:
Fix duplicate snapshots on multiple targets with identical roots
Closes #678


Patch:
diff --git a/cargo-insta/src/cargo.rs b/cargo-insta/src/cargo.rs
index 7e15f89..61a639f 100644
--- a/cargo-insta/src/cargo.rs
+++ b/cargo-insta/src/cargo.rs
@@ -7,21 +7,18 @@ use itertools::Itertools;
 // We need this because paths are not always conventional — for example cargo
 // can reference artifacts that are outside of the package root.
 pub(crate) fn find_snapshot_roots(package: &Package) -> Vec<PathBuf> {
-    let mut roots = Vec::new();
+    let mut roots = std::collections::HashSet::new();
 
     // the manifest path's parent is always a snapshot container.  For
     // a rationale see GH-70.  But generally a user would expect to be
     // able to put a snapshot into foo/snapshots instead of foo/src/snapshots.
     if let Some(manifest) = package.manifest_path.parent() {
-        roots.push(manifest.as_std_path().to_path_buf());
+        roots.insert(manifest.as_std_path().to_path_buf());
     }
 
     // additionally check all targets.
     for target in &package.targets {
-        // custom build scripts we can safely skip over.  In the past this
-        // caused issues with duplicate paths but that's resolved in other
-        // ways now.  We do not want to pick up snapshots in such places
-        // though.
+        // custom build scripts we can safely skip over.
         if target.kind.iter().any(|kind| kind == "custom-build") {
             continue;
         }
@@ -29,9 +26,12 @@ pub(crate) fn find_snapshot_roots(package: &Package) -> Vec<PathBuf> {
         // this gives us the containing source folder.  Typically this is
         // something like crate/src.
         let root = target.src_path.parent().unwrap().as_std_path();
-        roots.push(root.to_path_buf());
+        roots.insert(root.to_path_buf());
     }
 
+    // Convert HashSet back to Vec for the rest of the function
+    let roots: Vec<_> = roots.into_iter().collect();
+
     // TODO: I think this root reduction is duplicative over the logic in
     // `make_snapshot_walker`; could try removing.
 


Test patch:
diff --git a/cargo-insta/tests/functional/main.rs b/cargo-insta/tests/functional/main.rs
index 39de30e..d67f1dd 100644
--- a/cargo-insta/tests/functional/main.rs
+++ b/cargo-insta/tests/functional/main.rs
@@ -1,6 +1,10 @@
-/// Integration tests which allow creating a full repo, running `cargo-insta`
+/// # Functional tests
+///
+/// Functional tests which allow creating a full repo, running `cargo-insta`
 /// and then checking the output.
 ///
+/// ## Capturing output
+///
 /// By default, the output of the inner test is forwarded to the outer test with
 /// a colored prefix. If we want to assert the inner test contains some output,
 /// we need to disable that forwarding with `Stdio::piped()` like:
@@ -18,38 +22,43 @@
 /// );
 /// ```
 ///
-/// Often we want to see output from the test commands we run here; for example
-/// a `dbg` statement we add while debugging. Cargo by default hides the output
-/// of passing tests.
+/// ## Showing output of passing tests
+///
+/// Cargo by default shows the output of failing tests but hides the output of
+/// passing tests. Often we want to see output from the test commands we run
+/// here; for example a `dbg` statement we add while debugging.
 /// - Like any test, to forward the output of a passing outer test (i.e. one of
 ///   the `#[test]`s in this file) to the terminal, pass `--nocapture` to the
 ///   test runner, like `cargo insta test -- --nocapture`.
 /// - To forward the output of a passing inner test (i.e. the test commands we
-///   create and run within an outer test) to the output of an outer test, pass
+///   create and run within the code here) to the output of an outer test, pass
 ///   `--nocapture` in the command we create; for example `.args(["test",
 ///   "--accept", "--", "--nocapture"])`.
-///   - We also need to pass `--nocapture` to the outer test to forward that to
-///     the terminal, per the previous bullet.
+///   - Consistent with the previous bullet, If the outer test is passing we
+///     also need to pass `--nocapture` to the outer test in order to forward
+///     that to the terminal
+///
+/// ## Package names
 ///
 /// Note that the packages must have different names, or we'll see interference
-/// between the tests.
+/// between the tests[^1].
+///
+/// [1]: That seems to be because they all share the same `target` directory, which
+///      cargo will confuse for each other if they share the same name. I haven't
+///      worked out why — this is the case even if the files are the same between
+///      two tests but with different commands — and those files exist in different
+///      temporary workspace dirs. (We could try to enforce different names, or
+///      give up using a consistent target directory for a cache, but it would slow
+///      down repeatedly running the tests locally. To demonstrate the effect, name
+///      crates the same... This also causes issues when running the same tests
+///      concurrently.
 ///
-/// > That seems to be because they all share the same `target` directory, which
-/// > cargo will confuse for each other if they share the same name. I haven't
-/// > worked out why — this is the case even if the files are the same between
-/// > two tests but with different commands — and those files exist in different
-/// > temporary workspace dirs. (We could try to enforce different names, or
-/// > give up using a consistent target directory for a cache, but it would slow
-/// > down repeatedly running the tests locally. To demonstrate the effect, name
-/// > crates the same... This also causes issues when running the same tests
-/// > concurrently.
 use std::collections::HashMap;
 use std::env;
 use std::fs;
 use std::io::{BufRead, BufReader};
 use std::path::{Path, PathBuf};
-use std::process::Command;
-use std::process::Stdio;
+use std::process::{Command, Stdio};
 use std::thread;
 
 use console::style;
diff --git a/cargo-insta/tests/functional/workspace.rs b/cargo-insta/tests/functional/workspace.rs
index e05c9e0..f2b40d3 100644
--- a/cargo-insta/tests/functional/workspace.rs
+++ b/cargo-insta/tests/functional/workspace.rs
@@ -821,6 +821,67 @@ fn test_inline() {
     ");
 }
 
+/// Test for issue #678: Insta 1.41.0 finds spurious duplicate snapshots from tests in package root
+#[test]
+fn test_root_test_duplicate_snapshots() {
+    let test_project = TestFiles::new()
+        .add_file(
+            "Cargo.toml",
+            r#"
+[package]
+name = "root_test_duplicate_snapshots"
+version = "0.1.0"
+edition = "2021"
+
+[dependencies]
+insta = { path = '$PROJECT_PATH' }
+
+[[test]]
+name = "root_test"
+path = "root_test.rs"
+"#
+            .to_string(),
+        )
+        .add_file(
+            "root_test.rs",
+            r#"
+#[test]
+fn test_in_root() {
+    insta::assert_snapshot!("name", "content");
+}
+"#
+            .to_string(),
+        )
+        .create_project();
+
+    // Run the test to create the snapshot
+    let output = test_project
+        .insta_cmd()
+        .args(["test"])
+        .stderr(Stdio::piped())
+        .output()
+        .unwrap();
+
+    // Assert that we only have one snapshot file
+    assert_snapshot!(test_project.file_tree_diff(), @r"
+    --- Original file tree
+    +++ Updated file tree
+    @@ -1,3 +1,6 @@
+     
+    +  Cargo.lock
+       Cargo.toml
+       root_test.rs
+    +  snapshots
+    +    snapshots/root_test__name.snap.new
+    ");
+
+    // Check if the output reports 2 snapshots instead of 1
+    let stderr = String::from_utf8_lossy(&output.stderr);
+
+    // There should only be 1 snapshot
+    assert!(stderr.contains("1 snapshot to review"), "\n\n{}", stderr);
+}
+
 /// Check that `--manifest` points `cargo-insta` at another path
 #[test]
 fn test_manifest_option() {