From 71357a07afddd5ad7095fe3c1f75d77437eb27af Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Wed, 4 Jan 2023 23:26:30 +0100 Subject: [PATCH 1/2] Warn only if snapshots are found in hidden or ignored locations --- cargo-insta/src/cargo.rs | 10 +---- cargo-insta/src/cli.rs | 79 +++++++++++++++++++++++++++++++++------- cargo-insta/src/walk.rs | 76 +++++++++++++++++++------------------- 3 files changed, 105 insertions(+), 60 deletions(-) diff --git a/cargo-insta/src/cargo.rs b/cargo-insta/src/cargo.rs index 4073968e..d0dceab1 100644 --- a/cargo-insta/src/cargo.rs +++ b/cargo-insta/src/cargo.rs @@ -7,9 +7,7 @@ use std::process; use serde::Deserialize; -use crate::container::SnapshotContainer; use crate::utils::err_msg; -use crate::walk::{find_snapshots, FindFlags}; #[derive(Deserialize, Clone, Debug)] pub struct Target { @@ -57,11 +55,7 @@ impl Package { &self.version } - pub fn iter_snapshot_containers<'a>( - &self, - extensions: &'a [&'a str], - find_flags: FindFlags, - ) -> impl Iterator>> + 'a { + pub fn find_snapshot_roots<'a>(&self) -> Vec { let mut roots = Vec::new(); // the manifest path's parent is always a snapshot container. For @@ -101,8 +95,6 @@ impl Package { } reduced_roots - .into_iter() - .flat_map(move |root| find_snapshots(&root, extensions, find_flags)) } } diff --git a/cargo-insta/src/cli.rs b/cargo-insta/src/cli.rs index d9b00437..0a3e76e2 100644 --- a/cargo-insta/src/cli.rs +++ b/cargo-insta/src/cli.rs @@ -19,7 +19,7 @@ use uuid::Uuid; use crate::cargo::{find_packages, get_cargo, get_package_metadata, Package}; use crate::container::{Operation, SnapshotContainer}; use crate::utils::{err_msg, QuietExit}; -use crate::walk::{find_snapshots, make_deletion_walker, FindFlags}; +use crate::walk::{find_snapshots, make_deletion_walker, make_snapshot_walker, FindFlags}; /// A helper utility to work with insta snapshots. #[derive(StructOpt, Debug)] @@ -361,20 +361,31 @@ fn handle_target_args(target_args: &TargetArgs) -> Result, Box< fn load_snapshot_containers<'a>( loc: &'a LocationInfo, -) -> Result)>, Box> { +) -> Result< + ( + Vec<(SnapshotContainer, Option<&'a Package>)>, + HashSet, + ), + Box, +> { + let mut roots = HashSet::new(); let mut snapshot_containers = vec![]; if let Some(ref packages) = loc.packages { for package in packages.iter() { - for snapshot_container in package.iter_snapshot_containers(&loc.exts, loc.find_flags) { - snapshot_containers.push((snapshot_container?, Some(package))); + for root in package.find_snapshot_roots() { + roots.insert(root.clone()); + for snapshot_container in find_snapshots(&root, &loc.exts, loc.find_flags) { + snapshot_containers.push((snapshot_container?, Some(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)); } } - Ok(snapshot_containers) + Ok((snapshot_containers, roots)) } fn process_snapshots( @@ -385,14 +396,14 @@ fn process_snapshots( ) -> Result<(), Box> { let term = Term::stdout(); - let mut snapshot_containers = load_snapshot_containers(&loc)?; + let (mut snapshot_containers, roots) = load_snapshot_containers(&loc)?; let snapshot_count = snapshot_containers.iter().map(|x| x.0.len()).sum(); if snapshot_count == 0 { if !quiet { println!("{}: no snapshots to review", style("done").bold()); - show_ignore_hint(loc.find_flags); + show_ignore_hint(loc.find_flags, &snapshot_containers, &roots, &loc.exts); } return Ok(()); } @@ -585,7 +596,7 @@ fn test_run(mut cmd: TestCommand, color: &str) -> Result<(), Box> { )? } else { let loc = handle_target_args(&cmd.target_args)?; - let snapshot_containers = load_snapshot_containers(&loc)?; + let (snapshot_containers, roots) = load_snapshot_containers(&loc)?; let snapshot_count = snapshot_containers.iter().map(|x| x.0.len()).sum::(); if snapshot_count > 0 { eprintln!( @@ -598,7 +609,7 @@ fn test_run(mut cmd: TestCommand, color: &str) -> Result<(), Box> { return Err(QuietExit(1).into()); } else { println!("{}: no snapshots to review", style("info").bold()); - show_ignore_hint(loc.find_flags); + show_ignore_hint(loc.find_flags, &snapshot_containers, &roots, &loc.exts); } } @@ -868,7 +879,7 @@ fn pending_snapshots_cmd(cmd: PendingSnapshotsCommand) -> Result<(), Box Result<(), Box)], + roots: &HashSet, + extensions: &[&str], +) { + // there is nothing to do if we already search everything. + if find_flags.include_hidden && find_flags.include_ignored { + return; + } + + let mut found_extra = false; + let found_snapshots = snapshot_containers + .iter() + .filter_map(|x| x.0.snapshot_file()) + .collect::>(); + + for root in roots { + for snapshot in make_snapshot_walker( + root, + extensions, + FindFlags { + include_ignored: true, + include_hidden: true, + }, + ) + .filter_map(|e| e.ok()) + .filter(|x| { + let fname = x.file_name().to_string_lossy(); + fname.ends_with(".snap.new") || fname.ends_with(".pending-snap") + }) { + if !found_snapshots.contains(snapshot.path()) { + found_extra = true; + break; + } + } + } + + // we did not find any extra snapshots + if !found_extra { + return; + } + let (args, paths) = match (find_flags.include_ignored, find_flags.include_hidden) { - (true, true) => return, (true, false) => ("--include-ignored", "ignored"), (false, true) => ("--include-hidden", "hidden"), (false, false) => ( "--include-ignored and --include-hidden", "ignored or hidden", ), + (true, true) => unreachable!(), }; println!( "{}: {}", style("warning").yellow().bold(), format_args!( - "some paths are not picked up by cargo insta, use {} if you have snapshots in {} paths.", + "found snapshots in some paths which are not picked up by cargo insta, use {} if you have snapshots in {} paths.", args, paths, ) ); diff --git a/cargo-insta/src/walk.rs b/cargo-insta/src/walk.rs index f2e1243f..017ba0f3 100644 --- a/cargo-insta/src/walk.rs +++ b/cargo-insta/src/walk.rs @@ -23,8 +23,41 @@ fn is_hidden(entry: &DirEntry) -> bool { .unwrap_or(false) } +/// Finds all snapshots +pub fn find_snapshots<'a>( + root: &Path, + extensions: &'a [&'a str], + flags: FindFlags, +) -> impl Iterator>> + 'a { + make_snapshot_walker(&root, extensions, flags) + .filter_map(|e| e.ok()) + .filter_map(move |e| { + let fname = e.file_name().to_string_lossy(); + if fname.ends_with(".new") { + let new_path = e.into_path(); + let mut old_path = new_path.clone(); + old_path.set_extension(""); + Some(SnapshotContainer::load( + new_path, + old_path, + SnapshotContainerKind::External, + )) + } else if fname.starts_with('.') && fname.ends_with(".pending-snap") { + let mut target_path = e.path().to_path_buf(); + target_path.set_file_name(&fname[1..fname.len() - 13]); + Some(SnapshotContainer::load( + e.path().to_path_buf(), + target_path, + SnapshotContainerKind::Inline, + )) + } else { + None + } + }) +} + /// Creates a walker for snapshots. -fn make_snapshot_walker(path: &Path, extensions: &[&str], flags: FindFlags) -> Walk { +pub fn make_snapshot_walker(path: &Path, extensions: &[&str], flags: FindFlags) -> Walk { let mut builder = WalkBuilder::new(path); builder.standard_filters(!flags.include_ignored); if flags.include_hidden { @@ -48,7 +81,9 @@ fn make_snapshot_walker(path: &Path, extensions: &[&str], flags: FindFlags) -> W builder.build() } -/// A walker that deletes. +/// 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 fn make_deletion_walker( workspace_root: &Path, known_packages: Option<&[Package]>, @@ -68,9 +103,7 @@ pub fn make_deletion_walker( }) .collect() } else { - let mut hs = HashSet::new(); - hs.insert(workspace_root.to_path_buf()); - hs + Some(workspace_root.to_path_buf()).into_iter().collect() }; WalkBuilder::new(workspace_root) @@ -109,36 +142,3 @@ pub fn make_deletion_walker( }) .build() } - -/// Finds all snapshots -pub fn find_snapshots<'a>( - root: &Path, - extensions: &'a [&'a str], - flags: FindFlags, -) -> impl Iterator>> + 'a { - make_snapshot_walker(&root, extensions, flags) - .filter_map(|e| e.ok()) - .filter_map(move |e| { - let fname = e.file_name().to_string_lossy(); - if fname.ends_with(".new") { - let new_path = e.into_path(); - let mut old_path = new_path.clone(); - old_path.set_extension(""); - Some(SnapshotContainer::load( - new_path, - old_path, - SnapshotContainerKind::External, - )) - } else if fname.starts_with('.') && fname.ends_with(".pending-snap") { - let mut target_path = e.path().to_path_buf(); - target_path.set_file_name(&fname[1..fname.len() - 13]); - Some(SnapshotContainer::load( - e.path().to_path_buf(), - target_path, - SnapshotContainerKind::Inline, - )) - } else { - None - } - }) -} From b7070b38e2c0b129b0b47cbe08830bb4ef25be72 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Thu, 5 Jan 2023 10:35:12 +0100 Subject: [PATCH 2/2] Added a config flag to disable undiscoverable snapshots warning --- cargo-insta/src/cli.rs | 14 +++++++++----- src/env.rs | 10 ++++++++++ src/lib.rs | 3 +++ 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/cargo-insta/src/cli.rs b/cargo-insta/src/cli.rs index 0a3e76e2..df68b3ea 100644 --- a/cargo-insta/src/cli.rs +++ b/cargo-insta/src/cli.rs @@ -403,7 +403,9 @@ fn process_snapshots( if snapshot_count == 0 { if !quiet { println!("{}: no snapshots to review", style("done").bold()); - show_ignore_hint(loc.find_flags, &snapshot_containers, &roots, &loc.exts); + if loc.tool_config.review_warn_undiscovered() { + show_undiscovered_hint(loc.find_flags, &snapshot_containers, &roots, &loc.exts); + } } return Ok(()); } @@ -595,7 +597,6 @@ fn test_run(mut cmd: TestCommand, color: &str) -> Result<(), Box> { }, )? } else { - let loc = handle_target_args(&cmd.target_args)?; let (snapshot_containers, roots) = load_snapshot_containers(&loc)?; let snapshot_count = snapshot_containers.iter().map(|x| x.0.len()).sum::(); if snapshot_count > 0 { @@ -609,7 +610,9 @@ fn test_run(mut cmd: TestCommand, color: &str) -> Result<(), Box> { return Err(QuietExit(1).into()); } else { println!("{}: no snapshots to review", style("info").bold()); - show_ignore_hint(loc.find_flags, &snapshot_containers, &roots, &loc.exts); + if loc.tool_config.review_warn_undiscovered() { + show_undiscovered_hint(loc.find_flags, &snapshot_containers, &roots, &loc.exts); + } } } @@ -912,7 +915,7 @@ fn pending_snapshots_cmd(cmd: PendingSnapshotsCommand) -> Result<(), Box)], roots: &HashSet, @@ -969,7 +972,8 @@ fn show_ignore_hint( "{}: {}", style("warning").yellow().bold(), format_args!( - "found snapshots in some paths which are not picked up by cargo insta, use {} if you have snapshots in {} paths.", + "found undiscovered snapshots in some paths which are not picked up by cargo \ + insta. Use {} if you have snapshots in {} paths.", args, paths, ) ); diff --git a/src/env.rs b/src/env.rs index fd573d44..d9e9567c 100644 --- a/src/env.rs +++ b/src/env.rs @@ -117,6 +117,8 @@ pub struct ToolConfig { review_include_ignored: bool, #[cfg(feature = "_cargo_insta_internal")] review_include_hidden: bool, + #[cfg(feature = "_cargo_insta_internal")] + review_warn_undiscovered: bool, } impl ToolConfig { @@ -236,6 +238,10 @@ impl ToolConfig { review_include_ignored: resolve(&cfg, &["review", "include_ignored"]) .and_then(|x| x.as_bool()) .unwrap_or(false), + #[cfg(feature = "_cargo_insta_internal")] + review_warn_undiscovered: resolve(&cfg, &["review", "warn_undiscovered"]) + .and_then(|x| x.as_bool()) + .unwrap_or(true), }) } @@ -294,6 +300,10 @@ impl ToolConfig { pub fn review_include_ignored(&self) -> bool { self.review_include_ignored } + + pub fn review_warn_undiscovered(&self) -> bool { + self.review_warn_undiscovered + } } /// How snapshots are supposed to be updated diff --git a/src/lib.rs b/src/lib.rs index 5ae07141..718c1dbd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -220,6 +220,9 @@ //! include_ignored: true / false //! # also look for snapshots in hidden folders //! include_hidden: true / false +//! # show a warning if undiscovered (ignored or hidden) snapshots are found. +//! # defaults to true but creates a performance hit. +//! warn_undiscovered: true / false //! ``` //! //! # Optional: Faster Runs