Repository: mitsuhiko/insta
Pull request #532 (2024-07-22 03:57:31)

Fix workspace with default root, simplify lots walkers & metadata
~On top of https://github.com/mitsuhiko/insta/pull/531; that needs to merge first. This simplifies that code~

OK so:

-   this PR brings together a big bug fix and few refactors, since they were stepping on each other
-   it builds on top of some tests that a) demonstrate mostly correct behavior and b) show the bug — a panic when running `cargo test --accept --workspace` on a workspace with a default crate and other members. It fixes that bug, and removes the `#[should_panic]` attribute on that test. None of the previously correct behavior changes.
-   It simplifies the walkers a lot
-   It simplifies the metadata collection a lot

I realize it was a bit messy to get here. Hopefully now that we have tests, it's easier to review small changes to code, and we don't have quite so many slightly-overlapping changes pending at the same time. I do think some of the code was getting needlessly complicated and this makes it much simpler, as well as fixing the bug. (But ofc very open to suggested changes!)

max-sixty: OK so:
- this PR brings together a big bug fix and few refactors, since they were stepping on each other
- it builds on top of some tests that a) demonstrate mostly correct behavior and b) show the bug — a panic when running `cargo test --accept` on a workspace with a default crate and other members. It fixes that bug, and removes the `#[should_panic]` attribute on that test. None of the previously correct behavior changes.
- It simplifies the walkers a lot
- It simplifies the metadata collection a lot

I realize it was a bit messy to get here. Hopefully now that we have tests, it's easier to review small changes to code, and we don't have quite so many slightly-overlapping changes pending at the same time. I do think some of the code was getting needlessly complicated and this makes it much simpler, as well as fixing the bug. (But ofc very open to suggested changes!)
max-sixty: @mitsuhiko if you're happy with this, I think we could merge and then release 1.40...

