Consolidate onto INSTA_WORKSPACE_ROOT (#614)

Intended to fix #575. Though after some iterations, it ends up being a
code-clean-up. I'm not sure how `CARGO_MANIFEST_DIR` is leaking through
— it's only referenced by `env!` rather than `env::var`, so should only
matter at compile time.

Added a pretty thorough integration test (albeit maybe a bit too
complicated)

cargo-insta/tests/main.rs

@@ -27,10 +27,10 @@
 /// the same...). This also causes issues when running the same tests
 /// concurrently.
 use std::collections::HashMap;
-use std::env;
 use std::fs;
 use std::path::{Path, PathBuf};
 use std::process::Command;
+use std::{env, fs::remove_dir_all};
 
 use console::style;
 use ignore::WalkBuilder;
@@ -102,6 +102,16 @@ fn assert_success(output: &std::process::Output) {
     eprint!("{}", stderr);
 }
 
+fn assert_failure(output: &std::process::Output) {
+    eprint!("{}", String::from_utf8_lossy(&output.stderr));
+    eprint!("{}", String::from_utf8_lossy(&output.stdout));
+    assert!(
+        !output.status.success(),
+        "Tests unexpectedly succeeded: {}\n{}",
+        String::from_utf8_lossy(&output.stdout),
+        String::from_utf8_lossy(&output.stderr)
+    );
+}
 struct TestProject {
     /// Temporary directory where the project is created
     workspace_dir: PathBuf,
@@ -131,23 +141,26 @@ impl TestProject {
             workspace_dir,
         }
     }
-    fn cmd(&self) -> Command {
-        let mut command = Command::new(env!("CARGO_BIN_EXE_cargo-insta"));
+    fn clean_env(cmd: &mut Command) {
         // Remove environment variables so we don't inherit anything (such as
         // `INSTA_FORCE_PASS` or `CARGO_INSTA_*`) from a cargo-insta process
         // which runs this integration test.
         for (key, _) in env::vars() {
             if key.starts_with("CARGO_INSTA") || key.starts_with("INSTA") {
-                command.env_remove(&key);
+                cmd.env_remove(&key);
             }
         }
         // Turn off CI flag so that cargo insta test behaves as we expect
         // under normal operation
-        command.env("CI", "0");
+        cmd.env("CI", "0");
         // And any others that can affect the output
-        command.env_remove("CARGO_TERM_COLOR");
-        command.env_remove("CLICOLOR_FORCE");
-        command.env_remove("RUSTDOCFLAGS");
+        cmd.env_remove("CARGO_TERM_COLOR");
+        cmd.env_remove("CLICOLOR_FORCE");
+        cmd.env_remove("RUSTDOCFLAGS");
+    }
+    fn cmd(&self) -> Command {
+        let mut command = Command::new(env!("CARGO_BIN_EXE_cargo-insta"));
+        Self::clean_env(&mut command);
 
         command.current_dir(self.workspace_dir.as_path());
         // Use the same target directory as other tests, consistent across test
@@ -1166,3 +1179,127 @@ fn test_hashtag_escape() {
      }
     "####);
 }
+
+// Can't get the test binary discovery to work, don't have a windows machine to
+// hand, others are welcome to fix it. (No specific reason to think that insta
+// doesn't work on windows, just that the test doesn't work.)
+#[cfg(not(target_os = "windows"))]
+#[test]
+fn test_insta_workspace_root() {
+    // This function locates the compiled test binary in the target directory.
+    // It's necessary because the exact filename of the test binary includes a hash
+    // that we can't predict, so we need to search for it.
+    fn find_test_binary(dir: &Path) -> PathBuf {
+        dir.join("target/debug/deps")
+            .read_dir()
+            .unwrap()
+            .filter_map(Result::ok)
+            .find(|entry| {
+                let file_name = entry.file_name();
+                let file_name_str = file_name.to_str().unwrap_or("");
+                // We're looking for a file that:
+                file_name_str.starts_with("insta_workspace_root_test-") // Matches our test name
+                    && !file_name_str.contains('.') // Doesn't have an extension (it's the executable, not a metadata file)
+                    && entry.metadata().map(|m| m.is_file()).unwrap_or(false) // Is a file, not a directory
+            })
+            .map(|entry| entry.path())
+            .expect("Failed to find test binary")
+    }
+
+    fn run_test_binary(
+        binary_path: &Path,
+        current_dir: &Path,
+        env: Option<(&str, &str)>,
+    ) -> std::process::Output {
+        let mut cmd = Command::new(binary_path);
+        TestProject::clean_env(&mut cmd);
+        cmd.current_dir(current_dir);
+        if let Some((key, value)) = env {
+            cmd.env(key, value);
+        }
+        cmd.output().unwrap()
+    }
+
+    let test_project = TestFiles::new()
+        .add_file(
+            "Cargo.toml",
+            r#"
+    [package]
+    name = "insta_workspace_root_test"
+    version = "0.1.0"
+    edition = "2021"
+
+    [dependencies]
+    insta = { path = '$PROJECT_PATH' }
+    "#
+            .to_string(),
+        )
+        .add_file(
+            "src/lib.rs",
+            r#"
+    #[cfg(test)]
+    mod tests {
+        use insta::assert_snapshot;
+
+        #[test]
+        fn test_snapshot() {
+            assert_snapshot!("Hello, world!");
+        }
+    }
+    "#
+            .to_string(),
+        )
+        .create_project();
+
+    let mut cargo_cmd = Command::new("cargo");
+    TestProject::clean_env(&mut cargo_cmd);
+    let output = cargo_cmd
+        .args(["test", "--no-run"])
+        .current_dir(&test_project.workspace_dir)
+        .output()
+        .unwrap();
+    assert_success(&output);
+
+    let test_binary_path = find_test_binary(&test_project.workspace_dir);
+
+    // Run the test without snapshot (should fail)
+    assert_failure(&run_test_binary(
+        &test_binary_path,
+        &test_project.workspace_dir,
+        None,
+    ));
+
+    // Create the snapshot
+    assert_success(&run_test_binary(
+        &test_binary_path,
+        &test_project.workspace_dir,
+        Some(("INSTA_UPDATE", "always")),
+    ));
+
+    // Verify snapshot creation
+    assert!(test_project.workspace_dir.join("src/snapshots").exists());
+    assert!(test_project
+        .workspace_dir
+        .join("src/snapshots/insta_workspace_root_test__tests__snapshot.snap")
+        .exists());
+
+    // Move the workspace
+    let moved_workspace = {
+        let moved_workspace = PathBuf::from("/tmp/cargo-insta-test-moved");
+        remove_dir_all(&moved_workspace).ok();
+        fs::create_dir(&moved_workspace).unwrap();
+        fs::rename(&test_project.workspace_dir, &moved_workspace).unwrap();
+        moved_workspace
+    };
+    let moved_binary_path = find_test_binary(&moved_workspace);
+
+    // Run test in moved workspace without INSTA_WORKSPACE_ROOT (should fail)
+    assert_failure(&run_test_binary(&moved_binary_path, &moved_workspace, None));
+
+    // Run test in moved workspace with INSTA_WORKSPACE_ROOT (should pass)
+    assert_success(&run_test_binary(
+        &moved_binary_path,
+        &moved_workspace,
+        Some(("INSTA_WORKSPACE_ROOT", moved_workspace.to_str().unwrap())),
+    ));
+}

insta/src/env.rs

@@ -12,18 +12,16 @@ use crate::{
 
 lazy_static::lazy_static! {
     static ref WORKSPACES: Mutex<BTreeMap<String, Arc<PathBuf>>> = Mutex::new(BTreeMap::new());
-    static ref TOOL_CONFIGS: Mutex<BTreeMap<String, Arc<ToolConfig>>> = Mutex::new(BTreeMap::new());
+    static ref TOOL_CONFIGS: Mutex<BTreeMap<PathBuf, Arc<ToolConfig>>> = Mutex::new(BTreeMap::new());
 }
 
-pub fn get_tool_config(manifest_dir: &str) -> Arc<ToolConfig> {
-    let mut configs = TOOL_CONFIGS.lock().unwrap();
-    if let Some(rv) = configs.get(manifest_dir) {
-        return rv.clone();
-    }
-    let config =
-        Arc::new(ToolConfig::from_manifest_dir(manifest_dir).expect("failed to load tool config"));
-    configs.insert(manifest_dir.to_string(), config.clone());
-    config
+pub fn get_tool_config(workspace_dir: &Path) -> Arc<ToolConfig> {
+    TOOL_CONFIGS
+        .lock()
+        .unwrap()
+        .entry(workspace_dir.to_path_buf())
+        .or_insert_with(|| ToolConfig::from_workspace(workspace_dir).unwrap().into())
+        .clone()
 }
 
 /// The test runner to use.
@@ -125,11 +123,6 @@ pub struct ToolConfig {
 }
 
 impl ToolConfig {
-    /// Loads the tool config for a specific manifest.
-    pub fn from_manifest_dir(manifest_dir: &str) -> Result<ToolConfig, Error> {
-        ToolConfig::from_workspace(&get_cargo_workspace(manifest_dir))
-    }
-
     /// Loads the tool config from a cargo workspace.
     pub fn from_workspace(workspace_dir: &Path) -> Result<ToolConfig, Error> {
         let mut cfg = None;
@@ -411,43 +404,61 @@ pub fn snapshot_update_behavior(tool_config: &ToolConfig, unseen: bool) -> Snaps
 
 /// Returns the cargo workspace for a manifest
 pub fn get_cargo_workspace(manifest_dir: &str) -> Arc<PathBuf> {
-    // we really do not care about poisoning here.
-    let mut workspaces = WORKSPACES.lock().unwrap_or_else(|x| x.into_inner());
-    if let Some(rv) = workspaces.get(manifest_dir) {
-        rv.clone()
-    } else {
-        // If INSTA_WORKSPACE_ROOT environment variable is set, use the value
-        // as-is. This is useful for those users where the compiled in
-        // CARGO_MANIFEST_DIR points to some transient location. This can easily
-        // happen if the user builds the test in one directory but then tries to
-        // run it in another: even if sources are available in the new
-        // directory, in the past we would always go with the compiled-in value.
-        // The compiled-in directory may not even exist anymore.
-        let path = if let Ok(workspace_root) = std::env::var("INSTA_WORKSPACE_ROOT") {
-            Arc::new(PathBuf::from(workspace_root))
-        } else {
+    // If INSTA_WORKSPACE_ROOT environment variable is set, use the value as-is.
+    // This is useful where CARGO_MANIFEST_DIR at compilation points to some
+    // transient location. This can easily happen when building the test in one
+    // directory but running it in another.
+    if let Ok(workspace_root) = env::var("INSTA_WORKSPACE_ROOT") {
+        return PathBuf::from(workspace_root).into();
+    }
+
+    let error_message = || {
+        format!(
+            "`cargo metadata --format-version=1 --no-deps` in path `{}`",
+            manifest_dir
+        )
+    };
+
+    WORKSPACES
+        .lock()
+        // we really do not care about poisoning here.
+        .unwrap()
+        .entry(manifest_dir.to_string())
+        .or_insert_with(|| {
             let output = std::process::Command::new(
-                env::var("CARGO")
-                    .ok()
-                    .unwrap_or_else(|| "cargo".to_string()),
+                env::var("CARGO").unwrap_or_else(|_| "cargo".to_string()),
             )
-            .arg("metadata")
-            .arg("--format-version=1")
-            .arg("--no-deps")
+            .args(["metadata", "--format-version=1", "--no-deps"])
             .current_dir(manifest_dir)
             .output()
-            .unwrap();
-            let docs = crate::content::yaml::vendored::yaml::YamlLoader::load_from_str(
+            .unwrap_or_else(|e| panic!("failed to run {}\n\n{}", error_message(), e));
+
+            crate::content::yaml::vendored::yaml::YamlLoader::load_from_str(
                 std::str::from_utf8(&output.stdout).unwrap(),
             )
-            .unwrap();
-            let manifest = docs.first().expect("Unable to parse cargo manifest");
-            let workspace_root = PathBuf::from(manifest["workspace_root"].as_str().unwrap());
-            Arc::new(workspace_root)
-        };
-        workspaces.insert(manifest_dir.to_string(), path.clone());
-        path
-    }
+            .map_err(|e| e.to_string())
+            .and_then(|docs| {
+                docs.into_iter()
+                    .next()
+                    .ok_or_else(|| "No content found in yaml".to_string())
+            })
+            .and_then(|metadata| {
+                metadata["workspace_root"]
+                    .clone()
+                    .into_string()
+                    .ok_or_else(|| "Couldn't find `workspace_root`".to_string())
+            })
+            .map(|path| PathBuf::from(path).into())
+            .unwrap_or_else(|e| {
+                panic!(
+                    "failed to parse cargo metadata output from {}: {}\n\n{:?}",
+                    error_message(),
+                    e,
+                    output.stdout
+                )
+            })
+        })
+        .clone()
 }
 
 #[cfg(feature = "_cargo_insta_internal")]

insta/src/glob.rs

@@ -38,7 +38,7 @@ lazy_static::lazy_static! {
     };
 }
 
-pub fn glob_exec<F: FnMut(&Path)>(manifest_dir: &str, base: &Path, pattern: &str, mut f: F) {
+pub fn glob_exec<F: FnMut(&Path)>(workspace_dir: &Path, base: &Path, pattern: &str, mut f: F) {
     // If settings.allow_empty_glob() == true and `base` doesn't exist, skip
     // everything. This is necessary as `base` is user-controlled via `glob!/3`
     // and may not exist.
@@ -61,7 +61,7 @@ pub fn glob_exec<F: FnMut(&Path)>(manifest_dir: &str, base: &Path, pattern: &str
     GLOB_STACK.lock().unwrap().push(GlobCollector {
         failed: 0,
         show_insta_hint: false,
-        fail_fast: get_tool_config(manifest_dir).glob_fail_fast(),
+        fail_fast: get_tool_config(workspace_dir).glob_fail_fast(),
     });
 
     // step 1: collect all matching files

insta/src/macros.rs

@@ -15,6 +15,19 @@ macro_rules! _function_name {
     }};
 }
 
+#[doc(hidden)]
+#[macro_export]
+macro_rules! _get_workspace_root {
+    () => {{
+        use std::env;
+
+        // Note the `env!("CARGO_MANIFEST_DIR")` needs to be in the macro (in
+        // contrast to a function in insta) because the macro needs to capture
+        // the value in the caller library, an exclusive property of macros.
+        $crate::_macro_support::get_cargo_workspace(env!("CARGO_MANIFEST_DIR"))
+    }};
+}
+
 /// Asserts a `Serialize` snapshot in CSV format.
 ///
 /// **Feature:** `csv` (disabled by default)
@@ -349,7 +362,7 @@ macro_rules! _assert_snapshot_base {
             $name.into(),
             #[allow(clippy::redundant_closure_call)]
             &$transform(&$value),
-            env!("CARGO_MANIFEST_DIR"),
+            $crate::_get_workspace_root!().as_path(),
             $crate::_function_name!(),
             module_path!(),
             file!(),
@@ -485,17 +498,26 @@ macro_rules! with_settings {
 #[cfg_attr(docsrs, doc(cfg(feature = "glob")))]
 #[macro_export]
 macro_rules! glob {
+    // TODO: I think we could remove the three-argument version of this macro
+    // and just support a pattern such as
+    // `glob!("../test_data/inputs/*.txt"...`.
     ($base_path:expr, $glob:expr, $closure:expr) => {{
         use std::path::Path;
-        let base = $crate::_macro_support::get_cargo_workspace(env!("CARGO_MANIFEST_DIR"))
+
+        let base = $crate::_get_workspace_root!()
             .join(Path::new(file!()).parent().unwrap())
             .join($base_path)
             .to_path_buf();
 
         // we try to canonicalize but on some platforms (eg: wasm) that might not work, so
         // we instead silently fall back.
         let base = base.canonicalize().unwrap_or_else(|_| base);
-        $crate::_macro_support::glob_exec(env!("CARGO_MANIFEST_DIR"), &base, $glob, $closure);
+        $crate::_macro_support::glob_exec(
+            $crate::_get_workspace_root!().as_path(),
+            &base,
+            $glob,
+            $closure,
+        );
     }};
 
     ($glob:expr, $closure:expr) => {{