From a1880f00ceb9285a799147843f89cf6b61889b84 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Sun, 1 Sep 2024 17:28:46 -0700 Subject: [PATCH 01/10] 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!( From 40df53012ba95ff044e049d452758987f3fba5c4 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Sun, 1 Sep 2024 22:51:56 -0700 Subject: [PATCH 02/10] --- cargo-insta/src/cli.rs | 6 +- cargo-insta/tests/main.rs | 81 +++++++++++++++++- insta/src/output.rs | 8 +- insta/src/runtime.rs | 14 +--- insta/src/snapshot.rs | 168 ++++++++++++++++---------------------- 5 files changed, 164 insertions(+), 113 deletions(-) diff --git a/cargo-insta/src/cli.rs b/cargo-insta/src/cli.rs index 8f7046e9..527b116b 100644 --- a/cargo-insta/src/cli.rs +++ b/cargo-insta/src/cli.rs @@ -1061,12 +1061,14 @@ fn pending_snapshots_cmd(cmd: PendingSnapshotsCommand) -> Result<(), Box SnapshotPrinter<'a> { } fn print_changeset(&self) { - let old = self.old_snapshot.as_ref().map_or("", |x| x.contents_str()); + let old: String = self + .old_snapshot + .map_or("".to_string(), |x| x.contents_str()); let new = self.new_snapshot.contents_str(); - let newlines_matter = newlines_matter(old, new); + let newlines_matter = newlines_matter(old.as_str(), new.as_str()); let width = term_width(); let diff = TextDiff::configure() .algorithm(Algorithm::Patience) .timeout(Duration::from_millis(500)) - .diff_lines(old, new); + .diff_lines(old.as_str(), new.as_str()); print_line(width); if self.show_info { diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 798921c8..0de089bb 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -290,8 +290,7 @@ impl<'a> SnapshotAssertionContext<'a> { module_path.replace("::", "__"), None, MetaData::default(), - SnapshotKind::Inline, - SnapshotContents::from_inline(contents), + SnapshotContents::new(contents.to_string(), SnapshotKind::Inline), )); } }; @@ -319,12 +318,7 @@ impl<'a> SnapshotAssertionContext<'a> { } /// Creates the new snapshot from input values. - pub fn new_snapshot( - &self, - contents: SnapshotContents, - expr: &str, - kind: SnapshotKind, - ) -> Snapshot { + pub fn new_snapshot(&self, contents: SnapshotContents, expr: &str) -> Snapshot { Snapshot::from_components( self.module_path.replace("::", "__"), self.snapshot_name.as_ref().map(|x| x.to_string()), @@ -343,7 +337,6 @@ impl<'a> SnapshotAssertionContext<'a> { .and_then(|x| self.localize_path(x)) .map(|x| path_to_storage(&x)), }), - kind, contents, ) } @@ -651,7 +644,8 @@ pub fn assert_snapshot( Some(_) => SnapshotKind::File, None => SnapshotKind::Inline, }; - let new_snapshot = ctx.new_snapshot(new_snapshot_value.into(), expr, kind); + let new_snapshot = + ctx.new_snapshot(SnapshotContents::new(new_snapshot_value.into(), kind), expr); // memoize the snapshot file if requested. if let Some(ref snapshot_file) = ctx.snapshot_file { diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index 4bd52edc..ac3f7919 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -1,10 +1,10 @@ -use std::borrow::Cow; use std::env; use std::error::Error; use std::fs; use std::io::{BufRead, BufReader, Write}; use std::path::{Path, PathBuf}; use std::time::{SystemTime, UNIX_EPOCH}; +use std::{borrow::Cow, fmt}; use crate::content::{self, json, yaml, Content}; @@ -271,7 +271,7 @@ impl MetaData { } } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Copy)] pub enum SnapshotKind { Inline, File, @@ -283,7 +283,6 @@ pub struct Snapshot { module_name: String, snapshot_name: Option, metadata: MetaData, - kind: SnapshotKind, snapshot: SnapshotContents, } @@ -350,8 +349,10 @@ impl Snapshot { module_name, Some(snapshot_name), metadata, - SnapshotKind::File, - buf.into(), + SnapshotContents { + contents: buf, + kind: SnapshotKind::File, + }, )) } @@ -360,14 +361,12 @@ impl Snapshot { module_name: String, snapshot_name: Option, metadata: MetaData, - kind: SnapshotKind, snapshot: SnapshotContents, ) -> Snapshot { Snapshot { module_name, snapshot_name, metadata, - kind, snapshot, } } @@ -386,12 +385,13 @@ impl Snapshot { Some("snapshot_name") => snapshot_name = value.as_str().map(|x| x.to_string()), Some("metadata") => metadata = Some(MetaData::from_content(value)?), Some("snapshot") => { - snapshot = Some(SnapshotContents( - value + snapshot = Some(SnapshotContents { + contents: value .as_str() .ok_or(content::Error::UnexpectedDataType)? .to_string(), - )) + kind, + }); } _ => {} } @@ -401,7 +401,6 @@ 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 { @@ -415,7 +414,7 @@ impl Snapshot { fields.push(("snapshot_name", Content::from(name))); } fields.push(("metadata", self.metadata.as_content())); - fields.push(("snapshot", Content::from(self.snapshot.0.as_str()))); + fields.push(("snapshot", Content::from(self.snapshot.contents.as_str()))); Content::Struct("Content", fields) } @@ -445,9 +444,13 @@ impl Snapshot { self.contents() == other.contents() } + pub fn kind(&self) -> SnapshotKind { + self.snapshot.kind + } + /// Both exact snapshot contents and metadata matches another snapshot's. pub fn matches_fully(&self, other: &Snapshot) -> bool { - match self.kind { + 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() @@ -456,15 +459,15 @@ impl Snapshot { } } - /// The snapshot contents as a &str - pub fn contents_str(&self) -> &str { - self.snapshot.as_str() + /// The snapshot contents as a String + pub fn contents_str(&self) -> String { + self.snapshot.to_string() } fn serialize_snapshot(&self, md: &MetaData) -> String { let mut buf = yaml::to_string(&md.as_content()); buf.push_str("---\n"); - buf.push_str(self.contents_str()); + buf.push_str(self.contents_str().as_str()); buf.push('\n'); buf } @@ -532,37 +535,25 @@ impl Snapshot { /// The contents of a Snapshot // Could be Cow, but I think limited savings #[derive(Debug, Clone)] -pub struct SnapshotContents(String); +pub struct SnapshotContents { + contents: String, + pub kind: SnapshotKind, +} 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)) - } - - /// Returns the snapshot contents as string with surrounding whitespace removed. - pub fn as_str(&self) -> &str { - let out = self.0.trim_start_matches(['\r', '\n']).trim_end(); - // Old inline snapshots have `---` at the start, so this strips that if - // it exists. Soon we can start printing a warning and then eventually - // remove it in the next version. - match out.strip_prefix("---\n") { - Some(s) => s, - None => out, - } + pub fn new(contents: String, kind: SnapshotKind) -> SnapshotContents { + SnapshotContents { contents, kind } } - /// Returns the snapshot contents as string without any trimming. + /// Returns the snapshot contents as string without any normalization. pub fn as_str_exact(&self) -> &str { - self.0.as_str() + self.contents.as_str() } + /// Returns the string literal, including `#` delimiters, to insert into a + /// Rust source file. pub fn to_inline(&self, indentation: usize) -> String { - let contents = &self.0; + let contents = &self.contents; let mut out = String::new(); // We don't technically need to escape on newlines, but it reduces diffs @@ -612,40 +603,38 @@ impl SnapshotContents { out } -} -impl<'a> From> for SnapshotContents { - fn from(value: Cow<'a, str>) -> Self { - match value { - Cow::Borrowed(s) => SnapshotContents::from(s), - Cow::Owned(s) => SnapshotContents::from(s), + pub fn normalize(&self) -> String { + let kind_specific_normalization = match self.kind { + SnapshotKind::Inline => get_inline_snapshot_value(&self.contents), + SnapshotKind::File => self.contents.clone(), + }; + // Then this we do for both kinds + let out = kind_specific_normalization + .trim_start_matches(['\r', '\n']) + .trim_end(); + let out = out.replace("\r\n", "\n"); + // Old inline snapshots have `---` at the start, so this strips that if + // it exists. Soon we can start printing a warning and then eventually + // remove it in the next version. (this will move into a + // `normalize_legacy` method at some point) + match out.strip_prefix("---\n") { + Some(s) => s.to_string(), + None => out, } } } -impl From<&str> for SnapshotContents { - fn from(value: &str) -> SnapshotContents { - // make sure we have unix newlines consistently - SnapshotContents(value.replace("\r\n", "\n")) - } -} - -impl From for SnapshotContents { - fn from(value: String) -> SnapshotContents { - // make sure we have unix newlines consistently - SnapshotContents(value.replace("\r\n", "\n")) - } -} - -impl From for String { - fn from(value: SnapshotContents) -> String { - value.0 +impl fmt::Display for SnapshotContents { + /// Returns the snapshot contents as string with surrounding whitespace removed. + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.normalize()) } } impl PartialEq for SnapshotContents { fn eq(&self, other: &Self) -> bool { - self.as_str() == other.as_str() + self.to_string() == other.to_string() } } @@ -733,7 +722,7 @@ fn test_names_of_path() { /// /// This also changes all newlines to \n fn get_inline_snapshot_value(frozen_value: &str) -> String { - // TODO: could move this into the SnapshotContents `from_inline` method + // TODO: could move this into the SnapshotContents struct // (the only call site) if frozen_value.trim_start().starts_with('⋮') { @@ -783,14 +772,13 @@ fn get_inline_snapshot_value(frozen_value: &str) -> String { #[test] fn test_snapshot_contents() { use similar_asserts::assert_eq; - let snapshot_contents = SnapshotContents("testing".to_string()); - assert_eq!(snapshot_contents.to_inline(0), r#""testing""#); + assert_eq!( + SnapshotContents::new("testing".to_string(), SnapshotKind::Inline).to_inline(0), + r#""testing""# + ); - let t = &" -a -b"[1..]; assert_eq!( - SnapshotContents(t.to_string()).to_inline(0), + SnapshotContents::new("\na\nb".to_string(), SnapshotKind::Inline).to_inline(0), r##"r#" a b @@ -798,35 +786,23 @@ b ); assert_eq!( - SnapshotContents( - "a -b" - .to_string() - ) - .to_inline(4), + SnapshotContents::new("a\nb".to_string(), SnapshotKind::Inline).to_inline(4), r##"r#" a b "#"## ); - let t = &" - a - b"[1..]; assert_eq!( - SnapshotContents(t.to_string()).to_inline(0), + SnapshotContents::new("\n a\n b".to_string(), SnapshotKind::Inline).to_inline(0), r##"r#" a b "#"## ); - let t = &" -a - -b"[1..]; assert_eq!( - SnapshotContents(t.to_string()).to_inline(4), + SnapshotContents::new("\na\n\nb".to_string(), SnapshotKind::Inline).to_inline(4), r##"r#" a @@ -834,28 +810,28 @@ b"[1..]; "#"## ); - let t = &" - ab -"[1..]; assert_eq!( - SnapshotContents(t.to_string()).to_inline(0), + SnapshotContents::new("\n ab\n".to_string(), SnapshotKind::Inline).to_inline(0), r##"r#" ab "#"## ); - let t = "ab"; - assert_eq!(SnapshotContents(t.to_string()).to_inline(0), r#""ab""#); + assert_eq!( + SnapshotContents::new("ab".to_string(), SnapshotKind::Inline).to_inline(0), + r#""ab""# + ); } #[test] fn test_snapshot_contents_hashes() { - let t = "a###b"; - assert_eq!(SnapshotContents(t.to_string()).to_inline(0), r#""a###b""#); + assert_eq!( + SnapshotContents::new("a###b".to_string(), SnapshotKind::Inline).to_inline(0), + r#""a###b""# + ); - let t = "a\n\\###b"; assert_eq!( - SnapshotContents(t.to_string()).to_inline(0), + SnapshotContents::new("a\n\\###b".to_string(), SnapshotKind::Inline).to_inline(0), r#####"r####" a \###b From 20873c69c5cbd6ccbf560717a4418f03c88203fd Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Mon, 2 Sep 2024 10:05:35 -0700 Subject: [PATCH 03/10] --- insta/src/snapshot.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index ac3f7919..6eada222 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -553,7 +553,7 @@ impl SnapshotContents { /// Returns the string literal, including `#` delimiters, to insert into a /// Rust source file. pub fn to_inline(&self, indentation: usize) -> String { - let contents = &self.contents; + let contents = self.normalize(); let mut out = String::new(); // We don't technically need to escape on newlines, but it reduces diffs @@ -595,7 +595,7 @@ impl SnapshotContents { .chain(Some(format!("\n{:width$}", "", width = indentation))), ); } else { - out.push_str(contents); + out.push_str(contents.as_str()); } out.push('"'); @@ -796,8 +796,8 @@ b assert_eq!( SnapshotContents::new("\n a\n b".to_string(), SnapshotKind::Inline).to_inline(0), r##"r#" - a - b +a +b "#"## ); @@ -812,9 +812,7 @@ b assert_eq!( SnapshotContents::new("\n ab\n".to_string(), SnapshotKind::Inline).to_inline(0), - r##"r#" - ab -"#"## + r##""ab""## ); assert_eq!( From 35d5335ecedfec19e3633581b0e821a0ceee7f69 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Mon, 2 Sep 2024 10:14:11 -0700 Subject: [PATCH 04/10] --- cargo-insta/tests/main.rs | 19 +++++++++---------- insta/src/snapshot.rs | 6 ++++-- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index 5e7094f1..31c88726 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -1009,16 +1009,14 @@ fn test_wrong_indent_force() { assert_snapshot!(test_project.diff("src/lib.rs"), @r##" --- Original: src/lib.rs +++ Updated: src/lib.rs - @@ -5,7 +5,9 @@ + @@ -5,7 +5,7 @@ foo foo "#, @r#" - foo - foo - + - + foo - + foo - + + + foo + + foo "#); } "##); @@ -1045,7 +1043,7 @@ insta = { path = '$PROJECT_PATH' } r#" #[test] fn test_hashtag_escape() { - insta::assert_snapshot!("Value with #### hashtags\n", @""); + insta::assert_snapshot!("Value with\n#### hashtags\n", @""); } "# .to_string(), @@ -1063,13 +1061,14 @@ fn test_hashtag_escape() { assert_snapshot!(test_project.diff("src/main.rs"), @r######" --- Original: src/main.rs +++ Updated: src/main.rs - @@ -1,5 +1,7 @@ + @@ -1,5 +1,8 @@ #[test] fn test_hashtag_escape() { - - insta::assert_snapshot!("Value with #### hashtags\n", @""); - + insta::assert_snapshot!("Value with #### hashtags\n", @r#####" - + Value with #### hashtags + - insta::assert_snapshot!("Value with\n#### hashtags\n", @""); + + insta::assert_snapshot!("Value with\n#### hashtags\n", @r#####" + + Value with + + #### hashtags + "#####); } "######); diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index 6eada222..1ed899b6 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -448,14 +448,16 @@ impl Snapshot { self.snapshot.kind } - /// Both exact snapshot contents and metadata matches another snapshot's. + /// Both the exact snapshot contents and the persisted metadata match another snapshot's. pub fn matches_fully(&self, other: &Snapshot) -> bool { 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), + SnapshotKind::Inline => { + self.contents().as_str_exact() == other.contents().as_str_exact() + } } } From 75d4589ab9700060e5583fdf47b089f1a4d10fe9 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Tue, 3 Sep 2024 10:50:23 -0700 Subject: [PATCH 05/10] --- cargo-insta/src/cli.rs | 4 ++-- insta/src/output.rs | 6 +++--- insta/src/snapshot.rs | 20 +++++++++++++------- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/cargo-insta/src/cli.rs b/cargo-insta/src/cli.rs index 527b116b..9f15ca04 100644 --- a/cargo-insta/src/cli.rs +++ b/cargo-insta/src/cli.rs @@ -1061,8 +1061,8 @@ fn pending_snapshots_cmd(cmd: PendingSnapshotsCommand) -> Result<(), Box SnapshotPrinter<'a> { fn print_snapshot(&self) { print_line(term_width()); - let new_contents = self.new_snapshot.contents_str(); + let new_contents = self.new_snapshot.contents_string(); let width = term_width(); if self.show_info { @@ -120,8 +120,8 @@ impl<'a> SnapshotPrinter<'a> { fn print_changeset(&self) { let old: String = self .old_snapshot - .map_or("".to_string(), |x| x.contents_str()); - let new = self.new_snapshot.contents_str(); + .map_or("".to_string(), |x| x.contents_string()); + let new = self.new_snapshot.contents_string(); let newlines_matter = newlines_matter(old.as_str(), new.as_str()); let width = term_width(); diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index 1ed899b6..6aa2bda8 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -449,27 +449,29 @@ impl Snapshot { } /// Both the exact snapshot contents and the persisted metadata match another snapshot's. + // (could rename to `matches_exact` for consistency, after some current + // pending merge requests are merged) pub fn matches_fully(&self, other: &Snapshot) -> bool { + let contents_match_exact = + self.contents().as_str_exact() == other.contents().as_str_exact(); 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().as_str_exact() == other.contents().as_str_exact() + && contents_match_exact } + SnapshotKind::Inline => contents_match_exact, } } - /// The snapshot contents as a String - pub fn contents_str(&self) -> String { + /// The normalized snapshot contents as a String + pub fn contents_string(&self) -> String { self.snapshot.to_string() } fn serialize_snapshot(&self, md: &MetaData) -> String { let mut buf = yaml::to_string(&md.as_content()); buf.push_str("---\n"); - buf.push_str(self.contents_str().as_str()); + buf.push_str(self.contents_string().as_str()); buf.push('\n'); buf } @@ -544,6 +546,10 @@ pub struct SnapshotContents { impl SnapshotContents { pub fn new(contents: String, kind: SnapshotKind) -> SnapshotContents { + // We could store a normalized version of the string as part of `new`; + // it would avoid allocating a new `String` when we getting normalized + // versions, which we may do a few times. (We want to store the + // unnormalized version because it allows us to use `matches_fully`.) SnapshotContents { contents, kind } } From 5a0fd677541630ae1cb1986abae68a27ebb97a36 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Sun, 8 Sep 2024 10:48:04 -0700 Subject: [PATCH 06/10] Update changelog Add section, add missing word in 1.40 --- CHANGELOG.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9fe9a664..1081998c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,14 +2,16 @@ All notable changes to insta and cargo-insta are documented here. +## 1.41.0 + ## 1.40.0 -- `cargo-insta` no longer panics when running `cargo test --accept --workspace` - on a workspace with a default crate. #532 +- `cargo-insta` no longer panics when running `cargo insta 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 +- Added support for compact debug snapshots (`assert_compact_debug_snapshot`). #514 - Deprecate `--no-force-pass` in `cargo-insta`. The `--check` option covers the same functionality and has a clearer name. #513 From 0ea299cb9af7c0cc53d50a5e2b9ddca492457625 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Sun, 8 Sep 2024 10:50:07 -0700 Subject: [PATCH 07/10] --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1081998c..cd5b152e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ All notable changes to insta and cargo-insta are documented here. ## 1.41.0 +- `--force-update-snapshots` has more conservative and consistent behavior for + inline snapshots. As a side-effect of this, only the content within the inline + snapshot delimiters are assessed for changes, not the delimiters (e.g. `###`). + #581 + ## 1.40.0 - `cargo-insta` no longer panics when running `cargo insta test --accept --workspace` From daae9f6409c5852ce534a73bb96159ba8d547813 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Sun, 15 Sep 2024 15:50:48 -0700 Subject: [PATCH 08/10] --- insta/src/runtime.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 77005bbb..ad8a1ddd 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -683,7 +683,10 @@ pub fn assert_snapshot( if pass { ctx.cleanup_passing()?; - if tool_config.force_update_snapshots() { + if matches!( + tool_config.snapshot_update(), + crate::env::SnapshotUpdate::Force + ) { // Avoid creating new files if contents match exactly. In // particular, this would otherwise create lots of unneeded files // for inline snapshots From 264eda98d214d61dba98835898e06d0ad015df62 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Tue, 17 Sep 2024 10:39:03 -0700 Subject: [PATCH 09/10] --- insta/src/snapshot.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index e69f5660..dc0a210e 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -471,7 +471,7 @@ impl Snapshot { /// The normalized snapshot contents as a String pub fn contents_string(&self) -> String { - self.snapshot.to_string() + self.snapshot.normalize() } fn serialize_snapshot(&self, md: &MetaData) -> String { From abd1d75d826ed17570c1d74d3a8300cb35c76f7b Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Tue, 17 Sep 2024 20:46:21 -0700 Subject: [PATCH 10/10] . --- insta/src/snapshot.rs | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index dc0a210e..05463c56 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -315,6 +315,8 @@ impl Snapshot { let content = yaml::parse_str(&buf, p)?; MetaData::from_content(content)? // legacy format + // (but not viable to move into `match_legacy` given it's more than + // just the snapshot value itself...) } else { let mut rv = MetaData::default(); loop { @@ -420,7 +422,7 @@ impl Snapshot { fields.push(("snapshot_name", Content::from(name))); } fields.push(("metadata", self.metadata.as_content())); - fields.push(("snapshot", Content::from(self.snapshot.contents.as_str()))); + fields.push(("snapshot", Content::from(self.snapshot.to_string()))); Content::Struct("Content", fields) } @@ -553,18 +555,12 @@ pub struct SnapshotContents { impl SnapshotContents { pub fn new(contents: String, kind: SnapshotKind) -> SnapshotContents { // We could store a normalized version of the string as part of `new`; - // it would avoid allocating a new `String` when we getting normalized + // it would avoid allocating a new `String` when we get the normalized // versions, which we may do a few times. (We want to store the // unnormalized version because it allows us to use `matches_fully`.) SnapshotContents { contents, kind } } - /// Returns the snapshot contents as a normalized string (for example, - /// removing surrounding whitespace) - pub fn as_str(&self) -> String { - self.normalize() - } - /// Returns the snapshot contents as string without any normalization pub fn as_str_exact(&self) -> &str { self.contents.as_str() @@ -577,12 +573,12 @@ impl SnapshotContents { /// Snapshot matches based on the latest format. pub fn matches_latest(&self, other: &SnapshotContents) -> bool { - self.as_str() == other.as_str() + self.to_string() == other.to_string() } pub fn matches_legacy(&self, other: &SnapshotContents) -> bool { fn as_str_legacy(sc: &SnapshotContents) -> String { - let out = sc.as_str(); + let out = sc.to_string(); // Legacy inline snapshots have `---` at the start, so this strips that if // it exists. match out.strip_prefix("---\n") { @@ -649,7 +645,7 @@ impl SnapshotContents { out } - pub fn normalize(&self) -> String { + fn normalize(&self) -> String { let kind_specific_normalization = match self.kind { SnapshotKind::Inline => get_inline_snapshot_value(&self.contents), SnapshotKind::File => self.contents.clone(), @@ -663,7 +659,8 @@ impl SnapshotContents { } impl fmt::Display for SnapshotContents { - /// Returns the snapshot contents as string with surrounding whitespace removed. + /// Returns the snapshot contents as a normalized string (for example, + /// removing surrounding whitespace) fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}", self.normalize()) } @@ -675,7 +672,7 @@ impl PartialEq for SnapshotContents { if self.matches_latest(other) { true } else if self.matches_legacy(other) { - elog!("{} {}\n{}",style("Snapshot passes but is a legacy format. Please run `cargo insta test --force-update-snapshots --accept` to update to a newer format.").yellow().bold(),"Snapshot contents:", self.as_str()); + elog!("{} {}\n{}",style("Snapshot passes but is a legacy format. Please run `cargo insta test --force-update-snapshots --accept` to update to a newer format.").yellow().bold(),"Snapshot contents:", self.to_string()); true } else { false @@ -774,6 +771,8 @@ fn get_inline_snapshot_value(frozen_value: &str) -> String { elog!("A snapshot uses an old snapshot format; please update it to the new format with `cargo insta test --force-update-snapshots --accept`.\n\nSnapshot is at: {}", frozen_value); // legacy format - retain so old snapshots still work + // TODO: move this into `matches_legacy` after the current merge + // requests have settled. let mut buf = String::new(); let mut line_iter = frozen_value.lines(); let mut indentation = 0;