Repository: mitsuhiko/insta
Pull request #626 (2024-10-01 04:22:06)

Canonicalize snapshot paths before reducing
Fixes https://github.com/mitsuhiko/insta/issues/625

As ever, the reason is a bit different from the one I initially understood. It's actually that we're reducing the paths by whether one prefixes the other. But in this case the path is only a prefix before it's canonicalized!

On the upside, this case will now work without `--workspace-root`.


max-sixty: (requires a test before merging)

Patch:
diff --git a/cargo-insta/Cargo.toml b/cargo-insta/Cargo.toml
index 06954a7..2258d64 100644
--- a/cargo-insta/Cargo.toml
+++ b/cargo-insta/Cargo.toml
@@ -30,6 +30,7 @@ tempfile = "3.5.0"
 semver = {version = "1.0.7", features = ["serde"]}
 lazy_static = "1.4.0"
 clap = { workspace=true }
+itertools = "0.10.0"
 
 [dev-dependencies]
 walkdir = "2.3.1"
diff --git a/cargo-insta/src/cargo.rs b/cargo-insta/src/cargo.rs
index 15e2f07..7e15f89 100644
--- a/cargo-insta/src/cargo.rs
+++ b/cargo-insta/src/cargo.rs
@@ -1,9 +1,11 @@
 use std::path::PathBuf;
 
 pub(crate) use cargo_metadata::Package;
+use itertools::Itertools;
 
 /// Find snapshot roots within a package
-// (I'm not sure how necessary this is; relative to just using all paths?)
+// 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();
 
@@ -38,13 +40,19 @@ pub(crate) fn find_snapshot_roots(package: &Package) -> Vec<PathBuf> {
     // we would encounter paths twice.  If we don't skip them here we run
     // into issues where the existence of a build script causes a snapshot
     // to be picked up twice since the same path is determined.  (GH-15)
-    roots.sort_by_key(|x| x.as_os_str().len());
-    let mut reduced_roots = vec![];
-    for root in roots {
-        if !reduced_roots.iter().any(|x| root.starts_with(x)) {
-            reduced_roots.push(root);
-        }
-    }
-
-    reduced_roots
+    let canonical_roots: Vec<_> = roots
+        .iter()
+        .filter_map(|x| x.canonicalize().ok())
+        .sorted_by_key(|x| x.as_os_str().len())
+        .collect();
+
+    canonical_roots
+        .clone()
+        .into_iter()
+        .filter(|root| {
+            !canonical_roots
+                .iter()
+                .any(|x| root.starts_with(x) && root != x)
+        })
+        .collect()
 }


Test patch:
diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs
index 37e4dd6..e96cd08 100644
--- a/cargo-insta/tests/main.rs
+++ b/cargo-insta/tests/main.rs
@@ -1303,3 +1303,116 @@ fn test_insta_workspace_root() {
         Some(("INSTA_WORKSPACE_ROOT", moved_workspace.to_str().unwrap())),
     ));
 }
+
+#[test]
+fn test_external_test_path() {
+    let test_project = TestFiles::new()
+        .add_file(
+            "proj/Cargo.toml",
+            r#"
+[package]
+name = "external_test_path"
+version = "0.1.0"
+edition = "2021"
+
+[dependencies]
+insta = { path = '$PROJECT_PATH' }
+
+[[test]]
+name = "tlib"
+path = "../tests/lib.rs"
+"#
+            .to_string(),
+        )
+        .add_file(
+            "proj/src/lib.rs",
+            r#"
+pub fn hello() -> String {
+    "Hello, world!".to_string()
+}
+"#
+            .to_string(),
+        )
+        .add_file(
+            "tests/lib.rs",
+            r#"
+use external_test_path::hello;
+
+#[test]
+fn test_hello() {
+    insta::assert_snapshot!(hello());
+}
+"#
+            .to_string(),
+        )
+        .create_project();
+
+    // Change to the proj directory for running cargo commands
+    let proj_dir = test_project.workspace_dir.join("proj");
+
+    // Initially, the test should fail
+    let output = test_project
+        .cmd()
+        .current_dir(&proj_dir)
+        .args(["test", "--"])
+        .output()
+        .unwrap();
+
+    assert_failure(&output);
+
+    // Verify that the snapshot was created in the correct location
+    assert_snapshot!(TestProject::current_file_tree(&test_project.workspace_dir), @r"
+    proj
+      proj/Cargo.lock
+      proj/Cargo.toml
+      proj/src
+        proj/src/lib.rs
+    tests
+      tests/lib.rs
+      tests/snapshots
+        tests/snapshots/tlib__hello.snap.new
+    ");
+
+    // Run cargo insta accept
+    let output = test_project
+        .cmd()
+        .current_dir(&proj_dir)
+        .args(["test", "--accept"])
+        .output()
+        .unwrap();
+
+    assert_success(&output);
+
+    // Verify that the snapshot was created in the correct location
+    assert_snapshot!(TestProject::current_file_tree(&test_project.workspace_dir), @r"
+    proj
+      proj/Cargo.lock
+      proj/Cargo.toml
+      proj/src
+        proj/src/lib.rs
+    tests
+      tests/lib.rs
+      tests/snapshots
+        tests/snapshots/tlib__hello.snap
+    ");
+
+    // Run the test again, it should pass now
+    let output = Command::new(env!("CARGO_BIN_EXE_cargo-insta"))
+        .current_dir(&proj_dir)
+        .args(["test"])
+        .output()
+        .unwrap();
+
+    assert_success(&output);
+
+    let snapshot_path = test_project
+        .workspace_dir
+        .join("tests/snapshots/tlib__hello.snap");
+    assert_snapshot!(fs::read_to_string(snapshot_path).unwrap(), @r#"
+    ---
+    source: "../tests/lib.rs"
+    expression: hello()
+    ---
+    Hello, world!
+    "#);
+}