From 5c6ba53715b1285365b19caed47595569dba4311 Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Thu, 19 Sep 2024 09:01:35 -0700 Subject: [PATCH 1/2] Adjust force-update on inline snapshots to only view within the string (#581) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This solves the issue in #573 for the moment: - When `--force-update-snapshots` in passed, we only update inline snapshots when there's some difference _within_ the string, such as an additional linebreak at the start or the end - We no longer update the surrounding hashes. In the existing code, this happened because we were writing too many inline snapshots, not because we were actually checking the number of hashes - Checking the number of hashes isn't easy to do — reason outlined at https://github.com/mitsuhiko/insta/pull/573#issuecomment-2323500808 - The main change in the code is that we store the unnormalized snapshot and then normalize when we need to. The existing code stored the normalized version, which meant we couldn't evaluate whether the unnormalized versions were different It's a decent number of changes, but will integrate nicely with https://github.com/mitsuhiko/insta/pull/563. ~(FYI we currently don't look at the indentation, but we could adjust this)~ Now indentation works too --- CHANGELOG.md | 5 + cargo-insta/src/cli.rs | 6 +- cargo-insta/src/container.rs | 19 +-- cargo-insta/src/walk.rs | 6 +- cargo-insta/tests/main.rs | 155 ++++++++++++++++++++-- insta/src/lib.rs | 2 +- insta/src/output.rs | 12 +- insta/src/runtime.rs | 38 +++++- insta/src/snapshot.rs | 249 ++++++++++++++++++----------------- 9 files changed, 332 insertions(+), 160 deletions(-) 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` diff --git a/cargo-insta/src/cli.rs b/cargo-insta/src/cli.rs index 03c775ba..e3eafafe 100644 --- a/cargo-insta/src/cli.rs +++ b/cargo-insta/src/cli.rs @@ -1077,12 +1077,14 @@ fn pending_snapshots_cmd(cmd: PendingSnapshotsCommand) -> Result<(), Box, patcher: Option, } @@ -63,11 +58,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 { @@ -83,7 +78,7 @@ impl SnapshotContainer { }); None } - SnapshotContainerKind::Inline => { + SnapshotKind::Inline => { let mut pending_vec = PendingInlineSnapshot::load_batch(&snapshot_path)?; let mut have_new = false; @@ -139,8 +134,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 5ff9f754..d94c651a 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_pending_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_pending_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 3d8c2775..f1c65526 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -905,13 +905,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-linebreaks" version = "0.1.0" edition = "2021" @@ -924,8 +924,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(), @@ -950,16 +953,143 @@ 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-hashes" +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_inline_snapshot_indent() { + let test_project = TestFiles::new() + .add_file( + "Cargo.toml", + r#" +[package] +name = "inline-indent" +version = "0.1.0" +edition = "2021" + +[dependencies] +insta = { path = '$PROJECT_PATH' } +"# + .to_string(), + ) + .add_file( + "src/lib.rs", + r#####" +#[test] +fn test_wrong_indent_force() { + insta::assert_snapshot!(r#" + foo + foo + "#, @r#" + foo + foo + "#); +} +"##### + .to_string(), + ) + .create_project(); + + // Confirm the test passes despite the indent + let output = test_project + .cmd() + .args(["test", "--check", "--", "--nocapture"]) + .output() + .unwrap(); + assert_success(&output); + + // Then run the test with --force-update-snapshots and --accept to confirm + // the new snapshot is written + let output = test_project + .cmd() + .args([ + "test", + "--force-update-snapshots", + "--accept", + "--", + "--nocapture", + ]) + .output() + .unwrap(); + assert_success(&output); + + // https://github.com/mitsuhiko/insta/pull/563 will fix the starting & + // ending newlines + assert_snapshot!(test_project.diff("src/lib.rs"), @r##" + --- Original: src/lib.rs + +++ Updated: src/lib.rs + @@ -5,7 +5,7 @@ + foo + foo + "#, @r#" + - foo + - foo + + foo + + foo + "#); + } + "##); +} + #[test] fn test_hashtag_escape_in_inline_snapshot() { let test_project = TestFiles::new() @@ -981,7 +1111,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(), @@ -999,13 +1129,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/lib.rs b/insta/src/lib.rs index 4de7fc24..574d3807 100644 --- a/insta/src/lib.rs +++ b/insta/src/lib.rs @@ -287,7 +287,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/output.rs b/insta/src/output.rs index 075e914c..5c9d103b 100644 --- a/insta/src/output.rs +++ b/insta/src/output.rs @@ -103,7 +103,7 @@ impl<'a> 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 { @@ -118,15 +118,17 @@ impl<'a> SnapshotPrinter<'a> { } fn print_changeset(&self) { - let old = self.old_snapshot.as_ref().map_or("", |x| x.contents_str()); - let new = self.new_snapshot.contents_str(); - let newlines_matter = newlines_matter(old, new); + let old: String = self + .old_snapshot + .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(); 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 22deb330..ad8a1ddd 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> = @@ -78,6 +81,7 @@ impl<'a> From<&'a str> for ReferenceValue<'a> { } } +#[derive(Debug)] /// A reference to a snapshot pub enum ReferenceValue<'a> { /// A file snapshot, where the inner value is the snapshot name. @@ -289,7 +293,7 @@ impl<'a> SnapshotAssertionContext<'a> { module_path.replace("::", "__"), None, MetaData::default(), - SnapshotContents::from_inline(contents), + SnapshotContents::new(contents.to_string(), SnapshotKind::Inline), )); } }; @@ -643,7 +647,12 @@ 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(SnapshotContents::new(new_snapshot_value.into(), kind), expr); // memoize the snapshot file if requested. if let Some(ref snapshot_file) = ctx.snapshot_file { @@ -678,7 +687,22 @@ pub fn assert_snapshot( tool_config.snapshot_update(), crate::env::SnapshotUpdate::Force ) { - 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 9ff600b0..05463c56 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}, @@ -93,8 +93,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)?) + } _ => {} } } @@ -261,7 +265,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; @@ -272,6 +277,12 @@ impl MetaData { } } +#[derive(Debug, Clone, Copy)] +pub enum SnapshotKind { + Inline, + File, +} + /// A helper to work with file snapshots. #[derive(Debug, Clone)] pub struct Snapshot { @@ -304,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 { @@ -344,7 +357,10 @@ impl Snapshot { module_name, Some(snapshot_name), metadata, - buf.into(), + SnapshotContents { + contents: buf, + kind: SnapshotKind::File, + }, )) } @@ -364,7 +380,7 @@ impl 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; @@ -377,12 +393,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, + }); } _ => {} } @@ -405,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.0.as_str()))); + fields.push(("snapshot", Content::from(self.snapshot.to_string()))); Content::Struct("Content", fields) } @@ -435,21 +452,34 @@ impl Snapshot { self.contents() == other.contents() } - /// Snapshot contents _and_ metadata match another snapshot's. + pub fn kind(&self) -> SnapshotKind { + self.snapshot.kind + } + + /// 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 { - self.snapshot.matches_fully(&other.snapshot) - && self.metadata.trim_for_persistence() == other.metadata.trim_for_persistence() + 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() + && contents_match_exact + } + SnapshotKind::Inline => contents_match_exact, + } } - /// The snapshot contents as a &str - pub fn contents_str(&self) -> &str { - self.snapshot.as_str() + /// The normalized snapshot contents as a String + pub fn contents_string(&self) -> String { + self.snapshot.normalize() } 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_string().as_str()); buf.push('\n'); buf } @@ -517,29 +547,23 @@ 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 { - SnapshotContents(get_inline_snapshot_value(value)) - } - - /// Returns the snapshot contents as a normalized string (for example, - /// removing surrounding whitespace) - 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 { + // We could store a normalized version of the string as part of `new`; + // 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 string without any normalization pub fn as_str_exact(&self) -> &str { - self.0.as_str() + self.contents.as_str() } /// Matches another snapshot without any normalization @@ -549,26 +573,26 @@ 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) -> &str { - let out = sc.as_str(); + fn as_str_legacy(sc: &SnapshotContents) -> String { + let out = sc.to_string(); // Legacy inline snapshots have `---` at the start, so this strips that if // it exists. match out.strip_prefix("---\n") { - Some(old_snapshot) => old_snapshot, + Some(old_snapshot) => old_snapshot.to_string(), None => out, } } as_str_legacy(self) == as_str_legacy(other) } - /// Create the literal string to write inline, adding `#` to escape the - /// value, indentation, delimiters + /// 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.normalize(); let mut out = String::new(); // We don't technically need to escape on newlines, but it reduces diffs @@ -612,7 +636,7 @@ impl SnapshotContents { .chain(Some(format!("\n{:width$}", "", width = indentation))), ); } else { - out.push_str(contents); + out.push_str(contents.as_str()); } out.push('"'); @@ -620,34 +644,25 @@ 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), - } - } -} - -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")) + 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(); + out.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 a normalized string (for example, + /// removing surrounding whitespace) + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.normalize()) } } @@ -657,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 @@ -689,9 +704,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") @@ -751,13 +764,15 @@ 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('⋮') { 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; @@ -801,14 +816,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 @@ -816,35 +830,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 +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 @@ -852,28 +854,26 @@ b"[1..]; "#"## ); - let t = &" - ab -"[1..]; assert_eq!( - SnapshotContents(t.to_string()).to_inline(0), - r##"r#" - ab -"#"## + SnapshotContents::new("\n ab\n".to_string(), SnapshotKind::Inline).to_inline(0), + 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 @@ -891,10 +891,12 @@ fn test_normalize_inline_snapshot() { r#" 1 2 - "# + "# ), - r###"1 -2"### + r###" +1 +2 +"### ); assert_eq!( @@ -903,7 +905,8 @@ fn test_normalize_inline_snapshot() { 1 2"# ), - r###" 1 + r###" + 1 2"### ); @@ -914,8 +917,10 @@ fn test_normalize_inline_snapshot() { 2 "# ), - r###"1 -2"### + r###" +1 +2 +"### ); assert_eq!( @@ -925,7 +930,8 @@ fn test_normalize_inline_snapshot() { 2 "# ), - r###"1 + r###" +1 2"### ); @@ -935,7 +941,9 @@ fn test_normalize_inline_snapshot() { a "# ), - "a" + " +a +" ); assert_eq!(normalize_inline_snapshot(""), ""); @@ -948,9 +956,11 @@ fn test_normalize_inline_snapshot() { c "# ), - r###" a + r###" + a b -c"### +c + "### ); assert_eq!( @@ -959,7 +969,9 @@ c"### a "# ), - "a" + " +a + " ); assert_eq!( @@ -967,7 +979,8 @@ a " a" ), - "a" + " +a" ); assert_eq!( From 71e7340b257d73005634af5bb09a7633bb442ab5 Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Thu, 19 Sep 2024 09:05:14 -0700 Subject: [PATCH 2/2] Remove duplicate write check (#583) Stacks on #581, merge that first Now that we've consolidated the checks for whether to write snapshots into `matches_fully`, I think we can remove the check on whether the file contents match. --- insta/src/runtime.rs | 24 +++++++++------------- insta/src/snapshot.rs | 46 ++++++++----------------------------------- 2 files changed, 17 insertions(+), 53 deletions(-) diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index ad8a1ddd..38cb3a4e 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -389,8 +389,8 @@ impl<'a> SnapshotAssertionContext<'a> { match snapshot_update { SnapshotUpdateBehavior::InPlace => { if let Some(ref snapshot_file) = self.snapshot_file { - let saved = new_snapshot.save(snapshot_file)?; - if should_print && saved { + new_snapshot.save(snapshot_file)?; + if should_print { elog!( "{} {}", if unseen { @@ -409,14 +409,13 @@ impl<'a> SnapshotAssertionContext<'a> { SnapshotUpdateBehavior::NewFile => { if let Some(ref snapshot_file) = self.snapshot_file { // File snapshot - if let Some(new_path) = new_snapshot.save_new(snapshot_file)? { - if should_print { - elog!( - "{} {}", - style("stored new snapshot").green(), - style(new_path.display()).cyan().underlined(), - ); - } + let new_path = new_snapshot.save_new(snapshot_file)?; + if should_print { + elog!( + "{} {}", + style("stored new snapshot").green(), + style(new_path.display()).cyan().underlined(), + ); } } else if self.is_doctest { if should_print { @@ -690,11 +689,6 @@ pub fn assert_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() diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index 05463c56..af6c654e 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -484,40 +484,14 @@ impl Snapshot { buf } - fn save_with_metadata( - &self, - path: &Path, - ref_file: Option<&Path>, - md: &MetaData, - ) -> Result> { + fn save_with_metadata(&self, path: &Path, md: &MetaData) -> Result<(), Box> { if let Some(folder) = path.parent() { fs::create_dir_all(folder)?; } let serialized_snapshot = self.serialize_snapshot(md); - - // check the reference file for contents. Note that we always want to - // compare snapshots that were trimmed to persistence here. This is a - // stricter check than even `matches_fully`, since it's comparing the - // exact contents of the file. - if let Ok(old) = fs::read_to_string(ref_file.unwrap_or(path)) { - let persisted = match md.trim_for_persistence() { - Cow::Owned(trimmed) => Cow::Owned(self.serialize_snapshot(&trimmed)), - Cow::Borrowed(trimmed) => { - // This condition needs to hold, otherwise we need to - // compare the old value to a newly trimmed serialized snapshot - debug_assert_eq!(trimmed, md); - - Cow::Borrowed(&serialized_snapshot) - } - }; - if old == persisted.as_str() { - return Ok(false); - } - } - fs::write(path, serialized_snapshot)?; - Ok(true) + Ok(()) } /// Saves the snapshot. @@ -525,22 +499,18 @@ impl Snapshot { /// Returns `true` if the snapshot was saved. This will return `false` if there /// was already a snapshot with matching contents. #[doc(hidden)] - pub fn save(&self, path: &Path) -> Result> { - self.save_with_metadata(path, None, &self.metadata.trim_for_persistence()) + pub fn save(&self, path: &Path) -> Result<(), Box> { + self.save_with_metadata(path, &self.metadata.trim_for_persistence()) } /// Same as `save` but instead of writing a normal snapshot file this will write /// a `.snap.new` file with additional information. /// - /// If the existing snapshot matches the new file, then `None` is returned, otherwise - /// the name of the new snapshot file. - pub(crate) fn save_new(&self, path: &Path) -> Result, Box> { + /// The name of the new snapshot file is returned. + pub(crate) fn save_new(&self, path: &Path) -> Result> { let new_path = path.to_path_buf().with_extension("snap.new"); - if self.save_with_metadata(&new_path, Some(path), &self.metadata)? { - Ok(Some(new_path)) - } else { - Ok(None) - } + self.save_with_metadata(&new_path, &self.metadata)?; + Ok(new_path) } }