From 646031f8e658456d1970b80793d86fc019bf0f89 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Mon, 30 Sep 2024 21:22:02 -0700 Subject: [PATCH 1/2] 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 described there; 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`. --- cargo-insta/Cargo.toml | 1 + cargo-insta/src/cargo.rs | 28 ++++++++++++++++++---------- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/cargo-insta/Cargo.toml b/cargo-insta/Cargo.toml index 06954a72..2258d64f 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 15e2f07f..7e15f890 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 { let mut roots = Vec::new(); @@ -38,13 +40,19 @@ pub(crate) fn find_snapshot_roots(package: &Package) -> Vec { // 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() } From fb02fb67affcd2b2bd9cddb1a7d5d0a9657ce9c0 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Mon, 30 Sep 2024 23:01:20 -0700 Subject: [PATCH 2/2] test --- cargo-insta/tests/main.rs | 113 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index 37e4dd66..e96cd083 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! + "#); +}