(maybe also https://github.com/mitsuhiko/insta/pull/482 if you're happy with it, but less immediacy...)

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 633fd61..6350742 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,9 @@ All notable changes to insta and cargo-insta are documented here.
 
 ## 1.40.0
 
+- `cargo-insta` no longer panics when running `cargo test --accept --workspace`
+  on a workspace with a default crate. #532
+
 - MSRV for `insta` has been raised to 1.60, and for `cargo-insta` to 1.64.
 
 - Added support for compact debug snapshots (`assert_compact_debug_snapshot`). #514
diff --git a/cargo-insta/src/cargo.rs b/cargo-insta/src/cargo.rs
index ade0e79..8dc7b34 100644
--- a/cargo-insta/src/cargo.rs
+++ b/cargo-insta/src/cargo.rs
@@ -1,7 +1,6 @@
-use std::error::Error;
-use std::path::{Path, PathBuf};
+use std::path::PathBuf;
 
-pub(crate) use cargo_metadata::{Metadata, Package};
+pub(crate) use cargo_metadata::Package;
 
 pub(crate) fn find_snapshot_roots(package: &Package) -> Vec<PathBuf> {
     let mut roots = Vec::new();
@@ -44,33 +43,3 @@ pub(crate) fn find_snapshot_roots(package: &Package) -> Vec<PathBuf> {
 
     reduced_roots
 }
-
-pub(crate) fn get_metadata(
-    manifest_path: Option<&Path>,
-    all: bool,
-) -> Result<Metadata, Box<dyn Error>> {
-    let mut cmd = cargo_metadata::MetadataCommand::new();
-    if let Some(manifest_path) = manifest_path {
-        cmd.manifest_path(manifest_path);
-    }
-    if all {
-        cmd.no_deps();
-    }
-    let mut metadata = cmd.exec()?;
-    let Metadata {
-        packages,
-        workspace_members,
-        resolve,
-        ..
-    } = &mut metadata;
-    match resolve
-        .as_ref()
-        .and_then(|cargo_metadata::Resolve { root, .. }| root.as_ref())
-    {
-        Some(root) => packages.retain(|Package { id, .. }| id == root),
-        None => {
-            packages.retain(|Package { id, .. }| workspace_members.contains(id));
-        }
-    }
-    Ok(metadata)
-}
diff --git a/cargo-insta/src/cli.rs b/cargo-insta/src/cli.rs
index 27ed501..1d70010 100644
--- a/cargo-insta/src/cli.rs
+++ b/cargo-insta/src/cli.rs
@@ -1,6 +1,5 @@
 use std::borrow::{Borrow, Cow};
 use std::error::Error;
-use std::fmt::Display;
 use std::path::{Path, PathBuf};
 use std::{collections::HashSet, fmt};
 use std::{env, fs};
@@ -14,10 +13,10 @@ use insta::_cargo_insta_support::{
 use serde::Serialize;
 use uuid::Uuid;
 
-use crate::cargo::{find_snapshot_roots, get_metadata, Metadata, Package};
+use crate::cargo::{find_snapshot_roots, Package};
 use crate::container::{Operation, SnapshotContainer};
 use crate::utils::{err_msg, QuietExit};
-use crate::walk::{find_snapshots, make_deletion_walker, make_snapshot_walker, FindFlags};
+use crate::walk::{find_snapshots, make_snapshot_walker, FindFlags};
 
 use clap::{Args, Parser, Subcommand, ValueEnum};
 
@@ -251,7 +250,7 @@ fn query_snapshot(
     term: &Term,
     new: &Snapshot,
     old: Option<&Snapshot>,
-    pkg: Option<&Package>,
+    pkg: &Package,
     line: Option<u32>,
     i: usize,
     n: usize,
@@ -261,19 +260,14 @@ fn query_snapshot(
 ) -> Result<Operation, Box<dyn Error>> {
     loop {
         term.clear_screen()?;
-        let (pkg_name, pkg_version): (_, &dyn Display) = if let Some(pkg) = pkg {
-            (pkg.name.as_str(), &pkg.version)
-        } else {
-            ("unknown package", &"unknown version")
-        };
 
         println!(
             "{}{}{} {}@{}:",
             style("Reviewing [").bold(),
             style(format!("{}/{}", i, n)).yellow().bold(),
             style("]").bold(),
-            pkg_name,
-            pkg_version,
+            pkg.name.as_str(),
+            &pkg.version,
         );
 
         let mut printer = SnapshotPrinter::new(workspace_root, old, new);
@@ -356,7 +350,8 @@ fn handle_color(color: Option<ColorWhen>) {
 struct LocationInfo<'a> {
     tool_config: ToolConfig,
     workspace_root: PathBuf,
-    packages: Option<Vec<Package>>,
+    /// Packages to test
+    packages: Vec<Package>,
     exts: Vec<&'a str>,
     find_flags: FindFlags,
 }
@@ -368,7 +363,11 @@ fn get_find_flags(tool_config: &ToolConfig, target_args: &TargetArgs) -> FindFla
     }
 }
 
-fn handle_target_args(target_args: &TargetArgs) -> Result<LocationInfo<'_>, Box<dyn Error>> {
+fn handle_target_args<'a>(
+    target_args: &'a TargetArgs,
+    // Empty if none are selected, implying cargo default
+    packages: &'a [String],
+) -> Result<LocationInfo<'a>, Box<dyn Error>> {
     let exts: Vec<&str> = target_args.extensions.iter().map(|x| x.as_str()).collect();
 
     // if a workspace root is provided we first check if it points to a `Cargo.toml`.  If it
@@ -384,74 +383,77 @@ fn handle_target_args(target_args: &TargetArgs) -> Result<LocationInfo<'_>, Box<
             ))
         }
         (None, Some(manifest)) => (None, Some(Cow::Borrowed(manifest))),
-        (Some(root), manifest_path) => {
+        (Some(root), None) => {
             let assumed_manifest = root.join("Cargo.toml");
             if assumed_manifest.is_file() {
                 (None, Some(Cow::Owned(assumed_manifest)))
             } else {
-                (Some(root), manifest_path.map(Cow::Borrowed))
+                (Some(root), None)
             }
         }
         (None, None) => (None, None),
     };
 
