From a1880f00ceb9285a799147843f89cf6b61889b84 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Sun, 1 Sep 2024 17:28:46 -0700 Subject: [PATCH] Adjust force-update on inline snapshots to only view within the string --- cargo-insta/src/container.rs | 19 +++------ cargo-insta/src/walk.rs | 6 +-- cargo-insta/tests/main.rs | 69 +++++++++++++++++++++++++++--- insta/src/lib.rs | 2 +- insta/src/runtime.rs | 44 ++++++++++++++++--- insta/src/snapshot.rs | 83 +++++++++++++++++++++++++----------- 6 files changed, 168 insertions(+), 55 deletions(-) diff --git a/cargo-insta/src/container.rs b/cargo-insta/src/container.rs index 172bcc5b..0b4d4766 100644 --- a/cargo-insta/src/container.rs +++ b/cargo-insta/src/container.rs @@ -3,6 +3,7 @@ use std::fs; use std::path::{Path, PathBuf}; use insta::Snapshot; +pub(crate) use insta::SnapshotKind; use insta::_cargo_insta_support::{ContentError, PendingInlineSnapshot}; use crate::inline::FilePatcher; @@ -14,12 +15,6 @@ pub(crate) enum Operation { Skip, } -#[derive(Debug)] -pub(crate) enum SnapshotContainerKind { - Inline, - External, -} - #[derive(Debug)] pub(crate) struct PendingSnapshot { #[allow(dead_code)] @@ -51,7 +46,7 @@ impl PendingSnapshot { pub(crate) struct SnapshotContainer { snapshot_path: PathBuf, target_path: PathBuf, - kind: SnapshotContainerKind, + kind: SnapshotKind, snapshots: Vec, patcher: Option, } @@ -60,11 +55,11 @@ impl SnapshotContainer { pub(crate) fn load( snapshot_path: PathBuf, target_path: PathBuf, - kind: SnapshotContainerKind, + kind: SnapshotKind, ) -> Result> { let mut snapshots = Vec::new(); let patcher = match kind { - SnapshotContainerKind::External => { + SnapshotKind::File => { let old = if fs::metadata(&target_path).is_err() { None } else { @@ -80,7 +75,7 @@ impl SnapshotContainer { }); None } - SnapshotContainerKind::Inline => { + SnapshotKind::Inline => { let mut pending_vec = PendingInlineSnapshot::load_batch(&snapshot_path)?; let mut have_new = false; @@ -136,8 +131,8 @@ impl SnapshotContainer { pub(crate) fn snapshot_file(&self) -> Option<&Path> { match self.kind { - SnapshotContainerKind::External => Some(&self.target_path), - SnapshotContainerKind::Inline => None, + SnapshotKind::File => Some(&self.target_path), + SnapshotKind::Inline => None, } } diff --git a/cargo-insta/src/walk.rs b/cargo-insta/src/walk.rs index be9dc460..dd7ee8e5 100644 --- a/cargo-insta/src/walk.rs +++ b/cargo-insta/src/walk.rs @@ -5,7 +5,7 @@ use std::path::Path; use ignore::overrides::OverrideBuilder; use ignore::{DirEntry, Walk, WalkBuilder}; -use crate::container::{SnapshotContainer, SnapshotContainerKind}; +use crate::container::{SnapshotContainer, SnapshotKind}; #[derive(Debug, Copy, Clone)] pub(crate) struct FindFlags { @@ -37,7 +37,7 @@ pub(crate) fn find_snapshots<'a>( Some(SnapshotContainer::load( new_path, old_path, - SnapshotContainerKind::External, + SnapshotKind::File, )) } else if fname.starts_with('.') && fname.ends_with(".pending-snap") { let mut target_path = e.path().to_path_buf(); @@ -45,7 +45,7 @@ pub(crate) fn find_snapshots<'a>( Some(SnapshotContainer::load( e.path().to_path_buf(), target_path, - SnapshotContainerKind::Inline, + SnapshotKind::Inline, )) } else { None diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index a87dafa0..b87bcb72 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -837,13 +837,13 @@ Hello, world! } #[test] -fn test_force_update_inline_snapshot() { +fn test_force_update_inline_snapshot_linebreaks() { let test_project = TestFiles::new() .add_file( "Cargo.toml", r#" [package] -name = "force-update-inline" +name = "force-update-inline-linkbreaks" version = "0.1.0" edition = "2021" @@ -856,8 +856,11 @@ insta = { path = '$PROJECT_PATH' } "src/lib.rs", r#####" #[test] -fn test_excessive_hashes() { - insta::assert_snapshot!("foo", @r####"foo"####); +fn test_linebreaks() { + insta::assert_snapshot!("foo", @r####" + foo + + "####); } "##### .to_string(), @@ -882,16 +885,68 @@ fn test_excessive_hashes() { assert_snapshot!(test_project.diff("src/lib.rs"), @r#####" --- Original: src/lib.rs +++ Updated: src/lib.rs - @@ -1,5 +1,5 @@ + @@ -1,8 +1,5 @@ #[test] - fn test_excessive_hashes() { - - insta::assert_snapshot!("foo", @r####"foo"####); + fn test_linebreaks() { + - insta::assert_snapshot!("foo", @r####" + - foo + - + - "####); + insta::assert_snapshot!("foo", @"foo"); } "#####); } +#[test] +fn test_force_update_inline_snapshot_hashes() { + let test_project = TestFiles::new() + .add_file( + "Cargo.toml", + r#" +[package] +name = "force-update-inline" +version = "0.1.0" +edition = "2021" + +[dependencies] +insta = { path = '$PROJECT_PATH' } +"# + .to_string(), + ) + .add_file( + "src/lib.rs", + r#####" +#[test] +fn test_excessive_hashes() { + insta::assert_snapshot!("foo", @r####"foo"####); +} +"##### + .to_string(), + ) + .create_project(); + + // Run the test with --force-update-snapshots and --accept + let output = test_project + .cmd() + .args([ + "test", + "--force-update-snapshots", + "--accept", + "--", + "--nocapture", + ]) + .output() + .unwrap(); + + assert_success(&output); + + // TODO: we would like to update the number of hashes, but that's not easy + // given the reasons at https://github.com/mitsuhiko/insta/pull/573. So this + // result asserts the current state rather than the desired state. + assert_snapshot!(test_project.diff("src/lib.rs"), @""); +} + #[test] fn test_hashtag_escape_in_inline_snapshot() { let test_project = TestFiles::new() diff --git a/insta/src/lib.rs b/insta/src/lib.rs index a146e705..89e69c3e 100644 --- a/insta/src/lib.rs +++ b/insta/src/lib.rs @@ -288,7 +288,7 @@ mod glob; mod test; pub use crate::settings::Settings; -pub use crate::snapshot::{MetaData, Snapshot}; +pub use crate::snapshot::{MetaData, Snapshot, SnapshotKind}; /// Exposes some library internals. /// diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 90c96398..798921c8 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -8,14 +8,17 @@ use std::str; use std::sync::{Arc, Mutex}; use std::{borrow::Cow, env}; -use crate::env::{ - get_cargo_workspace, get_tool_config, memoize_snapshot_file, snapshot_update_behavior, - OutputBehavior, SnapshotUpdateBehavior, ToolConfig, -}; use crate::output::SnapshotPrinter; use crate::settings::Settings; use crate::snapshot::{MetaData, PendingInlineSnapshot, Snapshot, SnapshotContents}; use crate::utils::{path_to_storage, style}; +use crate::{ + env::{ + get_cargo_workspace, get_tool_config, memoize_snapshot_file, snapshot_update_behavior, + OutputBehavior, SnapshotUpdateBehavior, ToolConfig, + }, + snapshot::SnapshotKind, +}; lazy_static::lazy_static! { static ref TEST_NAME_COUNTERS: Mutex> = @@ -77,6 +80,7 @@ impl<'a> From<&'a str> for ReferenceValue<'a> { } } +#[derive(Debug)] /// A reference to a snapshot pub enum ReferenceValue<'a> { /// A named snapshot, where the inner value is the snapshot name. @@ -286,6 +290,7 @@ impl<'a> SnapshotAssertionContext<'a> { module_path.replace("::", "__"), None, MetaData::default(), + SnapshotKind::Inline, SnapshotContents::from_inline(contents), )); } @@ -314,7 +319,12 @@ impl<'a> SnapshotAssertionContext<'a> { } /// Creates the new snapshot from input values. - pub fn new_snapshot(&self, contents: SnapshotContents, expr: &str) -> Snapshot { + pub fn new_snapshot( + &self, + contents: SnapshotContents, + expr: &str, + kind: SnapshotKind, + ) -> Snapshot { Snapshot::from_components( self.module_path.replace("::", "__"), self.snapshot_name.as_ref().map(|x| x.to_string()), @@ -333,6 +343,7 @@ impl<'a> SnapshotAssertionContext<'a> { .and_then(|x| self.localize_path(x)) .map(|x| path_to_storage(&x)), }), + kind, contents, ) } @@ -636,7 +647,11 @@ pub fn assert_snapshot( let new_snapshot_value = Settings::with(|settings| settings.filters().apply_to(new_snapshot_value)); - let new_snapshot = ctx.new_snapshot(new_snapshot_value.into(), expr); + let kind = match ctx.snapshot_file { + Some(_) => SnapshotKind::File, + None => SnapshotKind::Inline, + }; + let new_snapshot = ctx.new_snapshot(new_snapshot_value.into(), expr, kind); // memoize the snapshot file if requested. if let Some(ref snapshot_file) = ctx.snapshot_file { @@ -668,7 +683,22 @@ pub fn assert_snapshot( ctx.cleanup_passing()?; if tool_config.force_update_snapshots() { - ctx.update_snapshot(new_snapshot)?; + // Avoid creating new files if contents match exactly. In + // particular, this would otherwise create lots of unneeded files + // for inline snapshots + // + // Note that there's a check down the stack on whether the file + // contents match exactly for file snapshots; probably we should + // combine that check with `matches_fully` and then use a single + // check for whether we force update snapshots. + let matches_fully = &ctx + .old_snapshot + .as_ref() + .map(|x| x.matches_fully(&new_snapshot)) + .unwrap_or(false); + if !matches_fully { + ctx.update_snapshot(new_snapshot)?; + } } // otherwise print information and update snapshots. } else { diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index 8c8fafe4..4bd52edc 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -87,8 +87,12 @@ impl PendingInlineSnapshot { match key.as_str() { Some("run_id") => run_id = value.as_str().map(|x| x.to_string()), Some("line") => line = value.as_u64().map(|x| x as u32), - Some("old") if !value.is_nil() => old = Some(Snapshot::from_content(value)?), - Some("new") if !value.is_nil() => new = Some(Snapshot::from_content(value)?), + Some("old") if !value.is_nil() => { + old = Some(Snapshot::from_content(value, SnapshotKind::Inline)?) + } + Some("new") if !value.is_nil() => { + new = Some(Snapshot::from_content(value, SnapshotKind::Inline)?) + } _ => {} } } @@ -255,7 +259,8 @@ impl MetaData { // snapshot. But we don't know that from here (notably // `self.input_file.is_none()` is not a correct approach). Given that // `--require-full-match` is experimental and we're working on making - // inline & file snapshots more coherent, I'm leaving this as is for now. + // inline & file snapshots more coherent, I'm leaving this as is for + // now. if self.assertion_line.is_some() { let mut rv = self.clone(); rv.assertion_line = None; @@ -266,12 +271,19 @@ impl MetaData { } } +#[derive(Debug, Clone)] +pub enum SnapshotKind { + Inline, + File, +} + /// A helper to work with stored snapshots. #[derive(Debug, Clone)] pub struct Snapshot { module_name: String, snapshot_name: Option, metadata: MetaData, + kind: SnapshotKind, snapshot: SnapshotContents, } @@ -338,6 +350,7 @@ impl Snapshot { module_name, Some(snapshot_name), metadata, + SnapshotKind::File, buf.into(), )) } @@ -347,18 +360,20 @@ impl Snapshot { module_name: String, snapshot_name: Option, metadata: MetaData, + kind: SnapshotKind, snapshot: SnapshotContents, ) -> Snapshot { Snapshot { module_name, snapshot_name, metadata, + kind, snapshot, } } #[cfg(feature = "_cargo_insta_internal")] - fn from_content(content: Content) -> Result> { + fn from_content(content: Content, kind: SnapshotKind) -> Result> { if let Content::Map(map) = content { let mut module_name = None; let mut snapshot_name = None; @@ -386,6 +401,7 @@ impl Snapshot { module_name: module_name.ok_or(content::Error::MissingField)?, snapshot_name, metadata: metadata.ok_or(content::Error::MissingField)?, + kind, snapshot: snapshot.ok_or(content::Error::MissingField)?, }) } else { @@ -429,10 +445,15 @@ impl Snapshot { self.contents() == other.contents() } - /// Snapshot contents _and_ metadata match another snapshot's. + /// Both exact snapshot contents and metadata matches another snapshot's. pub fn matches_fully(&self, other: &Snapshot) -> bool { - self.snapshot.matches_fully(&other.snapshot) - && self.metadata.trim_for_persistence() == other.metadata.trim_for_persistence() + match self.kind { + SnapshotKind::File => { + self.metadata.trim_for_persistence() == other.metadata.trim_for_persistence() + && self.contents().as_str_exact() == other.contents().as_str_exact() + } + SnapshotKind::Inline => self.contents().to_inline(0) == other.contents().to_inline(0), + } } /// The snapshot contents as a &str @@ -515,6 +536,11 @@ pub struct SnapshotContents(String); impl SnapshotContents { pub fn from_inline(value: &str) -> SnapshotContents { + // Note that if we wanted to allow for `matches_fully` to return false + // when indentation is different, we would need to store the unnormalize + // inline snapshot in this object, and then normalize it on request. + // Currently we normalize away the indentation and then can't compare + // values. SnapshotContents(get_inline_snapshot_value(value)) } @@ -535,10 +561,6 @@ impl SnapshotContents { self.0.as_str() } - pub fn matches_fully(&self, other: &SnapshotContents) -> bool { - self.as_str_exact() == other.as_str_exact() - } - pub fn to_inline(&self, indentation: usize) -> String { let contents = &self.0; let mut out = String::new(); @@ -651,9 +673,7 @@ fn min_indentation(snapshot: &str) -> usize { fn normalize_inline_snapshot(snapshot: &str) -> String { let indentation = min_indentation(snapshot); snapshot - .trim_end() .lines() - .skip_while(|l| l.is_empty()) .map(|l| l.get(indentation..).unwrap_or("")) .collect::>() .join("\n") @@ -853,10 +873,12 @@ fn test_normalize_inline_snapshot() { r#" 1 2 - "# + "# ), - r###"1 -2"### + r###" +1 +2 +"### ); assert_eq!( @@ -865,7 +887,8 @@ fn test_normalize_inline_snapshot() { 1 2"# ), - r###" 1 + r###" + 1 2"### ); @@ -876,8 +899,10 @@ fn test_normalize_inline_snapshot() { 2 "# ), - r###"1 -2"### + r###" +1 +2 +"### ); assert_eq!( @@ -887,7 +912,8 @@ fn test_normalize_inline_snapshot() { 2 "# ), - r###"1 + r###" +1 2"### ); @@ -897,7 +923,9 @@ fn test_normalize_inline_snapshot() { a "# ), - "a" + " +a +" ); assert_eq!(normalize_inline_snapshot(""), ""); @@ -910,9 +938,11 @@ fn test_normalize_inline_snapshot() { c "# ), - r###" a + r###" + a b -c"### +c + "### ); assert_eq!( @@ -921,7 +951,9 @@ c"### a "# ), - "a" + " +a + " ); assert_eq!( @@ -929,7 +961,8 @@ a " a" ), - "a" + " +a" ); assert_eq!(