+    let mut cmd = cargo_metadata::MetadataCommand::new();
+
+    // If a manifest path is provided, set it in the command
+    if let Some(manifest_path) = manifest_path {
+        cmd.manifest_path(manifest_path);
+    }
     if let Some(workspace_root) = workspace_root {
-        let tool_config = ToolConfig::from_workspace(workspace_root)?;
-        Ok(LocationInfo {
-            workspace_root: workspace_root.to_owned(),
-            packages: None,
-            exts,
-            find_flags: get_find_flags(&tool_config, target_args),
-            tool_config,
-        })
-    } else {
-        let Metadata {
-            packages,
-            workspace_root,
-            ..
-        } = get_metadata(
-            manifest_path.as_deref(),
-            target_args.all || target_args.workspace,
-        )?;
-        let workspace_root = workspace_root.as_std_path().to_path_buf();
-        let tool_config = ToolConfig::from_workspace(&workspace_root)?;
-        Ok(LocationInfo {
-            workspace_root,
-            packages: Some(packages),
-            exts,
-            find_flags: get_find_flags(&tool_config, target_args),
-            tool_config,
-        })
+        cmd.current_dir(workspace_root);
     }
+    let metadata = cmd.no_deps().exec()?;
+    let workspace_root = metadata.workspace_root.as_std_path().to_path_buf();
+    let tool_config = ToolConfig::from_workspace(&workspace_root)?;
+
+    // If `--all` is passed, or there's no root package, we include all
+    // packages. If packages are specified, we filter from all packages.
+    // Otherwise we use just the root package.
+    //
+    // (Once we're OK running on Cargo 1.71, we can replace `.root_package` with
+    // `.default_workspace_packages`.)
+    let packages = if metadata.root_package().is_none()
+        || (target_args.all || target_args.workspace)
+        || !packages.is_empty()
+    {
+        metadata
+            .workspace_packages()
+            .into_iter()
+            .filter(|p| packages.is_empty() || packages.contains(&p.name))
+            .cloned()
+            .collect()
+    } else {
+        vec![metadata.root_package().unwrap().clone()]
+    };
+
+    Ok(LocationInfo {
+        workspace_root,
+        packages,
+        exts,
+        find_flags: get_find_flags(&tool_config, target_args),
+        tool_config,
+    })
 }
 
 #[allow(clippy::type_complexity)]
 fn load_snapshot_containers<'a>(
     loc: &'a LocationInfo,
-) -> Result<
-    (
-        Vec<(SnapshotContainer, Option<&'a Package>)>,
-        HashSet<PathBuf>,
-    ),
-    Box<dyn Error>,
-> {
+) -> Result<(Vec<(SnapshotContainer, &'a Package)>, HashSet<PathBuf>), Box<dyn Error>> {
     let mut roots = HashSet::new();
     let mut snapshot_containers = vec![];
-    if let Some(ref packages) = loc.packages {
-        for package in packages.iter() {
-            for root in find_snapshot_roots(package) {
-                roots.insert(root.clone());
-                for snapshot_container in find_snapshots(&root, &loc.exts, loc.find_flags) {
-                    snapshot_containers.push((snapshot_container?, Some(package)));
-                }
+
+    debug_assert!(!loc.packages.is_empty());
+
+    for package in &loc.packages {
+        for root in find_snapshot_roots(package) {
+            roots.insert(root.clone());
+            for snapshot_container in find_snapshots(&root, &loc.exts, loc.find_flags) {
+                snapshot_containers.push((snapshot_container?, package));
             }
         }
-    } else {
-        roots.insert(loc.workspace_root.clone());
-        for snapshot_container in find_snapshots(&loc.workspace_root, &loc.exts, loc.find_flags) {
-            snapshot_containers.push((snapshot_container?, None));
-        }
     }
+
     snapshot_containers.sort_by(|a, b| a.0.snapshot_sort_key().cmp(&b.0.snapshot_sort_key()));
     Ok((snapshot_containers, roots))
 }
@@ -510,7 +512,7 @@ fn process_snapshots(
                     &term,
                     &snapshot_ref.new,
                     snapshot_ref.old.as_ref(),
-                    *package,
+                    package,
                     snapshot_ref.line,
                     num,
                     snapshot_count,
@@ -566,7 +568,7 @@ fn process_snapshots(
 }
 
 fn test_run(mut cmd: TestCommand, color: ColorWhen) -> Result<(), Box<dyn Error>> {
-    let loc = handle_target_args(&cmd.target_args)?;
+    let loc = handle_target_args(&cmd.target_args, &cmd.test_runner_options.package)?;
     match loc.tool_config.snapshot_update() {
         SnapshotUpdate::Auto => {
             if is_ci() {
@@ -655,13 +657,8 @@ fn test_run(mut cmd: TestCommand, color: ColorWhen) -> Result<(), Box<dyn Error>
     // handle unreferenced snapshots if we were instructed to do so and the
     // tests ran successfully
     if success {
-        if let Some(ref path) = snapshot_ref_file {
-            handle_unreferenced_snapshots(
-                path.borrow(),
-                &loc,
-                cmd.unreferenced,
-                &cmd.test_runner_options.package[..],
-            )?;
+        if let Some(ref snapshot_ref_path) = snapshot_ref_file {
+            handle_unreferenced_snapshots(snapshot_ref_path.borrow(), &loc, cmd.unreferenced)?;
         }
     }
 
@@ -669,7 +666,7 @@ fn test_run(mut cmd: TestCommand, color: ColorWhen) -> Result<(), Box<dyn Error>
         process_snapshots(
             false,
             None,
-            &handle_target_args(&cmd.target_args)?,
+            &loc,
             if cmd.accept {
                 Some(Operation::Accept)
             } else {
@@ -703,11 +700,11 @@ fn test_run(mut cmd: TestCommand, color: ColorWhen) -> Result<(), Box<dyn Error>
     }
 }
 
+/// Scan for any snapshots that were not referenced by any test.
 fn handle_unreferenced_snapshots(
-    path: &Path,
+    snapshot_ref_path: &Path,
     loc: &LocationInfo<'_>,
     unreferenced: UnreferencedSnapshots,
-    packages: &[String],
 ) -> Result<(), Box<dyn Error>> {
     enum Action {
         Delete,
@@ -730,7 +727,7 @@ fn handle_unreferenced_snapshots(
     };
 
     let mut files = HashSet::new();
-    match fs::read_to_string(path) {
+    match fs::read_to_string(snapshot_ref_path) {
         Ok(s) => {
             for line in s.lines() {
                 if let Ok(path) = fs::canonicalize(line) {
@@ -748,23 +745,28 @@ fn handle_unreferenced_snapshots(
     }
 
     let mut encountered_any = false;
-    for entry in make_deletion_walker(&loc.workspace_root, loc.packages.as_deref(), packages) {
-        let rel_path = match entry {
-            Ok(ref entry) => entry.path(),
-            _ => continue,
-        };
-        if !rel_path.is_file()
-            || !rel_path
-                .file_name()
-                .map_or(false, |x| x.to_str().unwrap_or("").ends_with(".snap"))
-        {
-            continue;
-        }
 
-        if let Ok(path) = fs::canonicalize(rel_path) {
-            if files.contains(&path) {
-                continue;
-            }
+    for package in loc.packages.clone() {
+        let walker = make_snapshot_walker(
+            package.manifest_path.parent().unwrap().as_std_path(),
+            &[".snap"],
+            FindFlags {
+                include_ignored: true,
+                include_hidden: true,
+            },
+        )
+        .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| name.ends_with(".snap"))
+                .unwrap_or(false)
+        })
+        .filter_map(|e| e.path().canonicalize().ok())
+        .filter(|path| !files.contains(path));
+
+        for path in walker {
             if !encountered_any {
                 match action {
                     Action::Delete => {
@@ -779,14 +781,14 @@ fn handle_unreferenced_snapshots(
                 }
                 encountered_any = true;
             }
-            eprintln!("  {}", rel_path.display());
+            eprintln!("  {}", path.display());
             if matches!(action, Action::Delete) {
-                fs::remove_file(path).ok();
+                fs::remove_file(&path).ok();
             }
         }
     }
 
-    fs::remove_file(path).ok();
+    fs::remove_file(snapshot_ref_path).ok();
 
     if !encountered_any {
         eprintln!("{}: no unreferenced snapshots found", style("info").bold());
@@ -978,7 +980,7 @@ fn prepare_test_runner<'snapshot_ref>(
 }
 
 fn show_cmd(cmd: ShowCommand) -> Result<(), Box<dyn Error>> {
-    let loc = handle_target_args(&cmd.target_args)?;
+    let loc = handle_target_args(&cmd.target_args, &[])?;
     let snapshot = Snapshot::from_file(&cmd.path)?;
     let mut printer = SnapshotPrinter::new(&loc.workspace_root, None, &snapshot);
     printer.set_snapshot_file(Some(&cmd.path));
@@ -1004,7 +1006,7 @@ fn pending_snapshots_cmd(cmd: PendingSnapshotsCommand) -> Result<(), Box<dyn Err
         },
     }
 
-    let loc = handle_target_args(&cmd.target_args)?;
+    let loc = handle_target_args(&cmd.target_args, &[])?;
     let (mut snapshot_containers, _) = load_snapshot_containers(&loc)?;
 
     for (snapshot_container, _package) in snapshot_containers.iter_mut() {
@@ -1037,7 +1039,7 @@ fn pending_snapshots_cmd(cmd: PendingSnapshotsCommand) -> Result<(), Box<dyn Err
 
 fn show_undiscovered_hint(
     find_flags: FindFlags,
-    snapshot_containers: &[(SnapshotContainer, Option<&Package>)],
+    snapshot_containers: &[(SnapshotContainer, &Package)],
     roots: &HashSet<PathBuf>,
     extensions: &[&str],
 ) {
@@ -1114,7 +1116,7 @@ pub(crate) fn run() -> Result<(), Box<dyn Error>> {
             process_snapshots(
                 cmd.quiet,
                 cmd.snapshot_filter.as_deref(),
-                &handle_target_args(&cmd.target_args)?,
+                &handle_target_args(&cmd.target_args, &[])?,
                 match opts.command {
                     Command::Review(_) => None,
                     Command::Accept(_) => Some(Operation::Accept),
diff --git a/cargo-insta/src/container.rs b/cargo-insta/src/container.rs
index f60fb1b..172bcc5 100644
--- a/cargo-insta/src/container.rs
+++ b/cargo-insta/src/container.rs
@@ -209,6 +209,7 @@ impl SnapshotContainer {
             }
         } else {
             // should only be one or this is weird
+            debug_assert!(self.snapshots.len() == 1);
             for snapshot in self.snapshots.iter() {
                 match snapshot.op {
                     Operation::Accept => {
diff --git a/cargo-insta/src/walk.rs b/cargo-insta/src/walk.rs
index 2c96e83..be9dc46 100644
--- a/cargo-insta/src/walk.rs
+++ b/cargo-insta/src/walk.rs
@@ -1,4 +1,3 @@
-use std::collections::HashSet;
 use std::error::Error;
 use std::ffi::OsStr;
 use std::path::Path;
@@ -6,7 +5,6 @@ use std::path::Path;
 use ignore::overrides::OverrideBuilder;
 use ignore::{DirEntry, Walk, WalkBuilder};
 
-use crate::cargo::Package;
 use crate::container::{SnapshotContainer, SnapshotContainerKind};
 
 #[derive(Debug, Copy, Clone)]
@@ -25,11 +23,11 @@ fn is_hidden(entry: &DirEntry) -> bool {
 
 /// Finds all snapshots
 pub(crate) fn find_snapshots<'a>(
-    root: &Path,
+    package_root: &Path,
     extensions: &'a [&'a str],
     flags: FindFlags,
 ) -> impl Iterator<Item = Result<SnapshotContainer, Box<dyn Error>>> + 'a {
-    make_snapshot_walker(root, extensions, flags)
+    make_snapshot_walker(package_root, extensions, flags)
         .filter_map(|e| e.ok())
         .filter_map(move |e| {
             let fname = e.file_name().to_string_lossy();
@@ -55,87 +53,52 @@ pub(crate) fn find_snapshots<'a>(
         })
 }
 
-/// Creates a walker for snapshots.
-pub(crate) fn make_snapshot_walker(path: &Path, extensions: &[&str], flags: FindFlags) -> Walk {
-    let mut builder = WalkBuilder::new(path);
+/// Creates a walker for snapshots & pending snapshots within a package.
+pub(crate) fn make_snapshot_walker(
+    package_root: &Path,
+    extensions: &[&str],
+    flags: FindFlags,
+) -> Walk {
+    let mut builder = WalkBuilder::new(package_root);
     builder.standard_filters(!flags.include_ignored);
     if flags.include_hidden {
         builder.hidden(false);
     } else {
+        // We add a custom hidden filter to avoid skipping over `.pending-snap` files
         builder.filter_entry(|e| e.file_type().map_or(false, |x| x.is_file()) || !is_hidden(e));
     }
 
-    let mut override_builder = OverrideBuilder::new(path);
+    let mut override_builder = OverrideBuilder::new(package_root);
     override_builder
         .add(".*.pending-snap")
         .unwrap()
         .add("*.snap.new")
         .unwrap();
-
     for ext in extensions {
         override_builder.add(&format!("*.{}.new", ext)).unwrap();
     }
-
     builder.overrides(override_builder.build().unwrap());
-    builder.build()
-}
 
-/// A walker that is used by the snapshot deletion code.
-///
-/// This really should be using the same logic as the main snapshot walker but today is is not.
-pub(crate) fn make_deletion_walker(
-    workspace_root: &Path,
-    known_packages: Option<&[Package]>,
-    selected_packages: &[String],
-) -> Walk {
-    let roots: HashSet<_> = if let Some(packages) = known_packages {
-        packages
-            .iter()
-            .filter_map(|x| {
-                // filter out packages we did not ask for.
-                if !selected_packages.is_empty() && !selected_packages.contains(&x.name) {
-                    return None;
-                }
-                x.manifest_path.parent().unwrap().canonicalize().ok()
-            })
-            .collect()
-    } else {
-        Some(workspace_root.to_path_buf()).into_iter().collect()
-    };
+    let root_path = package_root.to_path_buf();
 
-    WalkBuilder::new(workspace_root)
-        .filter_entry(move |entry| {
-            // we only filter down for directories
-            if !entry.file_type().map_or(false, |x| x.is_dir()) {
-                return true;
-            }
-
-            let canonicalized = match entry.path().canonicalize() {
-                Ok(path) => path,
-                Err(_) => return true,
-            };
-
-            // We always want to skip target even if it was not excluded by
-            // ignore files.
-            if entry.path().file_name() == Some(OsStr::new("target"))
-                && roots.contains(canonicalized.parent().unwrap())
-            {
+    // Add a custom filter to skip interior crates; otherwise we get duplicate
+    // snapshots (https://github.com/mitsuhiko/insta/issues/396)
+    builder.filter_entry(move |entry| {
+        if entry.file_type().map_or(false, |ft| ft.is_dir()) {
+            let cargo_toml_path = entry.path().join("Cargo.toml");
+            if cargo_toml_path.exists() && entry.path() != root_path {
+                // Skip this directory if it contains a Cargo.toml and is not the root
                 return false;
             }
+        }
+        // We always want to skip target even if it was not excluded by
+        // ignore files.
+        if entry.path().file_name() == Some(OsStr::new("target")) {
+            return false;
+        }
 
-            // do not enter crates which are not in the list of known roots
-            // of the workspace.
-            if !roots.contains(&canonicalized)
-                && entry
-                    .path()
-                    .join("Cargo.toml")
-                    .metadata()
-                    .map_or(false, |x| x.is_file())
-            {
-                return false;
-            }
+        true
+    });
 
-            true
-        })
-        .build()
+    builder.build()
 }


Test patch:
diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs
index 2d6e377..a8c7e2f 100644
--- a/cargo-insta/tests/main.rs
+++ b/cargo-insta/tests/main.rs
@@ -472,8 +472,6 @@ fn test_root() {
         )
 }
 
-// Currently panics
-#[should_panic]
 /// Check that in a workspace with a default root crate, running `cargo insta
 /// test --workspace` will update snapsnots in both the root crate and the
 /// member crate.