From e0612ae22e3ad287443928ea6ef140d78854bbb2 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Wed, 18 Sep 2024 22:31:14 +0200 Subject: [PATCH 01/41] Add binary snapshots This is the somewhat simplified version of #489 where we keep the full files in-memory during the assert and when reviewing. --- Cargo.lock | 41 ++- cargo-insta/Cargo.toml | 1 + cargo-insta/src/cli.rs | 36 +- cargo-insta/src/container.rs | 6 + cargo-insta/src/inline.rs | 8 +- cargo-insta/tests/main.rs | 6 +- insta/src/content/mod.rs | 2 - insta/src/lib.rs | 4 +- insta/src/macros.rs | 38 ++- insta/src/output.rs | 235 ++++++++----- insta/src/runtime.rs | 149 ++++++-- insta/src/snapshot.rs | 322 ++++++++++++++---- .../test_binary__binary_snapshot.snap | 6 + .../test_binary__binary_snapshot.snap.txt | 1 + insta/tests/test_binary.rs | 4 + 15 files changed, 657 insertions(+), 202 deletions(-) create mode 100644 insta/tests/snapshots/test_binary__binary_snapshot.snap create mode 100644 insta/tests/snapshots/test_binary__binary_snapshot.snap.txt create mode 100644 insta/tests/test_binary.rs diff --git a/Cargo.lock b/Cargo.lock index 075e5cef..341e736f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -75,6 +75,7 @@ dependencies = [ "insta", "itertools", "lazy_static", + "open", "proc-macro2", "semver", "serde", @@ -391,6 +392,15 @@ dependencies = [ "windows-sys 0.48.0", ] +[[package]] +name = "is-docker" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "928bae27f42bc99b60d9ac7334e3a21d10ad8f1835a4e12ec3ec0464765ed1b3" +dependencies = [ + "once_cell", +] + [[package]] name = "is-terminal" version = "0.4.12" @@ -402,6 +412,16 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "is-wsl" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "173609498df190136aa7dea1a91db051746d339e18476eed5ca40521f02d7aa5" +dependencies = [ + "is-docker", + "once_cell", +] + [[package]] name = "itertools" version = "0.10.5" @@ -464,9 +484,26 @@ checksum = "78ca9ab1a0babb1e7d5695e3530886289c18cf2f87ec19a575a0abdce112e3a3" [[package]] name = "once_cell" -version = "1.14.0" +version = "1.19.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3fdb12b2476b595f9358c5161aa467c2438859caa136dec86c26fdd2efe17b92" + +[[package]] +name = "open" +version = "5.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "61a877bf6abd716642a53ef1b89fb498923a4afca5c754f9050b4d081c05c4b3" +dependencies = [ + "is-wsl", + "libc", + "pathdiff", +] + +[[package]] +name = "pathdiff" +version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2f7254b99e31cad77da24b08ebf628882739a608578bb1bcdfc1f9c21260d7c0" +checksum = "8835116a5c179084a830efb3adc117ab007512b535bc1a21c991d3b32a6b44dd" [[package]] name = "pest" diff --git a/cargo-insta/Cargo.toml b/cargo-insta/Cargo.toml index 9e95c6fd..bd052c3d 100644 --- a/cargo-insta/Cargo.toml +++ b/cargo-insta/Cargo.toml @@ -30,6 +30,7 @@ tempfile = "3.5.0" semver = {version = "1.0.7", features = ["serde"]} lazy_static = "1.4.0" clap = { workspace=true } +open = "5.3.0" [dev-dependencies] walkdir = "2.3.1" diff --git a/cargo-insta/src/cli.rs b/cargo-insta/src/cli.rs index 03c775ba..31a2a2aa 100644 --- a/cargo-insta/src/cli.rs +++ b/cargo-insta/src/cli.rs @@ -6,6 +6,7 @@ use std::{env, fs}; use std::{io, process}; use console::{set_colors_enabled, style, Key, Term}; +use insta::internals::SnapshotContents; use insta::Snapshot; use insta::_cargo_insta_support::{ is_ci, SnapshotPrinter, SnapshotUpdate, TestRunner, ToolConfig, UnreferencedSnapshots, @@ -321,6 +322,24 @@ fn query_snapshot( style("toggle snapshot diff").dim() ); + let new_is_binary = new.contents().is_binary(); + let old_is_binary = old.is_some_and(|o| o.contents().is_binary()); + + if new_is_binary || old_is_binary { + println!( + " {} open {}", + style("o").cyan().bold(), + style(if new_is_binary && old_is_binary { + "open snapshot files in external tool" + } else if new_is_binary { + "open new snapshot file in external tool" + } else { + "open old snapshot file in external tool" + }) + .dim() + ); + } + loop { match term.read_key()? { Key::Char('a') | Key::Enter => return Ok(Operation::Accept), @@ -334,6 +353,19 @@ fn query_snapshot( *show_diff = !*show_diff; break; } + Key::Char('o') => { + if let Some(SnapshotContents::Binary(contents)) = old.map(|o| o.contents()) { + open::that_detached(contents.build_path(snapshot_file.unwrap()))?; + } + + if let SnapshotContents::Binary(contents) = new.contents() { + open::that_detached( + contents.build_path(snapshot_file.unwrap().with_extension("snap.new")), + )?; + } + + // there's no break here because there's no need to re-output anything + } _ => {} } } @@ -1081,8 +1113,8 @@ fn pending_snapshots_cmd(cmd: PendingSnapshotsCommand) -> Result<(), Box {} } + + if let Operation::Accept | Operation::Reject = snapshot.op { + let snapshot = Snapshot::from_file(&self.target_path).ok(); + + Snapshot::cleanup_extra_files(snapshot.as_ref(), &self.target_path)?; + } } } Ok(()) diff --git a/cargo-insta/src/inline.rs b/cargo-insta/src/inline.rs index b0ee5150..778e4e2f 100644 --- a/cargo-insta/src/inline.rs +++ b/cargo-insta/src/inline.rs @@ -104,8 +104,12 @@ impl FilePatcher { .collect(); // replace lines - let snapshot_line_contents = - [prefix, snapshot.to_inline(inline.indentation), suffix].join(""); + let snapshot_line_contents = [ + prefix, + snapshot.to_inline(inline.indentation).unwrap(), + suffix, + ] + .join(""); self.lines.splice( inline.start.0..=inline.end.0, diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index 3d8c2775..115e6976 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -876,12 +876,13 @@ Hello, world! assert_snapshot!(test_current_insta.diff("src/snapshots/test_force_update_current__force_update.snap"), @r#" --- Original: src/snapshots/test_force_update_current__force_update.snap +++ Updated: src/snapshots/test_force_update_current__force_update.snap - @@ -1,8 +1,5 @@ + @@ -1,8 +1,6 @@ - --- source: src/lib.rs -expression: +expression: "\"Hello, world!\"" + +snapshot_type: string --- Hello, world! - @@ -891,12 +892,13 @@ Hello, world! assert_snapshot!(test_insta_1_40_0.diff("src/snapshots/test_force_update_1_40_0__force_update.snap"), @r#" --- Original: src/snapshots/test_force_update_1_40_0__force_update.snap +++ Updated: src/snapshots/test_force_update_1_40_0__force_update.snap - @@ -1,8 +1,5 @@ + @@ -1,8 +1,6 @@ - --- source: src/lib.rs -expression: +expression: "\"Hello, world!\"" + +snapshot_type: string --- Hello, world! - diff --git a/insta/src/content/mod.rs b/insta/src/content/mod.rs index 8fe5e14e..435e922c 100644 --- a/insta/src/content/mod.rs +++ b/insta/src/content/mod.rs @@ -22,7 +22,6 @@ use std::fmt; pub enum Error { FailedParsingYaml(std::path::PathBuf), UnexpectedDataType, - #[cfg(feature = "_cargo_insta_internal")] MissingField, #[cfg(feature = "_cargo_insta_internal")] FileIo(std::io::Error, std::path::PathBuf), @@ -37,7 +36,6 @@ impl fmt::Display for Error { Error::UnexpectedDataType => { f.write_str("The present data type wasn't what was expected") } - #[cfg(feature = "_cargo_insta_internal")] Error::MissingField => f.write_str("A required field was missing"), #[cfg(feature = "_cargo_insta_internal")] Error::FileIo(e, p) => { diff --git a/insta/src/lib.rs b/insta/src/lib.rs index 4de7fc24..55b15861 100644 --- a/insta/src/lib.rs +++ b/insta/src/lib.rs @@ -333,7 +333,9 @@ pub use crate::redaction::{dynamic_redaction, rounded_redaction, sorted_redactio pub mod _macro_support { pub use crate::content::Content; pub use crate::env::get_cargo_workspace; - pub use crate::runtime::{assert_snapshot, with_allow_duplicates, AutoName, ReferenceValue}; + pub use crate::runtime::{ + assert_snapshot, with_allow_duplicates, AutoName, InlineValue, SnapshotValue, + }; #[cfg(feature = "serde")] pub use crate::serialization::{serialize_value, SerializationFormat, SnapshotLocation}; diff --git a/insta/src/macros.rs b/insta/src/macros.rs index ba837e40..e06dcae8 100644 --- a/insta/src/macros.rs +++ b/insta/src/macros.rs @@ -326,7 +326,7 @@ macro_rules! _assert_snapshot_base { $crate::_assert_snapshot_base!( transform = $transform, #[allow(clippy::needless_raw_string_hashes)] - $crate::_macro_support::ReferenceValue::Inline($snapshot), + $crate::_macro_support::InlineValue($snapshot), $($arg),* ) }; @@ -346,9 +346,39 @@ macro_rules! _assert_snapshot_base { // The main macro body — every call to this macro should end up here. (transform=$transform:expr, $name:expr, $value:expr, $debug_expr:expr $(,)?) => { $crate::_macro_support::assert_snapshot( - $name.into(), - #[allow(clippy::redundant_closure_call)] - &$transform(&$value), + ( + $name, + #[allow(clippy::redundant_closure_call)] + $transform(&$value).as_str(), + ).into(), + env!("CARGO_MANIFEST_DIR"), + $crate::_function_name!(), + module_path!(), + file!(), + line!(), + $debug_expr, + ) + .unwrap() + }; +} + +#[macro_export] +macro_rules! assert_binary_snapshot { + ($extension:expr, $value:expr $(,)?) => { + $crate::assert_binary_snapshot!($extension, $crate::_macro_support::AutoName, $value); + }; + + ($extension:expr, $name:expr, $value:expr $(,)?) => { + $crate::assert_binary_snapshot!($extension, $name, $value, stringify!($value)); + }; + + ($extension:expr, $name:expr, $value:expr, $debug_expr:expr $(,)?) => { + $crate::_macro_support::assert_snapshot( + $crate::_macro_support::SnapshotValue::Binary { + name: $name.into(), + content: $value, + extension: $extension, + }, env!("CARGO_MANIFEST_DIR"), $crate::_function_name!(), module_path!(), diff --git a/insta/src/output.rs b/insta/src/output.rs index 075e914c..7bd864e2 100644 --- a/insta/src/output.rs +++ b/insta/src/output.rs @@ -4,7 +4,7 @@ use std::{path::Path, time::Duration}; use similar::{Algorithm, ChangeTag, TextDiff}; use crate::content::yaml; -use crate::snapshot::{MetaData, Snapshot}; +use crate::snapshot::{MetaData, Snapshot, SnapshotContents, SnapshotContentsString}; use crate::utils::{format_rust_expression, style, term_width}; /// Snapshot printer utility. @@ -103,119 +103,183 @@ 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(); let width = term_width(); if self.show_info { self.print_info(); } println!("Snapshot Contents:"); - println!("──────┬{:─^1$}", "", width.saturating_sub(7)); - for (idx, line) in new_contents.lines().enumerate() { - println!("{:>5} │ {}", style(idx + 1).cyan().dim().bold(), line); + + match new_contents { + SnapshotContents::String(SnapshotContentsString(new_contents)) => { + println!("──────┬{:─^1$}", "", width.saturating_sub(7)); + for (idx, line) in new_contents.lines().enumerate() { + println!("{:>5} │ {}", style(idx + 1).cyan().dim().bold(), line); + } + println!("──────┴{:─^1$}", "", width.saturating_sub(7)); + } + SnapshotContents::Binary(new_contents) => { + println!( + "{}", + encode_file_link_escape( + &new_contents + .build_path(self.snapshot_file.unwrap().with_extension("snap.new")) + ) + ); + } } - println!("──────┴{:─^1$}", "", width.saturating_sub(7)); } 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 width = term_width(); - let diff = TextDiff::configure() - .algorithm(Algorithm::Patience) - .timeout(Duration::from_millis(500)) - .diff_lines(old, new); print_line(width); if self.show_info { self.print_info(); } - if !old.is_empty() { + let old_contents = self.old_snapshot.as_ref().map(|o| o.contents()); + let new_contents = self.new_snapshot.contents(); + + if let Some(SnapshotContents::Binary(contents)) = old_contents { println!( "{}", - style(format_args!("-{}", self.old_snapshot_hint)).red() + style(format_args!( + "-{}: {}", + self.old_snapshot_hint, + encode_file_link_escape(&contents.build_path(self.snapshot_file.unwrap())), + )) + .red() ); } - println!( - "{}", - style(format_args!("+{}", self.new_snapshot_hint)).green() - ); - println!("────────────┬{:─^1$}", "", width.saturating_sub(13)); - let mut has_changes = false; - for (idx, group) in diff.grouped_ops(4).iter().enumerate() { - if idx > 0 { - println!("┈┈┈┈┈┈┈┈┈┈┈┈┼{:┈^1$}", "", width.saturating_sub(13)); + if let SnapshotContents::Binary(contents) = new_contents { + println!( + "{}", + style(format_args!( + "+{}: {}", + self.new_snapshot_hint, + encode_file_link_escape( + &contents + .build_path(self.snapshot_file.unwrap().with_extension("snap.new")) + ), + )) + .green() + ); + } + + if let Some((old, new)) = match ( + self.old_snapshot.as_ref().map(|o| o.contents()), + self.new_snapshot.contents(), + ) { + (Some(SnapshotContents::Binary { .. }) | None, SnapshotContents::String(new)) => { + Some((None, Some(&new.0))) + } + (Some(SnapshotContents::String(old)), SnapshotContents::Binary { .. }) => { + Some((Some(&old.0), None)) + } + (Some(SnapshotContents::String(old)), SnapshotContents::String(new)) => { + Some((Some(&old.0), Some(&new.0))) + } + _ => None, + } { + let old_text = dbg!(old).map(String::as_str).unwrap_or(""); + let new_text = dbg!(new).map(String::as_str).unwrap_or(""); + + let newlines_matter = newlines_matter(old_text, new_text); + let diff = TextDiff::configure() + .algorithm(Algorithm::Patience) + .timeout(Duration::from_millis(500)) + .diff_lines(old_text, new_text); + + if old.is_some() { + println!( + "{}", + style(format_args!("-{}", self.old_snapshot_hint)).red() + ); + } + + if new.is_some() { + println!( + "{}", + style(format_args!("+{}", self.new_snapshot_hint)).green() + ); } - for op in group { - for change in diff.iter_inline_changes(op) { - match change.tag() { - ChangeTag::Insert => { - has_changes = true; - print!( - "{:>5} {:>5} │{}", - "", - style(change.new_index().unwrap()).cyan().dim().bold(), - style("+").green(), - ); - for &(emphasized, change) in change.values() { - let change = render_invisible(change, newlines_matter); - if emphasized { - print!("{}", style(change).green().underlined()); - } else { - print!("{}", style(change).green()); + + println!("────────────┬{:─^1$}", "", width.saturating_sub(13)); + let mut has_changes = false; + for (idx, group) in diff.grouped_ops(4).iter().enumerate() { + if idx > 0 { + println!("┈┈┈┈┈┈┈┈┈┈┈┈┼{:┈^1$}", "", width.saturating_sub(13)); + } + for op in group { + for change in diff.iter_inline_changes(op) { + match change.tag() { + ChangeTag::Insert => { + has_changes = true; + print!( + "{:>5} {:>5} │{}", + "", + style(change.new_index().unwrap()).cyan().dim().bold(), + style("+").green(), + ); + for &(emphasized, change) in change.values() { + let change = render_invisible(change, newlines_matter); + if emphasized { + print!("{}", style(change).green().underlined()); + } else { + print!("{}", style(change).green()); + } } } - } - ChangeTag::Delete => { - has_changes = true; - print!( - "{:>5} {:>5} │{}", - style(change.old_index().unwrap()).cyan().dim(), - "", - style("-").red(), - ); - for &(emphasized, change) in change.values() { - let change = render_invisible(change, newlines_matter); - if emphasized { - print!("{}", style(change).red().underlined()); - } else { - print!("{}", style(change).red()); + ChangeTag::Delete => { + has_changes = true; + print!( + "{:>5} {:>5} │{}", + style(change.old_index().unwrap()).cyan().dim(), + "", + style("-").red(), + ); + for &(emphasized, change) in change.values() { + let change = render_invisible(change, newlines_matter); + if emphasized { + print!("{}", style(change).red().underlined()); + } else { + print!("{}", style(change).red()); + } } } - } - ChangeTag::Equal => { - print!( - "{:>5} {:>5} │ ", - style(change.old_index().unwrap()).cyan().dim(), - style(change.new_index().unwrap()).cyan().dim().bold(), - ); - for &(_, change) in change.values() { - let change = render_invisible(change, newlines_matter); - print!("{}", style(change).dim()); + ChangeTag::Equal => { + print!( + "{:>5} {:>5} │ ", + style(change.old_index().unwrap()).cyan().dim(), + style(change.new_index().unwrap()).cyan().dim().bold(), + ); + for &(_, change) in change.values() { + let change = render_invisible(change, newlines_matter); + print!("{}", style(change).dim()); + } } } - } - if change.missing_newline() { - println!(); + if change.missing_newline() { + println!(); + } } } } - } - if !has_changes { - println!( - "{:>5} {:>5} │{}", - "", - style("-").dim(), - style(" snapshots are matching").cyan(), - ); - } + if !has_changes { + println!( + "{:>5} {:>5} │{}", + "", + style("-").dim(), + style(" snapshots are matching").cyan(), + ); + } - println!("────────────┴{:─^1$}", "", width.saturating_sub(13)); + println!("────────────┴{:─^1$}", "", width.saturating_sub(13)); + } } } @@ -356,6 +420,17 @@ fn print_info(metadata: &MetaData) { } } +/// Encodes a path as an OSC-8 escape sequence. This makes it a clickable link in supported +/// terminal emulators. +fn encode_file_link_escape(path: &Path) -> String { + assert!(path.is_absolute()); + format!( + "\x1b]8;;file://{}\x1b\\{}\x1b]8;;\x1b\\", + path.display(), + path.display() + ) +} + #[test] fn test_invisible() { assert_eq!( diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 22deb330..2fe4eeb2 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -4,6 +4,7 @@ use std::error::Error; use std::fs; use std::io::Write; use std::path::{Path, PathBuf}; +use std::rc::Rc; use std::str; use std::sync::{Arc, Mutex}; use std::{borrow::Cow, env}; @@ -14,7 +15,10 @@ use crate::env::{ }; use crate::output::SnapshotPrinter; use crate::settings::Settings; -use crate::snapshot::{MetaData, PendingInlineSnapshot, Snapshot, SnapshotContents}; +use crate::snapshot::{ + MetaData, PendingInlineSnapshot, Snapshot, SnapshotContents, SnapshotContentsBinary, + SnapshotType, +}; use crate::utils::{path_to_storage, style}; lazy_static::lazy_static! { @@ -48,42 +52,86 @@ macro_rules! elog { #[derive(Debug)] pub struct AutoName; -impl From for ReferenceValue<'static> { - fn from(_value: AutoName) -> ReferenceValue<'static> { - ReferenceValue::File(None) +pub struct InlineValue<'a>(pub &'a str); + +impl From for SnapshotName<'static> { + fn from(_value: AutoName) -> SnapshotName<'static> { + None } } -impl From> for ReferenceValue<'static> { - fn from(value: Option) -> ReferenceValue<'static> { - ReferenceValue::File(value.map(Cow::Owned)) +type SnapshotName<'a> = Option>; + +pub enum SnapshotValue<'a> { + String { + name: SnapshotName<'a>, + content: &'a str, + }, + + InlineString { + reference_content: &'a str, + content: &'a str, + }, + + Binary { + name: SnapshotName<'a>, + content: Vec, + extension: &'a str, + }, +} + +impl<'a> From<(AutoName, &'a str)> for SnapshotValue<'a> { + fn from((_, content): (AutoName, &'a str)) -> Self { + SnapshotValue::String { + name: None, + content, + } } } -impl From for ReferenceValue<'static> { - fn from(value: String) -> ReferenceValue<'static> { - ReferenceValue::File(Some(Cow::Owned(value))) +impl<'a> From<(Option, &'a str)> for SnapshotValue<'a> { + fn from((name, content): (Option, &'a str)) -> Self { + SnapshotValue::String { + name: name.map(Cow::Owned), + content, + } } } -impl<'a> From> for ReferenceValue<'a> { - fn from(value: Option<&'a str>) -> ReferenceValue<'a> { - ReferenceValue::File(value.map(Cow::Borrowed)) +impl<'a> From<(String, &'a str)> for SnapshotValue<'a> { + fn from((name, content): (String, &'a str)) -> Self { + SnapshotValue::String { + name: Some(Cow::Owned(name)), + content, + } } } -impl<'a> From<&'a str> for ReferenceValue<'a> { - fn from(value: &'a str) -> ReferenceValue<'a> { - ReferenceValue::File(Some(Cow::Borrowed(value))) +impl<'a> From<(Option<&'a str>, &'a str)> for SnapshotValue<'a> { + fn from((name, content): (Option<&'a str>, &'a str)) -> Self { + SnapshotValue::String { + name: name.map(Cow::Borrowed), + content, + } } } -/// A reference to a snapshot -pub enum ReferenceValue<'a> { - /// A file snapshot, where the inner value is the snapshot name. - File(Option>), - /// An inline snapshot, where the inner value is the snapshot contents. - Inline(&'a str), +impl<'a> From<(&'a str, &'a str)> for SnapshotValue<'a> { + fn from((name, content): (&'a str, &'a str)) -> Self { + SnapshotValue::String { + name: Some(Cow::Borrowed(name)), + content, + } + } +} + +impl<'a> From<(InlineValue<'a>, &'a str)> for SnapshotValue<'a> { + fn from((InlineValue(reference_content), content): (InlineValue<'a>, &'a str)) -> Self { + SnapshotValue::InlineString { + reference_content, + content, + } + } } fn is_doctest(function_name: &str) -> bool { @@ -217,7 +265,7 @@ struct SnapshotAssertionContext<'a> { impl<'a> SnapshotAssertionContext<'a> { fn prepare( - refval: ReferenceValue<'a>, + new_snapshot_value: &SnapshotValue<'a>, manifest_dir: &'a str, function_name: &'a str, module_path: &'a str, @@ -233,10 +281,10 @@ impl<'a> SnapshotAssertionContext<'a> { let mut pending_snapshots_path = None; let is_doctest = is_doctest(function_name); - match refval { - ReferenceValue::File(name) => { + match new_snapshot_value { + SnapshotValue::String { name, .. } | SnapshotValue::Binary { name, .. } => { let name = match name { - Some(name) => add_suffix_to_snapshot_name(name), + Some(name) => add_suffix_to_snapshot_name(name.clone()), None => { if is_doctest { panic!("Cannot determine reliable names for snapshot in doctests. Please use explicit names instead."); @@ -263,7 +311,10 @@ impl<'a> SnapshotAssertionContext<'a> { snapshot_name = Some(name); snapshot_file = Some(file); } - ReferenceValue::Inline(contents) => { + SnapshotValue::InlineString { + reference_content: contents, + .. + } => { if allow_duplicates() { duplication_key = Some(format!( "inline:{}|{}|{}", @@ -335,6 +386,12 @@ impl<'a> SnapshotAssertionContext<'a> { .input_file() .and_then(|x| self.localize_path(x)) .map(|x| path_to_storage(&x)), + snapshot_type: match contents { + SnapshotContents::String(_) => SnapshotType::String, + SnapshotContents::Binary(ref contents) => SnapshotType::Binary { + extension: contents.extension.clone(), + }, + }, }), contents, ) @@ -618,8 +675,7 @@ where /// assertion with a panic if needed. #[allow(clippy::too_many_arguments)] pub fn assert_snapshot( - refval: ReferenceValue<'_>, - new_snapshot_value: &str, + new_snapshot_value: SnapshotValue<'_>, manifest_dir: &str, function_name: &str, module_path: &str, @@ -628,7 +684,7 @@ pub fn assert_snapshot( expr: &str, ) -> Result<(), Box> { let ctx = SnapshotAssertionContext::prepare( - refval, + &new_snapshot_value, manifest_dir, function_name, module_path, @@ -638,12 +694,35 @@ pub fn assert_snapshot( let tool_config = get_tool_config(manifest_dir); - // apply filters if they are available - #[cfg(feature = "filters")] - let new_snapshot_value = - Settings::with(|settings| settings.filters().apply_to(new_snapshot_value)); + if let Some(snapshot_file) = &ctx.snapshot_file { + Snapshot::cleanup_extra_files(ctx.old_snapshot.as_ref(), snapshot_file)?; + } + + let content = match new_snapshot_value { + SnapshotValue::String { content, .. } | SnapshotValue::InlineString { content, .. } => { + // apply filters if they are available + #[cfg(feature = "filters")] + let content = Settings::with(|settings| settings.filters().apply_to(content)); + + content.into() + } + SnapshotValue::Binary { + content, extension, .. + } => { + assert!(extension != "new", "this file extension is not allowed"); + assert!( + !extension.starts_with("new."), + "file extensions starting with 'new.' are not allowed", + ); + + SnapshotContents::Binary(SnapshotContentsBinary { + contents: Rc::new(content), + extension: extension.to_string(), + }) + } + }; - let new_snapshot = ctx.new_snapshot(new_snapshot_value.into(), expr); + let new_snapshot = ctx.new_snapshot(content, 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 9ff600b0..01453b06 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -4,6 +4,7 @@ use std::error::Error; use std::fs; use std::io::{BufRead, BufReader, Write}; use std::path::{Path, PathBuf}; +use std::rc::Rc; use std::time::{SystemTime, UNIX_EPOCH}; use crate::{ @@ -134,6 +135,18 @@ impl PendingInlineSnapshot { } } +#[derive(Debug, Clone, PartialEq)] +pub enum SnapshotType { + String, + Binary { extension: String }, +} + +impl Default for SnapshotType { + fn default() -> Self { + SnapshotType::String + } +} + /// Snapshot metadata information. #[derive(Debug, Default, Clone, PartialEq)] pub struct MetaData { @@ -150,6 +163,8 @@ pub struct MetaData { pub(crate) info: Option, /// Reference to the input file. pub(crate) input_file: Option, + /// The type of the snapshot (string or binary). + pub(crate) snapshot_type: SnapshotType, } impl MetaData { @@ -203,6 +218,13 @@ impl MetaData { let mut expression = None; let mut info = None; let mut input_file = None; + let mut snapshot_type = TmpSnapshotType::String; + let mut extension = None; + + enum TmpSnapshotType { + String, + Binary, + } for (key, value) in map.into_iter() { match key.as_str() { @@ -212,6 +234,15 @@ impl MetaData { Some("expression") => expression = value.as_str().map(Into::into), Some("info") if !value.is_nil() => info = Some(value), Some("input_file") => input_file = value.as_str().map(Into::into), + Some("snapshot_type") => { + snapshot_type = match value.as_str() { + Some("binary") => TmpSnapshotType::Binary, + _ => TmpSnapshotType::String, + } + } + Some("extension") => { + extension = value.as_str().map(Into::into); + } _ => {} } } @@ -223,6 +254,12 @@ impl MetaData { expression, info, input_file, + snapshot_type: match snapshot_type { + TmpSnapshotType::String => SnapshotType::String, + TmpSnapshotType::Binary => SnapshotType::Binary { + extension: extension.ok_or(content::Error::MissingField)?, + }, + }, }) } else { Err(content::Error::UnexpectedDataType.into()) @@ -250,6 +287,16 @@ impl MetaData { fields.push(("input_file", Content::from(input_file))); } + let snapshot_type = Content::from(match self.snapshot_type { + SnapshotType::String => "string", + SnapshotType::Binary { ref extension } => { + fields.push(("extension", Content::from(extension.clone()))); + "binary" + } + }); + + fields.push(("snapshot_type", snapshot_type)); + Content::Struct("MetaData", fields) } @@ -329,14 +376,29 @@ impl Snapshot { rv }; - buf.clear(); - for (idx, line) in f.lines().enumerate() { - let line = line?; - if idx > 0 { - buf.push('\n'); + let contents = match metadata.snapshot_type { + SnapshotType::String => { + buf.clear(); + for (idx, line) in f.lines().enumerate() { + let line = line?; + if idx > 0 { + buf.push('\n'); + } + buf.push_str(&line); + } + + SnapshotContents::String(SnapshotContentsString(buf)) } - buf.push_str(&line); - } + SnapshotType::Binary { ref extension } => { + let path = build_binary_path(extension, p); + let contents = fs::read(path)?; + + SnapshotContents::Binary(SnapshotContentsBinary { + contents: Rc::new(contents), + extension: extension.clone(), + }) + } + }; let (snapshot_name, module_name) = names_of_path(p); @@ -344,7 +406,7 @@ impl Snapshot { module_name, Some(snapshot_name), metadata, - buf.into(), + contents, )) } @@ -377,12 +439,12 @@ 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( + snapshot = Some( value .as_str() .ok_or(content::Error::UnexpectedDataType)? - .to_string(), - )) + .into(), + ) } _ => {} } @@ -405,7 +467,10 @@ 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()))); + + if let SnapshotContents::String(SnapshotContentsString(ref content)) = self.snapshot { + fields.push(("snapshot", Content::from(content.as_str()))); + } Content::Struct("Content", fields) } @@ -442,15 +507,19 @@ impl Snapshot { } /// The snapshot contents as a &str - pub fn contents_str(&self) -> &str { + pub fn contents_str(&self) -> Option<&str> { self.snapshot.as_str() } 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('\n'); + + if let Some(contents_str) = self.contents_str() { + buf.push_str(contents_str); + buf.push('\n'); + } + buf } @@ -470,23 +539,31 @@ impl Snapshot { // 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) + // TODO: Do we need to compare the binary files here too? + if !self.contents().is_binary() { + 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); } - }; - if old == persisted.as_str() { - return Ok(false); } } fs::write(path, serialized_snapshot)?; + + if let SnapshotContents::Binary(ref contents) = self.snapshot { + fs::write(contents.build_path(path), &*contents.contents)?; + } + Ok(true) } @@ -512,18 +589,55 @@ impl Snapshot { Ok(None) } } + + pub fn cleanup_extra_files(snapshot: Option<&Self>, path: &Path) -> Result<(), Box> { + let binary_path = snapshot.and_then(|snapshot| match &snapshot.contents() { + SnapshotContents::String(_) => None, + SnapshotContents::Binary(contents) => Some(contents.build_path(path)), + }); + let binary_file_name = binary_path.as_ref().map(|path| path.file_name().unwrap()); + + let file_name = path.file_name().unwrap(); + + for entry in path.parent().unwrap().read_dir()? { + let entry = entry?; + let entry_file_name = entry.file_name(); + if entry_file_name + .as_encoded_bytes() + .starts_with(file_name.as_encoded_bytes()) + && entry_file_name != file_name + && binary_file_name.map_or(true, |x| x != entry_file_name) + { + std::fs::remove_file(entry.path())?; + } + } + + Ok(()) + } } /// The contents of a Snapshot -// Could be Cow, but I think limited savings #[derive(Debug, Clone)] -pub struct SnapshotContents(String); +pub enum SnapshotContents { + String(SnapshotContentsString), + Binary(SnapshotContentsBinary), +} -impl SnapshotContents { - pub fn from_inline(value: &str) -> SnapshotContents { - SnapshotContents(get_inline_snapshot_value(value)) - } +// Could be Cow, but I think limited savings +#[derive(Debug, Clone)] +pub struct SnapshotContentsString(pub String); + +#[derive(Debug, PartialEq, Eq, Clone)] +pub struct SnapshotContentsBinary { + // This is in an `Rc` because we need to be able to clone this struct cheaply and the contents + // of the `Vec` could be rather large. The reason it's not an `Rc<[u8]>` is because creating one + // of those would require re-allocating because of the additional size needed for the reference + // count. + pub contents: Rc>, + pub extension: String, +} +impl SnapshotContentsString { /// Returns the snapshot contents as a normalized string (for example, /// removing surrounding whitespace) pub fn as_str(&self) -> &str { @@ -542,18 +656,13 @@ impl SnapshotContents { self.0.as_str() } - /// Matches another snapshot without any normalization - pub fn matches_fully(&self, other: &SnapshotContents) -> bool { - self.as_str_exact() == other.as_str_exact() - } - /// Snapshot matches based on the latest format. - pub fn matches_latest(&self, other: &SnapshotContents) -> bool { + pub fn matches_latest(&self, other: &SnapshotContentsString) -> bool { self.as_str() == other.as_str() } - pub fn matches_legacy(&self, other: &SnapshotContents) -> bool { - fn as_str_legacy(sc: &SnapshotContents) -> &str { + pub fn matches_legacy(&self, other: &SnapshotContentsString) -> bool { + fn as_str_legacy(sc: &SnapshotContentsString) -> &str { let out = sc.as_str(); // Legacy inline snapshots have `---` at the start, so this strips that if // it exists. @@ -564,11 +673,50 @@ impl SnapshotContents { } as_str_legacy(self) == as_str_legacy(other) } +} + +impl SnapshotContentsBinary { + pub fn build_path(&self, path: impl Into) -> PathBuf { + build_binary_path(&self.extension, path) + } +} + +impl SnapshotContents { + pub fn is_binary(&self) -> bool { + matches!(self, SnapshotContents::Binary(_)) + } + + /// Returns the snapshot contents as a normalized string (for example, + /// removing surrounding whitespace) + pub fn as_str(&self) -> Option<&str> { + match self { + SnapshotContents::String(content) => Some(content.as_str()), + _ => None, + } + } + + pub fn from_inline(value: &str) -> SnapshotContents { + SnapshotContents::String(SnapshotContentsString(get_inline_snapshot_value(value))) + } + + /// Matches another snapshot without any normalization + pub fn matches_fully(&self, other: &SnapshotContents) -> bool { + match (self, other) { + (SnapshotContents::String(this), SnapshotContents::String(other)) => { + this.as_str_exact() == other.as_str_exact() + } + (SnapshotContents::Binary(this), SnapshotContents::Binary(other)) => this == other, + _ => false, + } + } /// Create the literal string to write inline, adding `#` to escape the /// value, indentation, delimiters - pub fn to_inline(&self, indentation: usize) -> String { - let contents = &self.0; + pub fn to_inline(&self, indentation: usize) -> Option { + let SnapshotContents::String(SnapshotContentsString(contents)) = &self else { + return None; + }; + let mut out = String::new(); // We don't technically need to escape on newlines, but it reduces diffs @@ -618,7 +766,7 @@ impl SnapshotContents { out.push('"'); out.push_str(&delimiter); - out + Some(out) } } @@ -634,37 +782,46 @@ impl<'a> From> for SnapshotContents { impl From<&str> for SnapshotContents { fn from(value: &str) -> SnapshotContents { // make sure we have unix newlines consistently - SnapshotContents(value.replace("\r\n", "\n")) + SnapshotContents::String(SnapshotContentsString(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 + SnapshotContents::String(SnapshotContentsString(value.replace("\r\n", "\n"))) } } impl PartialEq for SnapshotContents { fn eq(&self, other: &Self) -> bool { - // Ideally match on current rules, but otherwise fall back to legacy rules - 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()); - true - } else { - false + match (self, other) { + (SnapshotContents::String(this), SnapshotContents::String(other)) => { + // Ideally match on current rules, but otherwise fall back to legacy rules + if this.matches_latest(other) { + true + } else if this.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:", this.as_str()); + true + } else { + false + } + } + (SnapshotContents::Binary(this), SnapshotContents::Binary(other)) => this == other, + _ => false, } } } +fn build_binary_path(extension: &str, path: impl Into) -> PathBuf { + let path = path.into(); + let mut new_extension = path.extension().unwrap().to_os_string(); + new_extension.push("."); + new_extension.push(extension); + + path.with_extension(new_extension) +} + fn count_leading_spaces(value: &str) -> usize { value.chars().take_while(|x| x.is_whitespace()).count() } @@ -801,14 +958,16 @@ 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""#); + let snapshot_contents = SnapshotContents::String(SnapshotContentsString("testing".to_string())); + assert_eq!(snapshot_contents.to_inline(0).unwrap(), r#""testing""#); let t = &" a b"[1..]; assert_eq!( - SnapshotContents(t.to_string()).to_inline(0), + SnapshotContents::String(SnapshotContentsString(t.to_string())) + .to_inline(0) + .unwrap(), r##"r#" a b @@ -816,12 +975,13 @@ b ); assert_eq!( - SnapshotContents( + SnapshotContents::String(SnapshotContentsString( "a b" .to_string() - ) - .to_inline(4), + )) + .to_inline(4) + .unwrap(), r##"r#" a b @@ -832,7 +992,9 @@ b" a b"[1..]; assert_eq!( - SnapshotContents(t.to_string()).to_inline(0), + SnapshotContents::String(SnapshotContentsString(t.to_string())) + .to_inline(0) + .unwrap(), r##"r#" a b @@ -844,7 +1006,9 @@ a b"[1..]; assert_eq!( - SnapshotContents(t.to_string()).to_inline(4), + SnapshotContents::String(SnapshotContentsString(t.to_string())) + .to_inline(4) + .unwrap(), r##"r#" a @@ -856,24 +1020,38 @@ b"[1..]; ab "[1..]; assert_eq!( - SnapshotContents(t.to_string()).to_inline(0), + SnapshotContents::String(SnapshotContentsString(t.to_string())) + .to_inline(0) + .unwrap(), r##"r#" ab "#"## ); let t = "ab"; - assert_eq!(SnapshotContents(t.to_string()).to_inline(0), r#""ab""#); + assert_eq!( + SnapshotContents::String(SnapshotContentsString(t.to_string())) + .to_inline(0) + .unwrap(), + 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::String(SnapshotContentsString(t.to_string())) + .to_inline(0) + .unwrap(), + r#""a###b""# + ); let t = "a\n\\###b"; assert_eq!( - SnapshotContents(t.to_string()).to_inline(0), + SnapshotContents::String(SnapshotContentsString(t.to_string())) + .to_inline(0) + .unwrap(), r#####"r####" a \###b diff --git a/insta/tests/snapshots/test_binary__binary_snapshot.snap b/insta/tests/snapshots/test_binary__binary_snapshot.snap new file mode 100644 index 00000000..0d22a087 --- /dev/null +++ b/insta/tests/snapshots/test_binary__binary_snapshot.snap @@ -0,0 +1,6 @@ +--- +source: insta/tests/test_binary.rs +expression: "b\"test\".to_vec()" +extension: txt +snapshot_type: binary +--- diff --git a/insta/tests/snapshots/test_binary__binary_snapshot.snap.txt b/insta/tests/snapshots/test_binary__binary_snapshot.snap.txt new file mode 100644 index 00000000..30d74d25 --- /dev/null +++ b/insta/tests/snapshots/test_binary__binary_snapshot.snap.txt @@ -0,0 +1 @@ +test \ No newline at end of file diff --git a/insta/tests/test_binary.rs b/insta/tests/test_binary.rs new file mode 100644 index 00000000..e02c85b3 --- /dev/null +++ b/insta/tests/test_binary.rs @@ -0,0 +1,4 @@ +#[test] +fn test_binary_snapshot() { + insta::assert_binary_snapshot!("txt", b"test".to_vec()); +} From 4aaf10eef237e32cc48734eff660320b06850c9a Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Sun, 22 Sep 2024 20:36:54 +0200 Subject: [PATCH 02/41] Clean up merge related issues --- cargo-insta/src/container.rs | 19 +-- cargo-insta/src/inline.rs | 12 +- cargo-insta/src/walk.rs | 6 +- insta/src/lib.rs | 3 +- insta/src/output.rs | 22 +-- insta/src/runtime.rs | 23 +-- insta/src/snapshot.rs | 267 +++++++++++++++++------------------ 7 files changed, 177 insertions(+), 175 deletions(-) diff --git a/cargo-insta/src/container.rs b/cargo-insta/src/container.rs index 20626049..85c8e3a8 100644 --- a/cargo-insta/src/container.rs +++ b/cargo-insta/src/container.rs @@ -3,7 +3,7 @@ use std::fs; use std::path::{Path, PathBuf}; use insta::Snapshot; -pub(crate) use insta::SnapshotKind; +pub(crate) use insta::StringSnapshotKind; use insta::_cargo_insta_support::{ContentError, PendingInlineSnapshot}; use crate::inline::FilePatcher; @@ -49,7 +49,7 @@ impl PendingSnapshot { pub(crate) struct SnapshotContainer { snapshot_path: PathBuf, target_path: PathBuf, - kind: SnapshotKind, + kind: StringSnapshotKind, snapshots: Vec, patcher: Option, } @@ -58,11 +58,11 @@ impl SnapshotContainer { pub(crate) fn load( snapshot_path: PathBuf, target_path: PathBuf, - kind: SnapshotKind, + kind: StringSnapshotKind, ) -> Result> { let mut snapshots = Vec::new(); let patcher = match kind { - SnapshotKind::File => { + StringSnapshotKind::File => { let old = if fs::metadata(&target_path).is_err() { None } else { @@ -78,7 +78,7 @@ impl SnapshotContainer { }); None } - SnapshotKind::Inline => { + StringSnapshotKind::Inline => { let mut pending_vec = PendingInlineSnapshot::load_batch(&snapshot_path)?; let mut have_new = false; @@ -134,8 +134,8 @@ impl SnapshotContainer { pub(crate) fn snapshot_file(&self) -> Option<&Path> { match self.kind { - SnapshotKind::File => Some(&self.target_path), - SnapshotKind::Inline => None, + StringSnapshotKind::File => Some(&self.target_path), + StringSnapshotKind::Inline => None, } } @@ -182,7 +182,10 @@ impl SnapshotContainer { for (idx, snapshot) in self.snapshots.iter().enumerate() { match snapshot.op { Operation::Accept => { - patcher.set_new_content(idx, snapshot.new.contents()); + patcher.set_new_content( + idx, + snapshot.new.contents().as_string_contents().unwrap(), + ); did_accept = true; } Operation::Reject => {} diff --git a/cargo-insta/src/inline.rs b/cargo-insta/src/inline.rs index 778e4e2f..c8eced06 100644 --- a/cargo-insta/src/inline.rs +++ b/cargo-insta/src/inline.rs @@ -4,7 +4,7 @@ use std::fs; use std::io::Write; use std::path::{Path, PathBuf}; -use insta::_cargo_insta_support::SnapshotContents; +use insta::_cargo_insta_support::SnapshotContentsString; use proc_macro2::TokenTree; use syn::__private::ToTokens; @@ -90,7 +90,7 @@ impl FilePatcher { self.inline_snapshots[id].start.0 + 1 } - pub(crate) fn set_new_content(&mut self, id: usize, snapshot: &SnapshotContents) { + pub(crate) fn set_new_content(&mut self, id: usize, snapshot: &SnapshotContentsString) { let inline = &mut self.inline_snapshots[id]; // find prefix and suffix on the first and last lines @@ -104,12 +104,8 @@ impl FilePatcher { .collect(); // replace lines - let snapshot_line_contents = [ - prefix, - snapshot.to_inline(inline.indentation).unwrap(), - suffix, - ] - .join(""); + let snapshot_line_contents = + [prefix, snapshot.to_inline(inline.indentation), suffix].join(""); self.lines.splice( inline.start.0..=inline.end.0, diff --git a/cargo-insta/src/walk.rs b/cargo-insta/src/walk.rs index d94c651a..054bda37 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, SnapshotKind}; +use crate::container::{SnapshotContainer, StringSnapshotKind}; #[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, - SnapshotKind::File, + StringSnapshotKind::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, - SnapshotKind::Inline, + StringSnapshotKind::Inline, )) } else { None diff --git a/insta/src/lib.rs b/insta/src/lib.rs index 9415704c..f39cd1b8 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, SnapshotKind}; +pub use crate::snapshot::{MetaData, Snapshot, StringSnapshotKind}; /// Exposes some library internals. /// @@ -320,6 +320,7 @@ pub mod _cargo_insta_support { output::SnapshotPrinter, snapshot::PendingInlineSnapshot, snapshot::SnapshotContents, + snapshot::SnapshotContentsString, utils::is_ci, }; } diff --git a/insta/src/output.rs b/insta/src/output.rs index 7bd864e2..119d4b02 100644 --- a/insta/src/output.rs +++ b/insta/src/output.rs @@ -4,7 +4,7 @@ use std::{path::Path, time::Duration}; use similar::{Algorithm, ChangeTag, TextDiff}; use crate::content::yaml; -use crate::snapshot::{MetaData, Snapshot, SnapshotContents, SnapshotContentsString}; +use crate::snapshot::{MetaData, Snapshot, SnapshotContents}; use crate::utils::{format_rust_expression, style, term_width}; /// Snapshot printer utility. @@ -103,16 +103,16 @@ impl<'a> SnapshotPrinter<'a> { fn print_snapshot(&self) { print_line(term_width()); - let new_contents = self.new_snapshot.contents(); - let width = term_width(); if self.show_info { self.print_info(); } println!("Snapshot Contents:"); - match new_contents { - SnapshotContents::String(SnapshotContentsString(new_contents)) => { + match self.new_snapshot.contents() { + SnapshotContents::String(new_contents) => { + let new_contents = new_contents.to_string(); + println!("──────┬{:─^1$}", "", width.saturating_sub(7)); for (idx, line) in new_contents.lines().enumerate() { println!("{:>5} │ {}", style(idx + 1).cyan().dim().bold(), line); @@ -173,19 +173,19 @@ impl<'a> SnapshotPrinter<'a> { self.old_snapshot.as_ref().map(|o| o.contents()), self.new_snapshot.contents(), ) { - (Some(SnapshotContents::Binary { .. }) | None, SnapshotContents::String(new)) => { - Some((None, Some(&new.0))) + (Some(SnapshotContents::Binary(_)) | None, SnapshotContents::String(new)) => { + Some((None, Some(new.to_string()))) } (Some(SnapshotContents::String(old)), SnapshotContents::Binary { .. }) => { - Some((Some(&old.0), None)) + Some((Some(old.to_string()), None)) } (Some(SnapshotContents::String(old)), SnapshotContents::String(new)) => { - Some((Some(&old.0), Some(&new.0))) + Some((Some(old.to_string()), Some(new.to_string()))) } _ => None, } { - let old_text = dbg!(old).map(String::as_str).unwrap_or(""); - let new_text = dbg!(new).map(String::as_str).unwrap_or(""); + let old_text = old.as_deref().unwrap_or(""); + let new_text = new.as_deref().unwrap_or(""); let newlines_matter = newlines_matter(old_text, new_text); let diff = TextDiff::configure() diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 954b9d8f..7e95a769 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -13,7 +13,7 @@ use crate::output::SnapshotPrinter; use crate::settings::Settings; use crate::snapshot::{ MetaData, PendingInlineSnapshot, Snapshot, SnapshotContents, SnapshotContentsBinary, - SnapshotType, + SnapshotContentsString, SnapshotType, }; use crate::utils::{path_to_storage, style}; use crate::{ @@ -21,7 +21,7 @@ use crate::{ get_cargo_workspace, get_tool_config, memoize_snapshot_file, snapshot_update_behavior, OutputBehavior, SnapshotUpdateBehavior, ToolConfig, }, - snapshot::SnapshotKind, + snapshot::StringSnapshotKind, }; lazy_static::lazy_static! { @@ -343,7 +343,8 @@ impl<'a> SnapshotAssertionContext<'a> { module_path.replace("::", "__"), None, MetaData::default(), - SnapshotContents::new(contents.to_string(), SnapshotKind::Inline), + SnapshotContentsString::new(contents.to_string(), StringSnapshotKind::Inline) + .into(), )); } }; @@ -706,7 +707,12 @@ pub fn assert_snapshot( #[cfg(feature = "filters")] let content = Settings::with(|settings| settings.filters().apply_to(content)); - content.into() + let kind = match ctx.snapshot_file { + Some(_) => StringSnapshotKind::File, + None => StringSnapshotKind::Inline, + }; + + SnapshotContentsString::new(content.into(), kind).into() } SnapshotValue::Binary { content, extension, .. @@ -717,17 +723,14 @@ pub fn assert_snapshot( "file extensions starting with 'new.' are not allowed", ); - SnapshotContents::Binary(SnapshotContentsBinary { + SnapshotContentsBinary { contents: Rc::new(content), extension: extension.to_string(), - }) + } + .into() } }; - let kind = match ctx.snapshot_file { - Some(_) => SnapshotKind::File, - None => SnapshotKind::Inline, - }; let new_snapshot = ctx.new_snapshot(content, expr); // memoize the snapshot file if requested. diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index 3100484a..f1dec1eb 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -95,10 +95,10 @@ impl PendingInlineSnapshot { 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, SnapshotKind::Inline)?) + old = Some(Snapshot::from_content(value, StringSnapshotKind::Inline)?) } Some("new") if !value.is_nil() => { - new = Some(Snapshot::from_content(value, SnapshotKind::Inline)?) + new = Some(Snapshot::from_content(value, StringSnapshotKind::Inline)?) } _ => {} } @@ -324,8 +324,8 @@ impl MetaData { } } -#[derive(Debug, Clone, Copy)] -pub enum SnapshotKind { +#[derive(Debug, PartialEq, Eq, Clone, Copy)] +pub enum StringSnapshotKind { Inline, File, } @@ -400,16 +400,21 @@ impl Snapshot { buf.push_str(&line); } - SnapshotContents::String(SnapshotContentsString(buf)) + SnapshotContentsString { + contents: buf, + kind: StringSnapshotKind::File, + } + .into() } SnapshotType::Binary { ref extension } => { let path = build_binary_path(extension, p); let contents = fs::read(path)?; - SnapshotContents::Binary(SnapshotContentsBinary { + SnapshotContentsBinary { contents: Rc::new(contents), extension: extension.clone(), - }) + } + .into() } }; @@ -439,7 +444,10 @@ impl Snapshot { } #[cfg(feature = "_cargo_insta_internal")] - fn from_content(content: Content, kind: SnapshotKind) -> Result> { + fn from_content( + content: Content, + kind: StringSnapshotKind, + ) -> Result> { if let Content::Map(map) = content { let mut module_name = None; let mut snapshot_name = None; @@ -452,13 +460,16 @@ 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 { - contents: value - .as_str() - .ok_or(content::Error::UnexpectedDataType)? - .to_string(), - kind, - }); + snapshot = Some( + SnapshotContentsString { + contents: value + .as_str() + .ok_or(content::Error::UnexpectedDataType)? + .to_string(), + kind, + } + .into(), + ); } _ => {} } @@ -482,8 +493,8 @@ impl Snapshot { } fields.push(("metadata", self.metadata.as_content())); - if let SnapshotContents::String(SnapshotContentsString(ref content)) = self.snapshot { - fields.push(("snapshot", Content::from(content.as_str()))); + if let SnapshotContents::String(ref content) = self.snapshot { + fields.push(("snapshot", Content::from(content.to_string()))); } Content::Struct("Content", fields) @@ -514,34 +525,44 @@ impl Snapshot { self.contents() == other.contents() } - pub fn kind(&self) -> SnapshotKind { - self.snapshot.kind - } + // pub fn kind(&self) -> StringSnapshotKind { + // 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 { - let contents_match_exact = self.contents().matches_fully(other.contents()); - match self.kind() { - SnapshotKind::File => { - self.metadata.trim_for_persistence() == other.metadata.trim_for_persistence() - && contents_match_exact + match (self.contents(), other.contents()) { + (SnapshotContents::String(self_contents), SnapshotContents::String(other_contents)) => { + let contents_match_exact = self_contents == other_contents; + match self_contents.kind { + StringSnapshotKind::File => { + self.metadata.trim_for_persistence() + == other.metadata.trim_for_persistence() + && contents_match_exact + } + StringSnapshotKind::Inline => contents_match_exact, + } } - SnapshotKind::Inline => contents_match_exact, + (SnapshotContents::Binary(a), SnapshotContents::Binary(b)) => a == b, + _ => false, } } /// The normalized snapshot contents as a String - pub fn contents_string(&self) -> String { - self.snapshot.normalize() + pub fn contents_string(&self) -> Option { + match self.contents() { + SnapshotContents::String(contents) => Some(contents.normalize()), + SnapshotContents::Binary(_) => None, + } } fn serialize_snapshot(&self, md: &MetaData) -> String { let mut buf = yaml::to_string(&md.as_content()); buf.push_str("---\n"); - if let Some(contents_str) = self.contents_str() { + if let Some(ref contents_str) = self.contents_string() { buf.push_str(contents_str); buf.push('\n'); } @@ -617,8 +638,11 @@ pub enum SnapshotContents { } // Could be Cow, but I think limited savings -#[derive(Debug, Clone)] -pub struct SnapshotContentsString(pub String); +#[derive(Debug, PartialEq, Eq, Clone)] +pub struct SnapshotContentsString { + contents: String, + pub kind: StringSnapshotKind, +} #[derive(Debug, PartialEq, Eq, Clone)] pub struct SnapshotContentsBinary { @@ -630,32 +654,44 @@ pub struct SnapshotContentsBinary { pub extension: String, } +impl From for SnapshotContents { + fn from(value: SnapshotContentsString) -> Self { + SnapshotContents::String(value) + } +} + +impl From for SnapshotContents { + fn from(value: SnapshotContentsBinary) -> Self { + SnapshotContents::Binary(value) + } +} + impl SnapshotContentsString { - pub fn new(contents: String, kind: SnapshotKind) -> SnapshotContents { + pub fn new(contents: String, kind: StringSnapshotKind) -> SnapshotContentsString { // 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 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, - } + SnapshotContentsString { contents, kind } } - /// Returns the snapshot contents as string without any normalization - pub fn as_str_exact(&self) -> &str { - self.contents.as_str() - } + // /// 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, + // } + // } + + // /// Returns the snapshot contents as string without any normalization + // pub fn as_str_exact(&self) -> &str { + // self.contents.as_str() + // } /// Snapshot matches based on the latest format. pub fn matches_latest(&self, other: &SnapshotContentsString) -> bool { @@ -672,56 +708,28 @@ impl SnapshotContentsString { None => out, }; match sc.kind { - SnapshotKind::Inline => legacy_inline_normalize(&out), - SnapshotKind::File => out, + StringSnapshotKind::Inline => legacy_inline_normalize(&out), + StringSnapshotKind::File => out, } } as_str_legacy(self) == as_str_legacy(other) } -} - -impl SnapshotContentsBinary { - pub fn build_path(&self, path: impl Into) -> PathBuf { - build_binary_path(&self.extension, path) - } -} - -impl SnapshotContents { - pub fn is_binary(&self) -> bool { - matches!(self, SnapshotContents::Binary(_)) - } - - /// Returns the snapshot contents as a normalized string (for example, - /// removing surrounding whitespace) - pub fn as_str(&self) -> Option<&str> { - match self { - SnapshotContents::String(content) => Some(content.as_str()), - _ => None, - } - } - pub fn from_inline(value: &str) -> SnapshotContents { - SnapshotContents::String(SnapshotContentsString(get_inline_snapshot_value(value))) - } - - /// Matches another snapshot without any normalization - pub fn matches_fully(&self, other: &SnapshotContents) -> bool { - match (self, other) { - (SnapshotContents::String(this), SnapshotContents::String(other)) => { - this.as_str_exact() == other.as_str_exact() - } - (SnapshotContents::Binary(this), SnapshotContents::Binary(other)) => this == other, - _ => false, - } + fn normalize(&self) -> String { + let kind_specific_normalization = match self.kind { + StringSnapshotKind::Inline => normalize_inline_snapshot(&self.contents), + StringSnapshotKind::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") } /// Returns the string literal, including `#` delimiters, to insert into a /// Rust source file. - pub fn to_inline(&self, indentation: usize) -> Option { - let SnapshotContents::String(SnapshotContentsString(contents)) = &self else { - return None; - }; - + pub fn to_inline(&self, indentation: usize) -> String { let contents = self.normalize(); let mut out = String::new(); @@ -765,24 +773,30 @@ impl SnapshotContents { out.push('"'); out.push_str(&delimiter); + out + } +} - Some(out) +impl SnapshotContentsBinary { + pub fn build_path(&self, path: impl Into) -> PathBuf { + build_binary_path(&self.extension, path) } +} - fn normalize(&self) -> String { - let kind_specific_normalization = match self.kind { - SnapshotKind::Inline => normalize_inline_snapshot(&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 SnapshotContents { + pub fn is_binary(&self) -> bool { + matches!(self, SnapshotContents::Binary(_)) + } + + pub fn as_string_contents(&self) -> Option<&SnapshotContentsString> { + match self { + SnapshotContents::String(contents) => Some(contents), + SnapshotContents::Binary(_) => None, + } } } -impl fmt::Display for SnapshotContents { +impl fmt::Display for SnapshotContentsString { /// Returns the snapshot contents as a normalized string (for example, /// removing surrounding whitespace) fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -969,16 +983,15 @@ fn legacy_inline_normalize(frozen_value: &str) -> String { #[test] fn test_snapshot_contents() { use similar_asserts::assert_eq; - let snapshot_contents = SnapshotContents::String(SnapshotContentsString("testing".to_string())); - assert_eq!(snapshot_contents.to_inline(0).unwrap(), r#""testing""#); + let snapshot_contents = + SnapshotContentsString::new("testing".to_string(), StringSnapshotKind::Inline); + assert_eq!(snapshot_contents.to_inline(0), r#""testing""#); let t = &" a b"[1..]; assert_eq!( - SnapshotContents::String(SnapshotContentsString(t.to_string())) - .to_inline(0) - .unwrap(), + SnapshotContentsString::new(t.to_string(), StringSnapshotKind::Inline).to_inline(0), r##"r" a b @@ -986,9 +999,7 @@ b ); assert_eq!( - SnapshotContents::String(SnapshotContentsString("a\nb".to_string())) - .to_inline(4) - .unwrap(), + SnapshotContentsString::new("a\nb".to_string(), StringSnapshotKind::Inline).to_inline(4), r##"r" a b @@ -996,11 +1007,8 @@ b ); assert_eq!( - SnapshotContents::String(SnapshotContentsString("\n a\n b".to_string())) - .to_inline(0) - .unwrap(), - r##"r" - SnapshotContents::new("\n a\n b".to_string(), SnapshotKind::Inline).to_inline(0), + SnapshotContentsString::new("\n a\n b".to_string(), StringSnapshotKind::Inline) + .to_inline(0), r##"r" a b @@ -1008,9 +1016,8 @@ b ); assert_eq!( - SnapshotContents::String(SnapshotContentsString("\na\n\nb".to_string())) - .to_inline(4) - .unwrap(), + SnapshotContentsString::new("\na\n\nb".to_string(), StringSnapshotKind::Inline) + .to_inline(4), r##"r" a @@ -1019,18 +1026,13 @@ b ); assert_eq!( - SnapshotContents::String(SnapshotContentsString("\n ab\n".to_string())) - .to_inline(0) - .unwrap(), - r##"r#" - ab -"#"## + SnapshotContentsString::new("\n ab\n".to_string(), StringSnapshotKind::Inline) + .to_inline(0), + r##""ab""## ); assert_eq!( - SnapshotContents::String(SnapshotContentsString("ab".to_string())) - .to_inline(0) - .unwrap(), + SnapshotContentsString::new("ab".to_string(), StringSnapshotKind::Inline).to_inline(0), r#""ab""# ); } @@ -1038,16 +1040,13 @@ b #[test] fn test_snapshot_contents_hashes() { assert_eq!( - SnapshotContents::String(SnapshotContentsString("a###b".to_string())) - .to_inline(0) - .unwrap(), + SnapshotContentsString::new("a###b".to_string(), StringSnapshotKind::Inline).to_inline(0), r#""a###b""# ); assert_eq!( - SnapshotContents::String(SnapshotContentsString("a\n\\###b".to_string())) - .to_inline(0) - .unwrap(), + SnapshotContentsString::new("a\n\\###b".to_string(), StringSnapshotKind::Inline) + .to_inline(0), r#####"r" a \###b From 49891616f0ebf1b3a06a16ee7a15bbba25a05cc1 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Sun, 22 Sep 2024 23:14:11 +0200 Subject: [PATCH 03/41] Fix error when trying to clean up files in non-existent directory This fixes the failing tests. --- insta/src/snapshot.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index f1dec1eb..5096f137 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -1,7 +1,7 @@ use std::env; use std::error::Error; use std::fs; -use std::io::{BufRead, BufReader, Write}; +use std::io::{BufRead, BufReader, ErrorKind, Write}; use std::path::{Path, PathBuf}; use std::rc::Rc; use std::time::{SystemTime, UNIX_EPOCH}; @@ -613,7 +613,16 @@ impl Snapshot { let file_name = path.file_name().unwrap(); - for entry in path.parent().unwrap().read_dir()? { + let read_dir = path.parent().unwrap().read_dir(); + + if read_dir + .as_ref() + .is_err_and(|e| e.kind() == ErrorKind::NotFound) + { + return Ok(()); + } + + for entry in read_dir? { let entry = entry?; let entry_file_name = entry.file_name(); if entry_file_name From a970400209efbac81770b265cee9156c593e3712 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Mon, 23 Sep 2024 00:15:56 +0200 Subject: [PATCH 04/41] Rename some types for clarity --- cargo-insta/src/container.rs | 14 +-- cargo-insta/src/inline.rs | 4 +- cargo-insta/src/walk.rs | 6 +- cargo-insta/tests/main.rs | 4 +- insta/src/lib.rs | 4 +- insta/src/output.rs | 8 +- insta/src/runtime.rs | 40 ++++----- insta/src/snapshot.rs | 162 +++++++++++++++-------------------- 8 files changed, 107 insertions(+), 135 deletions(-) diff --git a/cargo-insta/src/container.rs b/cargo-insta/src/container.rs index 85c8e3a8..10b13cd3 100644 --- a/cargo-insta/src/container.rs +++ b/cargo-insta/src/container.rs @@ -3,7 +3,7 @@ use std::fs; use std::path::{Path, PathBuf}; use insta::Snapshot; -pub(crate) use insta::StringSnapshotKind; +pub(crate) use insta::TextSnapshotKind; use insta::_cargo_insta_support::{ContentError, PendingInlineSnapshot}; use crate::inline::FilePatcher; @@ -49,7 +49,7 @@ impl PendingSnapshot { pub(crate) struct SnapshotContainer { snapshot_path: PathBuf, target_path: PathBuf, - kind: StringSnapshotKind, + kind: TextSnapshotKind, snapshots: Vec, patcher: Option, } @@ -58,11 +58,11 @@ impl SnapshotContainer { pub(crate) fn load( snapshot_path: PathBuf, target_path: PathBuf, - kind: StringSnapshotKind, + kind: TextSnapshotKind, ) -> Result> { let mut snapshots = Vec::new(); let patcher = match kind { - StringSnapshotKind::File => { + TextSnapshotKind::File => { let old = if fs::metadata(&target_path).is_err() { None } else { @@ -78,7 +78,7 @@ impl SnapshotContainer { }); None } - StringSnapshotKind::Inline => { + TextSnapshotKind::Inline => { let mut pending_vec = PendingInlineSnapshot::load_batch(&snapshot_path)?; let mut have_new = false; @@ -134,8 +134,8 @@ impl SnapshotContainer { pub(crate) fn snapshot_file(&self) -> Option<&Path> { match self.kind { - StringSnapshotKind::File => Some(&self.target_path), - StringSnapshotKind::Inline => None, + TextSnapshotKind::File => Some(&self.target_path), + TextSnapshotKind::Inline => None, } } diff --git a/cargo-insta/src/inline.rs b/cargo-insta/src/inline.rs index c8eced06..82fb0502 100644 --- a/cargo-insta/src/inline.rs +++ b/cargo-insta/src/inline.rs @@ -4,7 +4,7 @@ use std::fs; use std::io::Write; use std::path::{Path, PathBuf}; -use insta::_cargo_insta_support::SnapshotContentsString; +use insta::_cargo_insta_support::TextSnapshotContents; use proc_macro2::TokenTree; use syn::__private::ToTokens; @@ -90,7 +90,7 @@ impl FilePatcher { self.inline_snapshots[id].start.0 + 1 } - pub(crate) fn set_new_content(&mut self, id: usize, snapshot: &SnapshotContentsString) { + pub(crate) fn set_new_content(&mut self, id: usize, snapshot: &TextSnapshotContents) { let inline = &mut self.inline_snapshots[id]; // find prefix and suffix on the first and last lines diff --git a/cargo-insta/src/walk.rs b/cargo-insta/src/walk.rs index 054bda37..1057812c 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, StringSnapshotKind}; +use crate::container::{SnapshotContainer, TextSnapshotKind}; #[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, - StringSnapshotKind::File, + TextSnapshotKind::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, - StringSnapshotKind::Inline, + TextSnapshotKind::Inline, )) } else { None diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index 821b0042..33115fb7 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -882,7 +882,7 @@ Hello, world! source: src/lib.rs -expression: +expression: "\"Hello, world!\"" - +snapshot_type: string + +snapshot_type: text --- Hello, world! - @@ -898,7 +898,7 @@ Hello, world! source: src/lib.rs -expression: +expression: "\"Hello, world!\"" - +snapshot_type: string + +snapshot_type: text --- Hello, world! - diff --git a/insta/src/lib.rs b/insta/src/lib.rs index f39cd1b8..c32dd513 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, StringSnapshotKind}; +pub use crate::snapshot::{MetaData, Snapshot, TextSnapshotKind}; /// Exposes some library internals. /// @@ -320,7 +320,7 @@ pub mod _cargo_insta_support { output::SnapshotPrinter, snapshot::PendingInlineSnapshot, snapshot::SnapshotContents, - snapshot::SnapshotContentsString, + snapshot::TextSnapshotContents, utils::is_ci, }; } diff --git a/insta/src/output.rs b/insta/src/output.rs index 119d4b02..7045de54 100644 --- a/insta/src/output.rs +++ b/insta/src/output.rs @@ -110,7 +110,7 @@ impl<'a> SnapshotPrinter<'a> { println!("Snapshot Contents:"); match self.new_snapshot.contents() { - SnapshotContents::String(new_contents) => { + SnapshotContents::Text(new_contents) => { let new_contents = new_contents.to_string(); println!("──────┬{:─^1$}", "", width.saturating_sub(7)); @@ -173,13 +173,13 @@ impl<'a> SnapshotPrinter<'a> { self.old_snapshot.as_ref().map(|o| o.contents()), self.new_snapshot.contents(), ) { - (Some(SnapshotContents::Binary(_)) | None, SnapshotContents::String(new)) => { + (Some(SnapshotContents::Binary(_)) | None, SnapshotContents::Text(new)) => { Some((None, Some(new.to_string()))) } - (Some(SnapshotContents::String(old)), SnapshotContents::Binary { .. }) => { + (Some(SnapshotContents::Text(old)), SnapshotContents::Binary { .. }) => { Some((Some(old.to_string()), None)) } - (Some(SnapshotContents::String(old)), SnapshotContents::String(new)) => { + (Some(SnapshotContents::Text(old)), SnapshotContents::Text(new)) => { Some((Some(old.to_string()), Some(new.to_string()))) } _ => None, diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 7e95a769..b7b7c508 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -12,8 +12,8 @@ use std::{borrow::Cow, env}; use crate::output::SnapshotPrinter; use crate::settings::Settings; use crate::snapshot::{ - MetaData, PendingInlineSnapshot, Snapshot, SnapshotContents, SnapshotContentsBinary, - SnapshotContentsString, SnapshotType, + BinarySnapshotContents, MetaData, PendingInlineSnapshot, Snapshot, SnapshotContents, + SnapshotType, TextSnapshotContents, }; use crate::utils::{path_to_storage, style}; use crate::{ @@ -21,7 +21,7 @@ use crate::{ get_cargo_workspace, get_tool_config, memoize_snapshot_file, snapshot_update_behavior, OutputBehavior, SnapshotUpdateBehavior, ToolConfig, }, - snapshot::StringSnapshotKind, + snapshot::TextSnapshotKind, }; lazy_static::lazy_static! { @@ -66,12 +66,12 @@ impl From for SnapshotName<'static> { type SnapshotName<'a> = Option>; pub enum SnapshotValue<'a> { - String { + Text { name: SnapshotName<'a>, content: &'a str, }, - InlineString { + InlineText { reference_content: &'a str, content: &'a str, }, @@ -85,7 +85,7 @@ pub enum SnapshotValue<'a> { impl<'a> From<(AutoName, &'a str)> for SnapshotValue<'a> { fn from((_, content): (AutoName, &'a str)) -> Self { - SnapshotValue::String { + SnapshotValue::Text { name: None, content, } @@ -94,7 +94,7 @@ impl<'a> From<(AutoName, &'a str)> for SnapshotValue<'a> { impl<'a> From<(Option, &'a str)> for SnapshotValue<'a> { fn from((name, content): (Option, &'a str)) -> Self { - SnapshotValue::String { + SnapshotValue::Text { name: name.map(Cow::Owned), content, } @@ -103,7 +103,7 @@ impl<'a> From<(Option, &'a str)> for SnapshotValue<'a> { impl<'a> From<(String, &'a str)> for SnapshotValue<'a> { fn from((name, content): (String, &'a str)) -> Self { - SnapshotValue::String { + SnapshotValue::Text { name: Some(Cow::Owned(name)), content, } @@ -112,7 +112,7 @@ impl<'a> From<(String, &'a str)> for SnapshotValue<'a> { impl<'a> From<(Option<&'a str>, &'a str)> for SnapshotValue<'a> { fn from((name, content): (Option<&'a str>, &'a str)) -> Self { - SnapshotValue::String { + SnapshotValue::Text { name: name.map(Cow::Borrowed), content, } @@ -121,7 +121,7 @@ impl<'a> From<(Option<&'a str>, &'a str)> for SnapshotValue<'a> { impl<'a> From<(&'a str, &'a str)> for SnapshotValue<'a> { fn from((name, content): (&'a str, &'a str)) -> Self { - SnapshotValue::String { + SnapshotValue::Text { name: Some(Cow::Borrowed(name)), content, } @@ -130,7 +130,7 @@ impl<'a> From<(&'a str, &'a str)> for SnapshotValue<'a> { impl<'a> From<(InlineValue<'a>, &'a str)> for SnapshotValue<'a> { fn from((InlineValue(reference_content), content): (InlineValue<'a>, &'a str)) -> Self { - SnapshotValue::InlineString { + SnapshotValue::InlineText { reference_content, content, } @@ -285,7 +285,7 @@ impl<'a> SnapshotAssertionContext<'a> { let is_doctest = is_doctest(function_name); match new_snapshot_value { - SnapshotValue::String { name, .. } | SnapshotValue::Binary { name, .. } => { + SnapshotValue::Text { name, .. } | SnapshotValue::Binary { name, .. } => { let name = match name { Some(name) => add_suffix_to_snapshot_name(name.clone()), None => { @@ -314,7 +314,7 @@ impl<'a> SnapshotAssertionContext<'a> { snapshot_name = Some(name); snapshot_file = Some(file); } - SnapshotValue::InlineString { + SnapshotValue::InlineText { reference_content: contents, .. } => { @@ -343,7 +343,7 @@ impl<'a> SnapshotAssertionContext<'a> { module_path.replace("::", "__"), None, MetaData::default(), - SnapshotContentsString::new(contents.to_string(), StringSnapshotKind::Inline) + TextSnapshotContents::new(contents.to_string(), TextSnapshotKind::Inline) .into(), )); } @@ -391,7 +391,7 @@ impl<'a> SnapshotAssertionContext<'a> { .and_then(|x| self.localize_path(x)) .map(|x| path_to_storage(&x)), snapshot_type: match contents { - SnapshotContents::String(_) => SnapshotType::String, + SnapshotContents::Text(_) => SnapshotType::Text, SnapshotContents::Binary(ref contents) => SnapshotType::Binary { extension: contents.extension.clone(), }, @@ -702,17 +702,17 @@ pub fn assert_snapshot( } let content = match new_snapshot_value { - SnapshotValue::String { content, .. } | SnapshotValue::InlineString { content, .. } => { + SnapshotValue::Text { content, .. } | SnapshotValue::InlineText { content, .. } => { // apply filters if they are available #[cfg(feature = "filters")] let content = Settings::with(|settings| settings.filters().apply_to(content)); let kind = match ctx.snapshot_file { - Some(_) => StringSnapshotKind::File, - None => StringSnapshotKind::Inline, + Some(_) => TextSnapshotKind::File, + None => TextSnapshotKind::Inline, }; - SnapshotContentsString::new(content.into(), kind).into() + TextSnapshotContents::new(content.into(), kind).into() } SnapshotValue::Binary { content, extension, .. @@ -723,7 +723,7 @@ pub fn assert_snapshot( "file extensions starting with 'new.' are not allowed", ); - SnapshotContentsBinary { + BinarySnapshotContents { contents: Rc::new(content), extension: extension.to_string(), } diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index 5096f137..f9a5bfd0 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -95,10 +95,10 @@ impl PendingInlineSnapshot { 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, StringSnapshotKind::Inline)?) + old = Some(Snapshot::from_content(value, TextSnapshotKind::Inline)?) } Some("new") if !value.is_nil() => { - new = Some(Snapshot::from_content(value, StringSnapshotKind::Inline)?) + new = Some(Snapshot::from_content(value, TextSnapshotKind::Inline)?) } _ => {} } @@ -141,13 +141,13 @@ impl PendingInlineSnapshot { #[derive(Debug, Clone, PartialEq)] pub enum SnapshotType { - String, + Text, Binary { extension: String }, } impl Default for SnapshotType { fn default() -> Self { - SnapshotType::String + SnapshotType::Text } } @@ -222,11 +222,11 @@ impl MetaData { let mut expression = None; let mut info = None; let mut input_file = None; - let mut snapshot_type = TmpSnapshotType::String; + let mut snapshot_type = TmpSnapshotType::Text; let mut extension = None; enum TmpSnapshotType { - String, + Text, Binary, } @@ -241,7 +241,7 @@ impl MetaData { Some("snapshot_type") => { snapshot_type = match value.as_str() { Some("binary") => TmpSnapshotType::Binary, - _ => TmpSnapshotType::String, + _ => TmpSnapshotType::Text, } } Some("extension") => { @@ -259,7 +259,7 @@ impl MetaData { info, input_file, snapshot_type: match snapshot_type { - TmpSnapshotType::String => SnapshotType::String, + TmpSnapshotType::Text => SnapshotType::Text, TmpSnapshotType::Binary => SnapshotType::Binary { extension: extension.ok_or(content::Error::MissingField)?, }, @@ -292,7 +292,7 @@ impl MetaData { } let snapshot_type = Content::from(match self.snapshot_type { - SnapshotType::String => "string", + SnapshotType::Text => "text", SnapshotType::Binary { ref extension } => { fields.push(("extension", Content::from(extension.clone()))); "binary" @@ -325,7 +325,7 @@ impl MetaData { } #[derive(Debug, PartialEq, Eq, Clone, Copy)] -pub enum StringSnapshotKind { +pub enum TextSnapshotKind { Inline, File, } @@ -390,7 +390,7 @@ impl Snapshot { }; let contents = match metadata.snapshot_type { - SnapshotType::String => { + SnapshotType::Text => { buf.clear(); for (idx, line) in f.lines().enumerate() { let line = line?; @@ -400,9 +400,9 @@ impl Snapshot { buf.push_str(&line); } - SnapshotContentsString { + TextSnapshotContents { contents: buf, - kind: StringSnapshotKind::File, + kind: TextSnapshotKind::File, } .into() } @@ -410,7 +410,7 @@ impl Snapshot { let path = build_binary_path(extension, p); let contents = fs::read(path)?; - SnapshotContentsBinary { + BinarySnapshotContents { contents: Rc::new(contents), extension: extension.clone(), } @@ -444,10 +444,7 @@ impl Snapshot { } #[cfg(feature = "_cargo_insta_internal")] - fn from_content( - content: Content, - kind: StringSnapshotKind, - ) -> Result> { + fn from_content(content: Content, kind: TextSnapshotKind) -> Result> { if let Content::Map(map) = content { let mut module_name = None; let mut snapshot_name = None; @@ -461,7 +458,7 @@ impl Snapshot { Some("metadata") => metadata = Some(MetaData::from_content(value)?), Some("snapshot") => { snapshot = Some( - SnapshotContentsString { + TextSnapshotContents { contents: value .as_str() .ok_or(content::Error::UnexpectedDataType)? @@ -493,7 +490,7 @@ impl Snapshot { } fields.push(("metadata", self.metadata.as_content())); - if let SnapshotContents::String(ref content) = self.snapshot { + if let SnapshotContents::Text(ref content) = self.snapshot { fields.push(("snapshot", Content::from(content.to_string()))); } @@ -525,24 +522,20 @@ impl Snapshot { self.contents() == other.contents() } - // pub fn kind(&self) -> StringSnapshotKind { - // 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 { match (self.contents(), other.contents()) { - (SnapshotContents::String(self_contents), SnapshotContents::String(other_contents)) => { + (SnapshotContents::Text(self_contents), SnapshotContents::Text(other_contents)) => { let contents_match_exact = self_contents == other_contents; match self_contents.kind { - StringSnapshotKind::File => { + TextSnapshotKind::File => { self.metadata.trim_for_persistence() == other.metadata.trim_for_persistence() && contents_match_exact } - StringSnapshotKind::Inline => contents_match_exact, + TextSnapshotKind::Inline => contents_match_exact, } } (SnapshotContents::Binary(a), SnapshotContents::Binary(b)) => a == b, @@ -553,7 +546,7 @@ impl Snapshot { /// The normalized snapshot contents as a String pub fn contents_string(&self) -> Option { match self.contents() { - SnapshotContents::String(contents) => Some(contents.normalize()), + SnapshotContents::Text(contents) => Some(contents.normalize()), SnapshotContents::Binary(_) => None, } } @@ -606,7 +599,7 @@ impl Snapshot { pub fn cleanup_extra_files(snapshot: Option<&Self>, path: &Path) -> Result<(), Box> { let binary_path = snapshot.and_then(|snapshot| match &snapshot.contents() { - SnapshotContents::String(_) => None, + SnapshotContents::Text(_) => None, SnapshotContents::Binary(contents) => Some(contents.build_path(path)), }); let binary_file_name = binary_path.as_ref().map(|path| path.file_name().unwrap()); @@ -642,19 +635,19 @@ impl Snapshot { /// The contents of a Snapshot #[derive(Debug, Clone)] pub enum SnapshotContents { - String(SnapshotContentsString), - Binary(SnapshotContentsBinary), + Text(TextSnapshotContents), + Binary(BinarySnapshotContents), } // Could be Cow, but I think limited savings #[derive(Debug, PartialEq, Eq, Clone)] -pub struct SnapshotContentsString { +pub struct TextSnapshotContents { contents: String, - pub kind: StringSnapshotKind, + pub kind: TextSnapshotKind, } #[derive(Debug, PartialEq, Eq, Clone)] -pub struct SnapshotContentsBinary { +pub struct BinarySnapshotContents { // This is in an `Rc` because we need to be able to clone this struct cheaply and the contents // of the `Vec` could be rather large. The reason it's not an `Rc<[u8]>` is because creating one // of those would require re-allocating because of the additional size needed for the reference @@ -663,52 +656,47 @@ pub struct SnapshotContentsBinary { pub extension: String, } -impl From for SnapshotContents { - fn from(value: SnapshotContentsString) -> Self { - SnapshotContents::String(value) +impl From for SnapshotContents { + fn from(value: TextSnapshotContents) -> Self { + SnapshotContents::Text(value) } } -impl From for SnapshotContents { - fn from(value: SnapshotContentsBinary) -> Self { +impl From for SnapshotContents { + fn from(value: BinarySnapshotContents) -> Self { SnapshotContents::Binary(value) } } -impl SnapshotContentsString { - pub fn new(contents: String, kind: StringSnapshotKind) -> SnapshotContentsString { +impl SnapshotContents { + pub fn is_binary(&self) -> bool { + matches!(self, SnapshotContents::Binary(_)) + } + + pub fn as_string_contents(&self) -> Option<&TextSnapshotContents> { + match self { + SnapshotContents::Text(contents) => Some(contents), + SnapshotContents::Binary(_) => None, + } + } +} + +impl TextSnapshotContents { + pub fn new(contents: String, kind: TextSnapshotKind) -> TextSnapshotContents { // 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`.) - SnapshotContentsString { contents, kind } + TextSnapshotContents { contents, kind } } - // /// 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, - // } - // } - - // /// Returns the snapshot contents as string without any normalization - // pub fn as_str_exact(&self) -> &str { - // self.contents.as_str() - // } - /// Snapshot matches based on the latest format. - pub fn matches_latest(&self, other: &SnapshotContentsString) -> bool { + pub fn matches_latest(&self, other: &TextSnapshotContents) -> bool { self.to_string() == other.to_string() } - pub fn matches_legacy(&self, other: &SnapshotContentsString) -> bool { - fn as_str_legacy(sc: &SnapshotContentsString) -> String { + pub fn matches_legacy(&self, other: &TextSnapshotContents) -> bool { + fn as_str_legacy(sc: &TextSnapshotContents) -> String { let out = sc.to_string(); // Legacy inline snapshots have `---` at the start, so this strips that if // it exists. @@ -717,8 +705,8 @@ impl SnapshotContentsString { None => out, }; match sc.kind { - StringSnapshotKind::Inline => legacy_inline_normalize(&out), - StringSnapshotKind::File => out, + TextSnapshotKind::Inline => legacy_inline_normalize(&out), + TextSnapshotKind::File => out, } } as_str_legacy(self) == as_str_legacy(other) @@ -726,8 +714,8 @@ impl SnapshotContentsString { fn normalize(&self) -> String { let kind_specific_normalization = match self.kind { - StringSnapshotKind::Inline => normalize_inline_snapshot(&self.contents), - StringSnapshotKind::File => self.contents.clone(), + TextSnapshotKind::Inline => normalize_inline_snapshot(&self.contents), + TextSnapshotKind::File => self.contents.clone(), }; // Then this we do for both kinds let out = kind_specific_normalization @@ -786,26 +774,13 @@ impl SnapshotContentsString { } } -impl SnapshotContentsBinary { +impl BinarySnapshotContents { pub fn build_path(&self, path: impl Into) -> PathBuf { build_binary_path(&self.extension, path) } } -impl SnapshotContents { - pub fn is_binary(&self) -> bool { - matches!(self, SnapshotContents::Binary(_)) - } - - pub fn as_string_contents(&self) -> Option<&SnapshotContentsString> { - match self { - SnapshotContents::String(contents) => Some(contents), - SnapshotContents::Binary(_) => None, - } - } -} - -impl fmt::Display for SnapshotContentsString { +impl fmt::Display for TextSnapshotContents { /// Returns the snapshot contents as a normalized string (for example, /// removing surrounding whitespace) fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -816,7 +791,7 @@ impl fmt::Display for SnapshotContentsString { impl PartialEq for SnapshotContents { fn eq(&self, other: &Self) -> bool { match (self, other) { - (SnapshotContents::String(this), SnapshotContents::String(other)) => { + (SnapshotContents::Text(this), SnapshotContents::Text(other)) => { // Ideally match on current rules, but otherwise fall back to legacy rules if this.matches_latest(other) { true @@ -993,14 +968,14 @@ fn legacy_inline_normalize(frozen_value: &str) -> String { fn test_snapshot_contents() { use similar_asserts::assert_eq; let snapshot_contents = - SnapshotContentsString::new("testing".to_string(), StringSnapshotKind::Inline); + TextSnapshotContents::new("testing".to_string(), TextSnapshotKind::Inline); assert_eq!(snapshot_contents.to_inline(0), r#""testing""#); let t = &" a b"[1..]; assert_eq!( - SnapshotContentsString::new(t.to_string(), StringSnapshotKind::Inline).to_inline(0), + TextSnapshotContents::new(t.to_string(), TextSnapshotKind::Inline).to_inline(0), r##"r" a b @@ -1008,7 +983,7 @@ b ); assert_eq!( - SnapshotContentsString::new("a\nb".to_string(), StringSnapshotKind::Inline).to_inline(4), + TextSnapshotContents::new("a\nb".to_string(), TextSnapshotKind::Inline).to_inline(4), r##"r" a b @@ -1016,7 +991,7 @@ b ); assert_eq!( - SnapshotContentsString::new("\n a\n b".to_string(), StringSnapshotKind::Inline) + TextSnapshotContents::new("\n a\n b".to_string(), TextSnapshotKind::Inline) .to_inline(0), r##"r" a @@ -1025,8 +1000,7 @@ b ); assert_eq!( - SnapshotContentsString::new("\na\n\nb".to_string(), StringSnapshotKind::Inline) - .to_inline(4), + TextSnapshotContents::new("\na\n\nb".to_string(), TextSnapshotKind::Inline).to_inline(4), r##"r" a @@ -1035,13 +1009,12 @@ b ); assert_eq!( - SnapshotContentsString::new("\n ab\n".to_string(), StringSnapshotKind::Inline) - .to_inline(0), + TextSnapshotContents::new("\n ab\n".to_string(), TextSnapshotKind::Inline).to_inline(0), r##""ab""## ); assert_eq!( - SnapshotContentsString::new("ab".to_string(), StringSnapshotKind::Inline).to_inline(0), + TextSnapshotContents::new("ab".to_string(), TextSnapshotKind::Inline).to_inline(0), r#""ab""# ); } @@ -1049,13 +1022,12 @@ b #[test] fn test_snapshot_contents_hashes() { assert_eq!( - SnapshotContentsString::new("a###b".to_string(), StringSnapshotKind::Inline).to_inline(0), + TextSnapshotContents::new("a###b".to_string(), TextSnapshotKind::Inline).to_inline(0), r#""a###b""# ); assert_eq!( - SnapshotContentsString::new("a\n\\###b".to_string(), StringSnapshotKind::Inline) - .to_inline(0), + TextSnapshotContents::new("a\n\\###b".to_string(), TextSnapshotKind::Inline).to_inline(0), r#####"r" a \###b From 85dd8f99e06b1d4aefeb4fb8c163336edd5fc20b Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Mon, 23 Sep 2024 00:23:19 +0200 Subject: [PATCH 05/41] Add more basic tests for binary snapshots --- .../test_binary__multipart_extension.snap | 6 ++++++ ...test_binary__multipart_extension.snap.tar.gz | 1 + insta/tests/test_binary.rs | 17 +++++++++++++++++ 3 files changed, 24 insertions(+) create mode 100644 insta/tests/snapshots/test_binary__multipart_extension.snap create mode 100644 insta/tests/snapshots/test_binary__multipart_extension.snap.tar.gz diff --git a/insta/tests/snapshots/test_binary__multipart_extension.snap b/insta/tests/snapshots/test_binary__multipart_extension.snap new file mode 100644 index 00000000..8e2b383a --- /dev/null +++ b/insta/tests/snapshots/test_binary__multipart_extension.snap @@ -0,0 +1,6 @@ +--- +source: insta/tests/test_binary.rs +expression: "b\"test\".to_vec()" +extension: tar.gz +snapshot_type: binary +--- diff --git a/insta/tests/snapshots/test_binary__multipart_extension.snap.tar.gz b/insta/tests/snapshots/test_binary__multipart_extension.snap.tar.gz new file mode 100644 index 00000000..30d74d25 --- /dev/null +++ b/insta/tests/snapshots/test_binary__multipart_extension.snap.tar.gz @@ -0,0 +1 @@ +test \ No newline at end of file diff --git a/insta/tests/test_binary.rs b/insta/tests/test_binary.rs index e02c85b3..dca4eae1 100644 --- a/insta/tests/test_binary.rs +++ b/insta/tests/test_binary.rs @@ -2,3 +2,20 @@ fn test_binary_snapshot() { insta::assert_binary_snapshot!("txt", b"test".to_vec()); } + +#[test] +#[should_panic(expected = "this file extension is not allowed")] +fn test_new_extension() { + insta::assert_binary_snapshot!("new", b"test".to_vec()); +} + +#[test] +#[should_panic(expected = "file extensions starting with 'new.' are not allowed")] +fn test_extension_starting_with_new() { + insta::assert_binary_snapshot!("new.gz", b"test".to_vec()); +} + +#[test] +fn test_multipart_extension() { + insta::assert_binary_snapshot!("tar.gz", b"test".to_vec()); +} From d3bad2a41a8a911a66f47a7db3b13376fd86e3e3 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Mon, 23 Sep 2024 04:04:36 +0200 Subject: [PATCH 06/41] Add integration tests for binary snapshots --- cargo-insta/tests/main.rs | 203 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 203 insertions(+) diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index 33115fb7..f9d45ece 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -81,6 +81,17 @@ fn assert_success(output: &std::process::Output) { ); } +fn assert_failure(output: &std::process::Output) { + eprint!("{}", String::from_utf8_lossy(&output.stderr)); + eprint!("{}", String::from_utf8_lossy(&output.stdout)); + assert!( + !output.status.success(), + "Tests succeeded: {}\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); +} + struct TestProject { /// Temporary directory where the project is created workspace_dir: PathBuf, @@ -183,6 +194,10 @@ impl TestProject { Some(("Original file tree", "Updated file tree")), ) } + + fn update_file>(&self, path: P, content: String) { + fs::write(self.workspace_dir.join(path), content).unwrap(); + } } #[test] @@ -1147,3 +1162,191 @@ fn test_hashtag_escape() { } "####); } + +#[test] +fn test_binary_pending() { + let test_project = TestFiles::new() + .add_file( + "Cargo.toml", + r#" +[package] +name = "test_binary_pending" +version = "0.1.0" +edition = "2021" + +[dependencies] +insta = { path = '$PROJECT_PATH' } +"# + .to_string(), + ) + .add_file( + "src/main.rs", + r#" +#[test] +fn test_binary_snapshot() { + insta::assert_binary_snapshot!("txt", b"test".to_vec()); +} +"# + .to_string(), + ) + .create_project(); + + let output = test_project.cmd().args(["test"]).output().unwrap(); + + assert_failure(&output); + + assert_snapshot!(test_project.file_tree_diff(), @r" + --- Original file tree + +++ Updated file tree + @@ -1,4 +1,8 @@ + + + Cargo.lock + Cargo.toml + src + src/main.rs + + src/snapshots + + src/snapshots/test_binary_pending__binary_snapshot.snap.new + + src/snapshots/test_binary_pending__binary_snapshot.snap.new.txt + "); +} + +#[test] +fn test_binary_accept() { + let test_project = TestFiles::new() + .add_file( + "Cargo.toml", + r#" +[package] +name = "test_binary_accept" +version = "0.1.0" +edition = "2021" + +[dependencies] +insta = { path = '$PROJECT_PATH' } +"# + .to_string(), + ) + .add_file( + "src/main.rs", + r#" +#[test] +fn test_binary_snapshot() { + insta::assert_binary_snapshot!("txt", b"test".to_vec()); +} +"# + .to_string(), + ) + .create_project(); + + let output = test_project + .cmd() + .args(["test", "--accept"]) + .output() + .unwrap(); + + assert_success(&output); + + assert_snapshot!(test_project.file_tree_diff(), @r" + --- Original file tree + +++ Updated file tree + @@ -1,4 +1,8 @@ + + + Cargo.lock + Cargo.toml + src + src/main.rs + + src/snapshots + + src/snapshots/test_binary_accept__binary_snapshot.snap + + src/snapshots/test_binary_accept__binary_snapshot.snap.txt + "); +} + +#[test] +fn test_binary_change_extension() { + let test_project = TestFiles::new() + .add_file( + "Cargo.toml", + r#" +[package] +name = "test_binary_change_extension" +version = "0.1.0" +edition = "2021" + +[dependencies] +insta = { path = '$PROJECT_PATH' } +"# + .to_string(), + ) + .add_file( + "src/main.rs", + r#" +#[test] +fn test_binary_snapshot() { + insta::assert_binary_snapshot!("txt", b"test".to_vec()); +} +"# + .to_string(), + ) + .create_project(); + + let output = test_project + .cmd() + .args(["test", "--accept"]) + .output() + .unwrap(); + + assert_success(&output); + + test_project.update_file( + "src/main.rs", + r#" +#[test] +fn test_binary_snapshot() { + insta::assert_binary_snapshot!("json", b"test".to_vec()); +} +"# + .to_string(), + ); + + let output = test_project.cmd().args(["test"]).output().unwrap(); + + assert_failure(&output); + + assert_snapshot!(test_project.file_tree_diff(), @r" + --- Original file tree + +++ Updated file tree + @@ -1,4 +1,10 @@ + + + Cargo.lock + Cargo.toml + src + src/main.rs + + src/snapshots + + src/snapshots/test_binary_change_extension__binary_snapshot.snap + + src/snapshots/test_binary_change_extension__binary_snapshot.snap.new + + src/snapshots/test_binary_change_extension__binary_snapshot.snap.new.json + + src/snapshots/test_binary_change_extension__binary_snapshot.snap.txt + "); + + let output = test_project + .cmd() + .args(["test", "--accept"]) + .output() + .unwrap(); + + assert_success(&output); + + assert_snapshot!(test_project.file_tree_diff(), @r" + --- Original file tree + +++ Updated file tree + @@ -1,4 +1,8 @@ + + + Cargo.lock + Cargo.toml + src + src/main.rs + + src/snapshots + + src/snapshots/test_binary_change_extension__binary_snapshot.snap + + src/snapshots/test_binary_change_extension__binary_snapshot.snap.json + "); +} From 5bb0f70520e6500d7d8543739ec70c6de0168908 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Mon, 23 Sep 2024 04:15:51 +0200 Subject: [PATCH 07/41] Use `Self` type as parameter for matches* methods --- insta/src/snapshot.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index 6e6a8999..f1a4a01b 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -518,14 +518,14 @@ impl Snapshot { } /// Snapshot contents match another snapshot's. - pub fn matches(&self, other: &Snapshot) -> bool { + pub fn matches(&self, other: &Self) -> bool { self.contents() == other.contents() } /// 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 { + pub fn matches_fully(&self, other: &Self) -> bool { match (self.contents(), other.contents()) { (SnapshotContents::Text(self_contents), SnapshotContents::Text(other_contents)) => { let contents_match_exact = self_contents == other_contents; @@ -691,11 +691,11 @@ impl TextSnapshotContents { } /// Snapshot matches based on the latest format. - pub fn matches_latest(&self, other: &TextSnapshotContents) -> bool { + pub fn matches_latest(&self, other: &Self) -> bool { self.to_string() == other.to_string() } - pub fn matches_legacy(&self, other: &TextSnapshotContents) -> bool { + pub fn matches_legacy(&self, other: &Self) -> bool { fn as_str_legacy(sc: &TextSnapshotContents) -> String { let out = sc.to_string(); // Legacy inline snapshots have `---` at the start, so this strips that if From 7607518395710c4b78ac611db6de00818afaf926 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Mon, 23 Sep 2024 04:24:49 +0200 Subject: [PATCH 08/41] Improve panic message for forbidden `.new` binary snapshot file extension --- insta/src/runtime.rs | 5 ++++- insta/tests/test_binary.rs | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index b7b7c508..8cd05dc7 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -717,7 +717,10 @@ pub fn assert_snapshot( SnapshotValue::Binary { content, extension, .. } => { - assert!(extension != "new", "this file extension is not allowed"); + assert!( + extension != "new", + "'.new' is not allowed as a file extension" + ); assert!( !extension.starts_with("new."), "file extensions starting with 'new.' are not allowed", diff --git a/insta/tests/test_binary.rs b/insta/tests/test_binary.rs index dca4eae1..7a2986a1 100644 --- a/insta/tests/test_binary.rs +++ b/insta/tests/test_binary.rs @@ -4,7 +4,7 @@ fn test_binary_snapshot() { } #[test] -#[should_panic(expected = "this file extension is not allowed")] +#[should_panic(expected = "'.new' is not allowed as a file extension")] fn test_new_extension() { insta::assert_binary_snapshot!("new", b"test".to_vec()); } From 58f459c59e22ea89e2a4899a71f15fa6e7e64f2e Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Mon, 23 Sep 2024 05:14:39 +0200 Subject: [PATCH 09/41] Fix MSRV related issues --- cargo-insta/src/cli.rs | 2 +- insta/src/snapshot.rs | 20 ++++++++++++-------- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/cargo-insta/src/cli.rs b/cargo-insta/src/cli.rs index 6086f0e3..f89eeb71 100644 --- a/cargo-insta/src/cli.rs +++ b/cargo-insta/src/cli.rs @@ -323,7 +323,7 @@ fn query_snapshot( ); let new_is_binary = new.contents().is_binary(); - let old_is_binary = old.is_some_and(|o| o.contents().is_binary()); + let old_is_binary = old.map(|o| o.contents().is_binary()).unwrap_or(false); if new_is_binary || old_is_binary { println!( diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index f1a4a01b..d0b006c3 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -604,23 +604,27 @@ impl Snapshot { }); let binary_file_name = binary_path.as_ref().map(|path| path.file_name().unwrap()); - let file_name = path.file_name().unwrap(); + // The file name to compare against has to be valid utf-8 as it is generated by this crate + // out of utf-8 strings. + let file_name = path.file_name().unwrap().to_str().unwrap(); let read_dir = path.parent().unwrap().read_dir(); - if read_dir - .as_ref() - .is_err_and(|e| e.kind() == ErrorKind::NotFound) - { - return Ok(()); + match read_dir { + Err(e) if e.kind() == ErrorKind::NotFound => return Ok(()), + _ => (), } for entry in read_dir? { let entry = entry?; let entry_file_name = entry.file_name(); + + // We'll just skip over files with non-utf-8 names. The assumption being that those + // would not have been generated by this crate. if entry_file_name - .as_encoded_bytes() - .starts_with(file_name.as_encoded_bytes()) + .to_str() + .map(|f| f.starts_with(file_name)) + .unwrap_or(false) && entry_file_name != file_name && binary_file_name.map_or(true, |x| x != entry_file_name) { From 5aa7a136e3c4067eabf66707015674465f564b7f Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Mon, 23 Sep 2024 07:11:42 +0200 Subject: [PATCH 10/41] Add docstrings and rename `Text` to `FileText` in SnapshotValue enum --- insta/src/runtime.rs | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 8cd05dc7..372e2ac4 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -63,29 +63,38 @@ impl From for SnapshotName<'static> { } } +/// The name of a snapshot, from which the path is derived. type SnapshotName<'a> = Option>; pub enum SnapshotValue<'a> { - Text { + /// A text snapshot that gets stored along with the metadata in the same file. + FileText { name: SnapshotName<'a>, content: &'a str, }, + /// An inline snapshot. InlineText { + /// The reference content from the macro invocation that will be compared against. reference_content: &'a str, + + /// The new content from the expression that is passed to the macro. content: &'a str, }, + /// A binary snapshot that gets stored as a separate file next to the metadata file. Binary { name: SnapshotName<'a>, content: Vec, + + /// The extension of the separate file. extension: &'a str, }, } impl<'a> From<(AutoName, &'a str)> for SnapshotValue<'a> { fn from((_, content): (AutoName, &'a str)) -> Self { - SnapshotValue::Text { + SnapshotValue::FileText { name: None, content, } @@ -94,7 +103,7 @@ impl<'a> From<(AutoName, &'a str)> for SnapshotValue<'a> { impl<'a> From<(Option, &'a str)> for SnapshotValue<'a> { fn from((name, content): (Option, &'a str)) -> Self { - SnapshotValue::Text { + SnapshotValue::FileText { name: name.map(Cow::Owned), content, } @@ -103,7 +112,7 @@ impl<'a> From<(Option, &'a str)> for SnapshotValue<'a> { impl<'a> From<(String, &'a str)> for SnapshotValue<'a> { fn from((name, content): (String, &'a str)) -> Self { - SnapshotValue::Text { + SnapshotValue::FileText { name: Some(Cow::Owned(name)), content, } @@ -112,7 +121,7 @@ impl<'a> From<(String, &'a str)> for SnapshotValue<'a> { impl<'a> From<(Option<&'a str>, &'a str)> for SnapshotValue<'a> { fn from((name, content): (Option<&'a str>, &'a str)) -> Self { - SnapshotValue::Text { + SnapshotValue::FileText { name: name.map(Cow::Borrowed), content, } @@ -121,7 +130,7 @@ impl<'a> From<(Option<&'a str>, &'a str)> for SnapshotValue<'a> { impl<'a> From<(&'a str, &'a str)> for SnapshotValue<'a> { fn from((name, content): (&'a str, &'a str)) -> Self { - SnapshotValue::Text { + SnapshotValue::FileText { name: Some(Cow::Borrowed(name)), content, } @@ -285,7 +294,7 @@ impl<'a> SnapshotAssertionContext<'a> { let is_doctest = is_doctest(function_name); match new_snapshot_value { - SnapshotValue::Text { name, .. } | SnapshotValue::Binary { name, .. } => { + SnapshotValue::FileText { name, .. } | SnapshotValue::Binary { name, .. } => { let name = match name { Some(name) => add_suffix_to_snapshot_name(name.clone()), None => { @@ -702,7 +711,7 @@ pub fn assert_snapshot( } let content = match new_snapshot_value { - SnapshotValue::Text { content, .. } | SnapshotValue::InlineText { content, .. } => { + SnapshotValue::FileText { content, .. } | SnapshotValue::InlineText { content, .. } => { // apply filters if they are available #[cfg(feature = "filters")] let content = Settings::with(|settings| settings.filters().apply_to(content)); From 907dd6346b9b879d1c5c771190de4b939095b972 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Tue, 24 Sep 2024 11:51:05 +0200 Subject: [PATCH 11/41] Improve error message for integration tests --- cargo-insta/tests/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index f9d45ece..83ae7f1e 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -86,7 +86,7 @@ fn assert_failure(output: &std::process::Output) { eprint!("{}", String::from_utf8_lossy(&output.stdout)); assert!( !output.status.success(), - "Tests succeeded: {}\n{}", + "Tests unexpectedly succeeded: {}\n{}", String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr) ); From 931016e51ea70659ad24820da0b5fdcadaffa0d8 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Tue, 24 Sep 2024 12:28:11 +0200 Subject: [PATCH 12/41] Update docstrings --- insta/src/runtime.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 2d85fb5e..c673806e 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -70,6 +70,8 @@ pub enum SnapshotValue<'a> { /// A text snapshot that gets stored along with the metadata in the same file. FileText { name: SnapshotName<'a>, + + /// The new generated value to compare against any previously approved content. content: &'a str, }, @@ -78,13 +80,15 @@ pub enum SnapshotValue<'a> { /// The reference content from the macro invocation that will be compared against. reference_content: &'a str, - /// The new content from the expression that is passed to the macro. + /// The new generated value to compare against any previously approved content. content: &'a str, }, /// A binary snapshot that gets stored as a separate file next to the metadata file. Binary { name: SnapshotName<'a>, + + /// The new generated value to compare against any previously approved content. content: Vec, /// The extension of the separate file. From 7d82e108258e115c65681fe336bfbb61d48a4913 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Wed, 25 Sep 2024 17:42:09 +0200 Subject: [PATCH 13/41] Fix named binary snapshots As part of this I changed the macro internals for the text snapshots as well. --- insta/src/lib.rs | 2 +- insta/src/macros.rs | 44 ++++++++----- insta/src/runtime.rs | 66 +++++-------------- insta/tests/snapshots/test_binary__name.snap | 6 ++ .../snapshots/test_binary__name.snap.json | 1 + insta/tests/test_binary.rs | 5 ++ 6 files changed, 56 insertions(+), 68 deletions(-) create mode 100644 insta/tests/snapshots/test_binary__name.snap create mode 100644 insta/tests/snapshots/test_binary__name.snap.json diff --git a/insta/src/lib.rs b/insta/src/lib.rs index c32dd513..9917d564 100644 --- a/insta/src/lib.rs +++ b/insta/src/lib.rs @@ -335,7 +335,7 @@ pub mod _macro_support { pub use crate::content::Content; pub use crate::env::get_cargo_workspace; pub use crate::runtime::{ - assert_snapshot, with_allow_duplicates, AutoName, InlineValue, SnapshotValue, + assert_snapshot, with_allow_duplicates, AutoName, SnapshotName, SnapshotValue, }; #[cfg(feature = "serde")] diff --git a/insta/src/macros.rs b/insta/src/macros.rs index e06dcae8..effad91e 100644 --- a/insta/src/macros.rs +++ b/insta/src/macros.rs @@ -320,37 +320,45 @@ macro_rules! assert_compact_debug_snapshot { #[doc(hidden)] #[macro_export] macro_rules! _assert_snapshot_base { - // If there's an inline literal value, wrap the literal in a - // `ReferenceValue::Inline`, call self. - (transform=$transform:expr, $($arg:expr),*, @$snapshot:literal $(,)?) => { + // If there's an inline literal value, wrap the literal and value in a + // `SnapshotValue::InlineText`, call self. + (transform=$transform:expr, $value:expr $(, $debug_expr:expr)?, @$snapshot:literal $(,)?) => { $crate::_assert_snapshot_base!( - transform = $transform, #[allow(clippy::needless_raw_string_hashes)] - $crate::_macro_support::InlineValue($snapshot), - $($arg),* + $crate::_macro_support::SnapshotValue::InlineText { + reference_content: $snapshot, + content: $transform(&$value).as_str(), + }, + $($debug_expr,)? + stringify!($value), ) }; - // If there's no debug_expr, use the stringified value, call self. - (transform=$transform:expr, $name:expr, $value:expr $(,)?) => { - $crate::_assert_snapshot_base!(transform = $transform, $name, $value, stringify!($value)) - }; // If there's no name (and necessarily no debug expr), auto generate the // name, call self. (transform=$transform:expr, $value:expr $(,)?) => { $crate::_assert_snapshot_base!( - transform = $transform, + transform=$transform, $crate::_macro_support::AutoName, - $value + $value, + ) + }; + // If there's a name, wrap the name and value in a SnapshotValue::FileText, call self. + (transform=$transform:expr, $name:expr, $value:expr $(, $debug_expr:expr)? $(,)?) => { + $crate::_assert_snapshot_base!( + $crate::_macro_support::SnapshotValue::FileText { + name: $name.into(), + content: $transform(&$value).as_str(), + }, + $($debug_expr,)? + stringify!($value), ) }; // The main macro body — every call to this macro should end up here. - (transform=$transform:expr, $name:expr, $value:expr, $debug_expr:expr $(,)?) => { + // The extra value gets ignored here because above we can't only pass $value if there's no + // $debug_expr due to macro_rules not having anything like an else. + ($snapshot_value:expr, $debug_expr:expr $(, $_:expr)? $(,)?) => { $crate::_macro_support::assert_snapshot( - ( - $name, - #[allow(clippy::redundant_closure_call)] - $transform(&$value).as_str(), - ).into(), + $snapshot_value, env!("CARGO_MANIFEST_DIR"), $crate::_function_name!(), module_path!(), diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 6798878a..df62d1eb 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -55,16 +55,8 @@ macro_rules! elog { #[derive(Debug)] pub struct AutoName; -pub struct InlineValue<'a>(pub &'a str); - -impl From for SnapshotName<'static> { - fn from(_value: AutoName) -> SnapshotName<'static> { - None - } -} - /// The name of a snapshot, from which the path is derived. -type SnapshotName<'a> = Option>; +pub struct SnapshotName<'a>(pub Option>); pub enum SnapshotValue<'a> { /// A text snapshot that gets stored along with the metadata in the same file. @@ -96,57 +88,33 @@ pub enum SnapshotValue<'a> { }, } -impl<'a> From<(AutoName, &'a str)> for SnapshotValue<'a> { - fn from((_, content): (AutoName, &'a str)) -> Self { - SnapshotValue::FileText { - name: None, - content, - } - } -} - -impl<'a> From<(Option, &'a str)> for SnapshotValue<'a> { - fn from((name, content): (Option, &'a str)) -> Self { - SnapshotValue::FileText { - name: name.map(Cow::Owned), - content, - } +impl From for SnapshotName<'static> { + fn from(_value: AutoName) -> SnapshotName<'static> { + SnapshotName(None) } } -impl<'a> From<(String, &'a str)> for SnapshotValue<'a> { - fn from((name, content): (String, &'a str)) -> Self { - SnapshotValue::FileText { - name: Some(Cow::Owned(name)), - content, - } +impl From for SnapshotName<'static> { + fn from(value: String) -> SnapshotName<'static> { + SnapshotName(Some(Cow::Owned(value))) } } -impl<'a> From<(Option<&'a str>, &'a str)> for SnapshotValue<'a> { - fn from((name, content): (Option<&'a str>, &'a str)) -> Self { - SnapshotValue::FileText { - name: name.map(Cow::Borrowed), - content, - } +impl From> for SnapshotName<'static> { + fn from(value: Option) -> Self { + SnapshotName(value.map(Cow::Owned)) } } -impl<'a> From<(&'a str, &'a str)> for SnapshotValue<'a> { - fn from((name, content): (&'a str, &'a str)) -> Self { - SnapshotValue::FileText { - name: Some(Cow::Borrowed(name)), - content, - } +impl<'a> From<&'a str> for SnapshotName<'a> { + fn from(value: &'a str) -> SnapshotName<'a> { + SnapshotName(Some(Cow::Borrowed(value))) } } -impl<'a> From<(InlineValue<'a>, &'a str)> for SnapshotValue<'a> { - fn from((InlineValue(reference_content), content): (InlineValue<'a>, &'a str)) -> Self { - SnapshotValue::InlineText { - reference_content, - content, - } +impl<'a> From> for SnapshotName<'a> { + fn from(value: Option<&'a str>) -> Self { + SnapshotName(value.map(Cow::Borrowed)) } } @@ -299,7 +267,7 @@ impl<'a> SnapshotAssertionContext<'a> { match new_snapshot_value { SnapshotValue::FileText { name, .. } | SnapshotValue::Binary { name, .. } => { - let name = match name { + let name = match &name.0 { Some(name) => add_suffix_to_snapshot_name(name.clone()), None => { if is_doctest { diff --git a/insta/tests/snapshots/test_binary__name.snap b/insta/tests/snapshots/test_binary__name.snap new file mode 100644 index 00000000..a99b42ee --- /dev/null +++ b/insta/tests/snapshots/test_binary__name.snap @@ -0,0 +1,6 @@ +--- +source: insta/tests/test_binary.rs +expression: "b\"null\".to_vec()" +extension: json +snapshot_type: binary +--- diff --git a/insta/tests/snapshots/test_binary__name.snap.json b/insta/tests/snapshots/test_binary__name.snap.json new file mode 100644 index 00000000..ec747fa4 --- /dev/null +++ b/insta/tests/snapshots/test_binary__name.snap.json @@ -0,0 +1 @@ +null \ No newline at end of file diff --git a/insta/tests/test_binary.rs b/insta/tests/test_binary.rs index 7a2986a1..9e0346ac 100644 --- a/insta/tests/test_binary.rs +++ b/insta/tests/test_binary.rs @@ -19,3 +19,8 @@ fn test_extension_starting_with_new() { fn test_multipart_extension() { insta::assert_binary_snapshot!("tar.gz", b"test".to_vec()); } + +#[test] +fn test_named() { + insta::assert_binary_snapshot!("json", "name", b"null".to_vec()); +} From 4727fb1780407fe44481b83a727c07b0e8f4808d Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Wed, 25 Sep 2024 18:01:11 +0200 Subject: [PATCH 14/41] Add integration test for removal of pending binary snapshot --- cargo-insta/tests/main.rs | 51 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index 83ae7f1e..72cbbb5b 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -1350,3 +1350,54 @@ fn test_binary_snapshot() { + src/snapshots/test_binary_change_extension__binary_snapshot.snap.json "); } + +#[test] +fn test_binary_pending_snapshot_removal() { + let test_project = TestFiles::new() + .add_file( + "Cargo.toml", + r#" +[package] +name = "test_binary_pending_snapshot_removal" +version = "0.1.0" +edition = "2021" + +[dependencies] +insta = { path = '$PROJECT_PATH' } +"# + .to_string(), + ) + .add_file( + "src/main.rs", + r#" +#[test] +fn test_binary_snapshot() { + insta::assert_binary_snapshot!("txt", b"test".to_vec()); +} +"# + .to_string(), + ) + .create_project(); + + let output = test_project.cmd().args(["test"]).output().unwrap(); + + assert_failure(&output); + + test_project.update_file("src/main.rs", "".to_string()); + + let output = test_project.cmd().args(["test"]).output().unwrap(); + + assert_success(&output); + + assert_snapshot!(test_project.file_tree_diff(), @r" + --- Original file tree + +++ Updated file tree + @@ -1,4 +1,6 @@ + + + Cargo.lock + Cargo.toml + src + src/main.rs + + src/snapshots + "); +} From 654e1a6ee8038a616d254fd8376f4e199aab6780 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Wed, 25 Sep 2024 18:13:57 +0200 Subject: [PATCH 15/41] Add integration tests for changing between binary and text snapshots --- cargo-insta/tests/main.rs | 162 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 162 insertions(+) diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index 72cbbb5b..232a1d1a 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -1401,3 +1401,165 @@ fn test_binary_snapshot() { + src/snapshots "); } + +#[test] +fn test_change_text_to_binary() { + let test_project = TestFiles::new() + .add_file( + "Cargo.toml", + r#" +[package] +name = "test_change_text_to_binary" +version = "0.1.0" +edition = "2021" + +[dependencies] +insta = { path = '$PROJECT_PATH' } +"# + .to_string(), + ) + .add_file( + "src/main.rs", + r#" +#[test] +fn test() { + insta::assert_snapshot!("test"); +} +"# + .to_string(), + ) + .create_project(); + + let output = test_project + .cmd() + .args(["test", "--accept"]) + .output() + .unwrap(); + + assert_success(&output); + assert_snapshot!(test_project.file_tree_diff(), @r" + --- Original file tree + +++ Updated file tree + @@ -1,4 +1,7 @@ + + + Cargo.lock + Cargo.toml + src + src/main.rs + + src/snapshots + + src/snapshots/test_change_text_to_binary__test.snap + "); + + test_project.update_file( + "src/main.rs", + r#" +#[test] +fn test() { + insta::assert_binary_snapshot!("txt", b"test".to_vec()); +} +"# + .to_string(), + ); + + let output = test_project + .cmd() + .args(["test", "--accept"]) + .output() + .unwrap(); + + assert_success(&output); + assert_snapshot!(test_project.file_tree_diff(), @r" + --- Original file tree + +++ Updated file tree + @@ -1,4 +1,8 @@ + + + Cargo.lock + Cargo.toml + src + src/main.rs + + src/snapshots + + src/snapshots/test_change_text_to_binary__test.snap + + src/snapshots/test_change_text_to_binary__test.snap.txt + "); +} + +#[test] +fn test_change_binary_to_text() { + let test_project = TestFiles::new() + .add_file( + "Cargo.toml", + r#" +[package] +name = "test_change_binary_to_text" +version = "0.1.0" +edition = "2021" + +[dependencies] +insta = { path = '$PROJECT_PATH' } +"# + .to_string(), + ) + .add_file( + "src/main.rs", + r#" +#[test] +fn test() { + insta::assert_binary_snapshot!("json", "some_name", b"{}".to_vec()); +} +"# + .to_string(), + ) + .create_project(); + + let output = test_project + .cmd() + .args(["test", "--accept"]) + .output() + .unwrap(); + + assert_success(&output); + assert_snapshot!(test_project.file_tree_diff(), @r" + --- Original file tree + +++ Updated file tree + @@ -1,4 +1,8 @@ + + + Cargo.lock + Cargo.toml + src + src/main.rs + + src/snapshots + + src/snapshots/test_change_binary_to_text__some_name.snap + + src/snapshots/test_change_binary_to_text__some_name.snap.json + "); + + test_project.update_file( + "src/main.rs", + r#" +#[test] +fn test() { + insta::assert_snapshot!("some_name", "test"); +} +"# + .to_string(), + ); + + let output = test_project + .cmd() + .args(["test", "--accept"]) + .output() + .unwrap(); + + assert_success(&output); + assert_snapshot!(test_project.file_tree_diff(), @r" + --- Original file tree + +++ Updated file tree + @@ -1,4 +1,7 @@ + + + Cargo.lock + Cargo.toml + src + src/main.rs + + src/snapshots + + src/snapshots/test_change_binary_to_text__some_name.snap + "); +} From 1c7f54654237992dd18d63f832dcc7d023fc5871 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Thu, 26 Sep 2024 15:25:03 +0200 Subject: [PATCH 16/41] Update cleanup of binary snapshot files cleanup_extra_files gets replaced with cleanup_previous_pending_binary_snapshots and in cargo-insta we directly remove the relevant binary files now because the snapshots are already loaded there. --- cargo-insta/src/container.rs | 39 ++++++++++++++++---------------- insta/src/runtime.rs | 43 ++++++++++++++++++++++++++++++++---- insta/src/snapshot.rs | 40 +-------------------------------- 3 files changed, 60 insertions(+), 62 deletions(-) diff --git a/cargo-insta/src/container.rs b/cargo-insta/src/container.rs index 0bb6a76d..10255a81 100644 --- a/cargo-insta/src/container.rs +++ b/cargo-insta/src/container.rs @@ -2,6 +2,7 @@ use std::error::Error; use std::fs; use std::path::{Path, PathBuf}; +use insta::internals::SnapshotContents; use insta::Snapshot; pub(crate) use insta::TextSnapshotKind; use insta::_cargo_insta_support::{ContentError, PendingInlineSnapshot}; @@ -167,7 +168,7 @@ impl SnapshotContainer { // Try removing the snapshot file. If it fails, it's // likely because it another process removed it; which // is fine — print a message and continue. - let try_removing_snapshot = |p| { + let try_removing_snapshot = |p: &Path| { fs::remove_file(p).unwrap_or_else(|_| { eprintln!( "Pending snapshot file at {:?} couldn't be removed. It was likely removed by another process.", @@ -216,31 +217,31 @@ impl SnapshotContainer { for snapshot in self.snapshots.iter() { match snapshot.op { Operation::Accept => { - let snapshot = Snapshot::from_file(&self.pending_path).map_err(|e| { - // If it's an IO error, pass a ContentError back so - // we get a slightly clearer error message - match e.downcast::() { - Ok(io_error) => Box::new(ContentError::FileIo( - *io_error, - self.pending_path.to_path_buf(), - )), - Err(other_error) => other_error, - } - })?; - snapshot.save(&self.target_path)?; try_removing_snapshot(&self.pending_path); + + if let Some(ref old) = snapshot.old { + if let SnapshotContents::Binary(contents) = old.contents() { + try_removing_snapshot(&contents.build_path(&self.target_path)); + } + } + + if let SnapshotContents::Binary(contents) = snapshot.new.contents() { + try_removing_snapshot(&contents.build_path(&self.pending_path)); + } + + // We save at the end because we might write a binary file into the same + // path again. + snapshot.new.save(&self.target_path)?; } Operation::Reject => { try_removing_snapshot(&self.pending_path); + + if let SnapshotContents::Binary(contents) = snapshot.new.contents() { + try_removing_snapshot(&contents.build_path(&self.pending_path)); + } } Operation::Skip => {} } - - if let Operation::Accept | Operation::Reject = snapshot.op { - let snapshot = Snapshot::from_file(&self.target_path).ok(); - - Snapshot::cleanup_extra_files(snapshot.as_ref(), &self.target_path)?; - } } } Ok(()) diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index df62d1eb..28268227 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -2,7 +2,7 @@ use std::cell::RefCell; use std::collections::{BTreeMap, BTreeSet}; use std::error::Error; use std::fs; -use std::io::Write; +use std::io::{ErrorKind, Write}; use std::path::{Path, PathBuf}; use std::rc::Rc; use std::str; @@ -401,6 +401,43 @@ impl<'a> SnapshotAssertionContext<'a> { Ok(()) } + /// Removes any old .snap.new.* files that belonged to previous pending snapshots. This should + /// only ever remove maxium one file because we do this every time before we create a new + /// pending snapshot. + pub fn cleanup_previous_pending_binary_snapshots(&self) -> Result<(), Box> { + if let Some(ref path) = self.snapshot_file { + // The file name to compare against has to be valid utf-8 as it is generated by this crate + // out of utf-8 strings. + let file_name_prefix = format!("{}.new.", path.file_name().unwrap().to_str().unwrap()); + + let read_dir = path.parent().unwrap().read_dir(); + + match read_dir { + Err(e) if e.kind() == ErrorKind::NotFound => return Ok(()), + _ => (), + } + + // We have to loop over where whole directory here because there is no filesystem API + // for getting files by prefix. + for entry in read_dir? { + let entry = entry?; + let entry_file_name = entry.file_name(); + + // We'll just skip over files with non-utf-8 names. The assumption being that those + // would not have been generated by this crate. + if entry_file_name + .to_str() + .map(|f| f.starts_with(&file_name_prefix)) + .unwrap_or(false) + { + std::fs::remove_file(entry.path())?; + } + } + } + + Ok(()) + } + /// Writes the changes of the snapshot back. pub fn update_snapshot( &self, @@ -676,9 +713,7 @@ pub fn assert_snapshot( assertion_line, )?; - if let Some(snapshot_file) = &ctx.snapshot_file { - Snapshot::cleanup_extra_files(ctx.old_snapshot.as_ref(), snapshot_file)?; - } + ctx.cleanup_previous_pending_binary_snapshots()?; let content = match new_snapshot_value { SnapshotValue::FileText { content, .. } | SnapshotValue::InlineText { content, .. } => { diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index c12197a6..41e1d1c2 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -1,7 +1,7 @@ use std::env; use std::error::Error; use std::fs; -use std::io::{BufRead, BufReader, ErrorKind, Write}; +use std::io::{BufRead, BufReader, Write}; use std::path::{Path, PathBuf}; use std::rc::Rc; use std::time::{SystemTime, UNIX_EPOCH}; @@ -598,44 +598,6 @@ impl Snapshot { self.save_with_metadata(&new_path, &self.metadata)?; Ok(new_path) } - - pub fn cleanup_extra_files(snapshot: Option<&Self>, path: &Path) -> Result<(), Box> { - let binary_path = snapshot.and_then(|snapshot| match &snapshot.contents() { - SnapshotContents::Text(_) => None, - SnapshotContents::Binary(contents) => Some(contents.build_path(path)), - }); - let binary_file_name = binary_path.as_ref().map(|path| path.file_name().unwrap()); - - // The file name to compare against has to be valid utf-8 as it is generated by this crate - // out of utf-8 strings. - let file_name = path.file_name().unwrap().to_str().unwrap(); - - let read_dir = path.parent().unwrap().read_dir(); - - match read_dir { - Err(e) if e.kind() == ErrorKind::NotFound => return Ok(()), - _ => (), - } - - for entry in read_dir? { - let entry = entry?; - let entry_file_name = entry.file_name(); - - // We'll just skip over files with non-utf-8 names. The assumption being that those - // would not have been generated by this crate. - if entry_file_name - .to_str() - .map(|f| f.starts_with(file_name)) - .unwrap_or(false) - && entry_file_name != file_name - && binary_file_name.map_or(true, |x| x != entry_file_name) - { - std::fs::remove_file(entry.path())?; - } - } - - Ok(()) - } } /// The contents of a Snapshot From 091b1a4a3dbb83c83297fbb3ae9cfca6cc400777 Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Thu, 26 Sep 2024 12:39:20 -0700 Subject: [PATCH 17/41] Add a couple comments (#617) As I'm trying to understand parts of the code --- insta/src/env.rs | 2 +- insta/src/runtime.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/insta/src/env.rs b/insta/src/env.rs index 807f8d36..776ef8b0 100644 --- a/insta/src/env.rs +++ b/insta/src/env.rs @@ -480,7 +480,7 @@ impl std::str::FromStr for UnreferencedSnapshots { } } -/// Memoizes a snapshot file in the reference file. +/// Memoizes a snapshot file in the reference file, as part of removing unreferenced snapshots. pub fn memoize_snapshot_file(snapshot_file: &Path) { if let Ok(path) = env::var("INSTA_SNAPSHOT_REFERENCES_FILE") { let mut f = fs::OpenOptions::new() diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 28268227..f3966220 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -750,7 +750,7 @@ pub fn assert_snapshot( let new_snapshot = ctx.new_snapshot(content, expr); - // memoize the snapshot file if requested. + // memoize the snapshot file if requested, as part of potentially removing unreferenced snapshots if let Some(ref snapshot_file) = ctx.snapshot_file { memoize_snapshot_file(snapshot_file); } From 2d08233cabf00af7c46b7705b6bd2a263b96f357 Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Thu, 26 Sep 2024 12:42:16 -0700 Subject: [PATCH 18/41] Add colors to integration test output to discriminate between inner & outer tests (#616) Also add some docs --- Cargo.lock | 1 + cargo-insta/Cargo.toml | 1 + cargo-insta/tests/main.rs | 39 ++++++++++++++++++++++++++++++--------- 3 files changed, 32 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 341e736f..beadf28b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -83,6 +83,7 @@ dependencies = [ "similar", "syn", "tempfile", + "termcolor", "uuid", "walkdir", ] diff --git a/cargo-insta/Cargo.toml b/cargo-insta/Cargo.toml index bd052c3d..d0e4707b 100644 --- a/cargo-insta/Cargo.toml +++ b/cargo-insta/Cargo.toml @@ -36,3 +36,4 @@ open = "5.3.0" walkdir = "2.3.1" similar= "2.2.1" itertools = "0.10.0" +termcolor = "1.1.2" diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index 232a1d1a..3dd7d1c0 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -1,6 +1,18 @@ /// Integration tests which allow creating a full repo, running `cargo-insta` /// and then checking the output. /// +/// Often we want to see output from the test commands we run here; for example +/// a `dbg!` statement we add while debugging. Cargo by default hides the output +/// of passing tests. +/// - Like any test, to forward the output of an outer test (i.e. one of the +/// `#[test]`s in this file) to the terminal, pass `--nocapture` to the test +/// runner, like `cargo insta test -- --nocapture`. +/// - To forward the output of an inner test (i.e. the test commands we create +/// and run within an outer test) to the output of an outer test, pass +/// `--nocapture` in the command we create; for example `.args(["test", +/// "--accept", "--", "--nocapture"])`. We then also need to pass +/// `--nocapture` to the outer test to forward that to the terminal. +/// /// We can write more docs if that would be helpful. For the moment one thing to /// be aware of: it seems the packages must have different names, or we'll see /// interference between the tests. @@ -20,6 +32,7 @@ use std::fs; use std::path::{Path, PathBuf}; use std::process::Command; +use console::style; use ignore::WalkBuilder; use insta::assert_snapshot; use itertools::Itertools; @@ -67,18 +80,26 @@ fn target_dir() -> PathBuf { } fn assert_success(output: &std::process::Output) { - // Print stderr. Cargo test hides this when tests are successful, but if a - // test successfully exectues a command but then fails (e.g. on a snapshot), - // we would otherwise lose any output from the command such as `dbg!` - // statements. - eprint!("{}", String::from_utf8_lossy(&output.stderr)); - eprint!("{}", String::from_utf8_lossy(&output.stdout)); + // Color the inner output so we can tell what's coming from there vs our own + // output + + // Should we also do something like indent them? Or add a prefix? + let stdout = format!("{}", style(String::from_utf8_lossy(&output.stdout)).green()); + let stderr = format!("{}", style(String::from_utf8_lossy(&output.stderr)).red()); + assert!( output.status.success(), "Tests failed: {}\n{}", - String::from_utf8_lossy(&output.stdout), - String::from_utf8_lossy(&output.stderr) + stdout, + stderr ); + + // Print stdout & stderr. Cargo test hides this when tests are successful, but if an + // test function in this file successfully executes an inner test command + // but then fails (e.g. on a snapshot), we would otherwise lose any output + // from that inner command, such as `dbg!` statements. + eprint!("{}", stdout); + eprint!("{}", stderr); } fn assert_failure(output: &std::process::Output) { @@ -245,7 +266,7 @@ fn test_json_snapshot() { let output = test_project .cmd() - .args(["test", "--accept"]) + .args(["test", "--accept", "--", "--nocapture"]) .output() .unwrap(); From 851b16f99a6ecbf14723c7393aa1f59f53f4c573 Mon Sep 17 00:00:00 2001 From: Frank Elsinga Date: Fri, 27 Sep 2024 00:02:24 +0200 Subject: [PATCH 19/41] chore: fixed `warn(elided_named_lifetimes)` (#620) This fixes an instance of [`warn(elided_named_lifetimes)`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/builtin/static.ELIDED_NAMED_LIFETIMES.html) which should be uncontroversial. ``` warning: elided lifetime has a name --> insta/src/content/yaml/vendored/emitter.rs:106:51 | 105 | impl<'a> YamlEmitter<'a> { | -- lifetime `'a` declared here 106 | pub fn new(writer: &'a mut dyn fmt::Write) -> YamlEmitter { | ^^^^^^^^^^^ this elided lifetime gets resolved as `'a` | = note: `#[warn(elided_named_lifetimes)]` on by default warning: `insta` (lib) generated 1 warning ``` I know that this currently only "affects" the nightly compliler. Fixing in case this becomes a warn by default lint in the future ^^ Docs on this lint: - https://doc.rust-lang.org/nightly/rustc/lints/listing/warn-by-default.html#elided-named-lifetimes - https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/builtin/static.ELIDED_NAMED_LIFETIMES.html --- insta/src/content/yaml/vendored/emitter.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/insta/src/content/yaml/vendored/emitter.rs b/insta/src/content/yaml/vendored/emitter.rs index d04b4fa9..eddcaae4 100644 --- a/insta/src/content/yaml/vendored/emitter.rs +++ b/insta/src/content/yaml/vendored/emitter.rs @@ -103,7 +103,7 @@ fn escape_str(wr: &mut dyn fmt::Write, v: &str) -> Result<(), fmt::Error> { } impl<'a> YamlEmitter<'a> { - pub fn new(writer: &'a mut dyn fmt::Write) -> YamlEmitter { + pub fn new(writer: &'a mut dyn fmt::Write) -> YamlEmitter<'a> { YamlEmitter { writer, best_indent: 2, From c8beea3328edca14ca0a26af1f2c5a39c4b19ae3 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Fri, 27 Sep 2024 00:14:09 -0700 Subject: [PATCH 20/41] Move a few functions onto context object --- insta/src/runtime.rs | 208 +++++++++++++++++++++---------------------- 1 file changed, 104 insertions(+), 104 deletions(-) diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index f3966220..2040c945 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -46,6 +46,17 @@ macro_rules! elog { writeln!(std::io::stderr(), $($arg)*).ok(); }) } +#[cfg(feature = "glob")] +macro_rules! print_or_panic { + ($fail_fast:expr, $($tokens:tt)*) => {{ + if (!$fail_fast) { + eprintln!($($tokens)*); + eprintln!(); + } else { + panic!($($tokens)*); + } + }} +} /// Special marker to use an automatic name. /// @@ -515,127 +526,116 @@ impl<'a> SnapshotAssertionContext<'a> { Ok(snapshot_update) } -} -fn prevent_inline_duplicate(function_name: &str, assertion_file: &str, assertion_line: u32) { - let key = format!("{}|{}|{}", function_name, assertion_file, assertion_line); - let mut set = INLINE_DUPLICATES.lock().unwrap(); - if set.contains(&key) { - // drop the lock so we don't poison it - drop(set); - panic!( - "Insta does not allow inline snapshot assertions in loops. \ - Wrap your assertions in allow_duplicates! to change this." + /// This prints the information about the snapshot + fn print_snapshot_info(&self, new_snapshot: &Snapshot) { + let mut printer = SnapshotPrinter::new( + self.cargo_workspace.as_path(), + self.old_snapshot.as_ref(), + new_snapshot, ); - } - set.insert(key); -} - -/// This prints the information about the snapshot -fn print_snapshot_info(ctx: &SnapshotAssertionContext, new_snapshot: &Snapshot) { - let mut printer = SnapshotPrinter::new( - ctx.cargo_workspace.as_path(), - ctx.old_snapshot.as_ref(), - new_snapshot, - ); - printer.set_line(Some(ctx.assertion_line)); - printer.set_snapshot_file(ctx.snapshot_file.as_deref()); - printer.set_title(Some("Snapshot Summary")); - printer.set_show_info(true); - match ctx.tool_config.output_behavior() { - OutputBehavior::Summary => { - printer.print(); - } - OutputBehavior::Diff => { - printer.set_show_diff(true); - printer.print(); + printer.set_line(Some(self.assertion_line)); + printer.set_snapshot_file(self.snapshot_file.as_deref()); + printer.set_title(Some("Snapshot Summary")); + printer.set_show_info(true); + match self.tool_config.output_behavior() { + OutputBehavior::Summary => { + printer.print(); + } + OutputBehavior::Diff => { + printer.set_show_diff(true); + printer.print(); + } + _ => {} } - _ => {} } -} -#[cfg(feature = "glob")] -macro_rules! print_or_panic { - ($fail_fast:expr, $($tokens:tt)*) => {{ - if (!$fail_fast) { - eprintln!($($tokens)*); - eprintln!(); - } else { - panic!($($tokens)*); - } - }} -} - -/// Finalizes the assertion based on the update result. -fn finalize_assertion(ctx: &SnapshotAssertionContext, update_result: SnapshotUpdateBehavior) { - // if we are in glob mode, we want to adjust the finalization - // so that we do not show the hints immediately. - let fail_fast = { - #[cfg(feature = "glob")] - { - if let Some(top) = crate::glob::GLOB_STACK.lock().unwrap().last() { - top.fail_fast - } else { + /// Finalizes the assertion when the snapshot comparison fails, potentially + /// panicking to fail the test + fn finalize(&self, update_result: SnapshotUpdateBehavior) { + // if we are in glob mode, we want to adjust the finalization + // so that we do not show the hints immediately. + let fail_fast = { + #[cfg(feature = "glob")] + { + if let Some(top) = crate::glob::GLOB_STACK.lock().unwrap().last() { + top.fail_fast + } else { + true + } + } + #[cfg(not(feature = "glob"))] + { true } - } - #[cfg(not(feature = "glob"))] + }; + + if fail_fast + && update_result == SnapshotUpdateBehavior::NewFile + && self.tool_config.output_behavior() != OutputBehavior::Nothing + && !self.is_doctest { - true + println!( + "{hint}", + hint = style("To update snapshots run `cargo insta review`").dim(), + ); } - }; - if fail_fast - && update_result == SnapshotUpdateBehavior::NewFile - && ctx.tool_config.output_behavior() != OutputBehavior::Nothing - && !ctx.is_doctest - { - println!( - "{hint}", - hint = style("To update snapshots run `cargo insta review`").dim(), - ); - } + if update_result != SnapshotUpdateBehavior::InPlace && !self.tool_config.force_pass() { + if fail_fast && self.tool_config.output_behavior() != OutputBehavior::Nothing { + let msg = if env::var("INSTA_CARGO_INSTA") == Ok("1".to_string()) { + "Stopped on the first failure." + } else { + "Stopped on the first failure. Run `cargo insta test` to run all snapshots." + }; + println!("{hint}", hint = style(msg).dim(),); + } - if update_result != SnapshotUpdateBehavior::InPlace && !ctx.tool_config.force_pass() { - if fail_fast && ctx.tool_config.output_behavior() != OutputBehavior::Nothing { - let msg = if env::var("INSTA_CARGO_INSTA") == Ok("1".to_string()) { - "Stopped on the first failure." - } else { - "Stopped on the first failure. Run `cargo insta test` to run all snapshots." - }; - println!("{hint}", hint = style(msg).dim(),); - } + // if we are in glob mode, count the failures and print the + // errors instead of panicking. The glob will then panic at + // the end. + #[cfg(feature = "glob")] + { + let mut stack = crate::glob::GLOB_STACK.lock().unwrap(); + if let Some(glob_collector) = stack.last_mut() { + glob_collector.failed += 1; + if update_result == SnapshotUpdateBehavior::NewFile + && self.tool_config.output_behavior() != OutputBehavior::Nothing + { + glob_collector.show_insta_hint = true; + } - // if we are in glob mode, count the failures and print the - // errors instead of panicking. The glob will then panic at - // the end. - #[cfg(feature = "glob")] - { - let mut stack = crate::glob::GLOB_STACK.lock().unwrap(); - if let Some(glob_collector) = stack.last_mut() { - glob_collector.failed += 1; - if update_result == SnapshotUpdateBehavior::NewFile - && ctx.tool_config.output_behavior() != OutputBehavior::Nothing - { - glob_collector.show_insta_hint = true; + print_or_panic!( + fail_fast, + "snapshot assertion from glob for '{}' failed in line {}", + self.snapshot_name.as_deref().unwrap_or("unnamed snapshot"), + self.assertion_line + ); + return; } - - print_or_panic!( - fail_fast, - "snapshot assertion from glob for '{}' failed in line {}", - ctx.snapshot_name.as_deref().unwrap_or("unnamed snapshot"), - ctx.assertion_line - ); - return; } + + panic!( + "snapshot assertion for '{}' failed in line {}", + self.snapshot_name.as_deref().unwrap_or("unnamed snapshot"), + self.assertion_line + ); } + } +} +fn prevent_inline_duplicate(function_name: &str, assertion_file: &str, assertion_line: u32) { + let key = format!("{}|{}|{}", function_name, assertion_file, assertion_line); + let mut set = INLINE_DUPLICATES.lock().unwrap(); + if set.contains(&key) { + // drop the lock so we don't poison it + drop(set); panic!( - "snapshot assertion for '{}' failed in line {}", - ctx.snapshot_name.as_deref().unwrap_or("unnamed snapshot"), - ctx.assertion_line + "Insta does not allow inline snapshot assertions in loops. \ + Wrap your assertions in allow_duplicates! to change this." ); } + set.insert(key); } fn record_snapshot_duplicate( @@ -797,9 +797,9 @@ pub fn assert_snapshot( } // otherwise print information and update snapshots. } else { - print_snapshot_info(&ctx, &new_snapshot); + ctx.print_snapshot_info(&new_snapshot); let update_result = ctx.update_snapshot(new_snapshot)?; - finalize_assertion(&ctx, update_result); + ctx.finalize(update_result); } Ok(()) From 6aaae63330835e40ac4b22606751cde23694e7b3 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Fri, 27 Sep 2024 15:01:53 +0200 Subject: [PATCH 21/41] Add docstrings to integration tests for binary snapshots --- cargo-insta/tests/main.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index d1a1a306..ee890a72 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -1186,6 +1186,7 @@ fn test_hashtag_escape() { "####); } +/// A pending binary snapshot should have a binary file with the passed extension alongside it. #[test] fn test_binary_pending() { let test_project = TestFiles::new() @@ -1233,6 +1234,7 @@ fn test_binary_snapshot() { "); } +/// An accepted binary snapshot should have a binary file with the passed extension alongside it. #[test] fn test_binary_accept() { let test_project = TestFiles::new() @@ -1284,6 +1286,9 @@ fn test_binary_snapshot() { "); } +/// Changing the extension passed to the `assert_binary_snapshot` macro should create a new pending +/// snapshot with a binary file with the new extension alongside it and once approved the old binary +/// file with the old extension should be deleted. #[test] fn test_binary_change_extension() { let test_project = TestFiles::new() @@ -1374,6 +1379,8 @@ fn test_binary_snapshot() { "); } +/// An assert with a pending binary snapshot should have both the metadata file and the binary file +/// deleted when the assert is removed and the tests are re-run. #[test] fn test_binary_pending_snapshot_removal() { let test_project = TestFiles::new() @@ -1425,6 +1432,8 @@ fn test_binary_snapshot() { "); } +/// Replacing a text snapshot with binary one should work and simply replace the text snapshot file +/// with the new metadata file and a new binary snapshot file alongside it. #[test] fn test_change_text_to_binary() { let test_project = TestFiles::new() @@ -1506,6 +1515,8 @@ fn test() { "); } +/// When changing a snapshot from a binary to a text snapshot the previous binary file should be +/// gone after having approved the the binary snapshot. #[test] fn test_change_binary_to_text() { let test_project = TestFiles::new() From 1b4f7ec471fc50356a222bf7b24dce3c9f5b4722 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Fri, 27 Sep 2024 18:40:55 +0200 Subject: [PATCH 22/41] Remove redundant extension and extra struct for binary snapshots --- cargo-insta/src/cli.rs | 15 ++++++----- cargo-insta/src/container.rs | 13 +++++---- insta/src/output.rs | 49 ++++++++++++++++++++------------- insta/src/runtime.rs | 30 ++++++++++++--------- insta/src/snapshot.rs | 52 +++++++++++++++--------------------- 5 files changed, 83 insertions(+), 76 deletions(-) diff --git a/cargo-insta/src/cli.rs b/cargo-insta/src/cli.rs index 805a9cc8..3ea9ab7e 100644 --- a/cargo-insta/src/cli.rs +++ b/cargo-insta/src/cli.rs @@ -6,7 +6,6 @@ use std::{env, fs}; use std::{io, process}; use console::{set_colors_enabled, style, Key, Term}; -use insta::internals::SnapshotContents; use insta::Snapshot; use insta::_cargo_insta_support::{ is_ci, SnapshotPrinter, SnapshotUpdate, TestRunner, ToolConfig, UnreferencedSnapshots, @@ -354,14 +353,16 @@ fn query_snapshot( break; } Key::Char('o') => { - if let Some(SnapshotContents::Binary(contents)) = old.map(|o| o.contents()) { - open::that_detached(contents.build_path(snapshot_file.unwrap()))?; + if let Some(old) = old { + if let Some(path) = old.build_binary_path(snapshot_file.unwrap()) { + open::that_detached(path)?; + } } - if let SnapshotContents::Binary(contents) = new.contents() { - open::that_detached( - contents.build_path(snapshot_file.unwrap().with_extension("snap.new")), - )?; + if let Some(path) = + new.build_binary_path(snapshot_file.unwrap().with_extension("snap.new")) + { + open::that_detached(path)?; } // there's no break here because there's no need to re-output anything diff --git a/cargo-insta/src/container.rs b/cargo-insta/src/container.rs index 10255a81..1764cfac 100644 --- a/cargo-insta/src/container.rs +++ b/cargo-insta/src/container.rs @@ -2,7 +2,6 @@ use std::error::Error; use std::fs; use std::path::{Path, PathBuf}; -use insta::internals::SnapshotContents; use insta::Snapshot; pub(crate) use insta::TextSnapshotKind; use insta::_cargo_insta_support::{ContentError, PendingInlineSnapshot}; @@ -220,13 +219,13 @@ impl SnapshotContainer { try_removing_snapshot(&self.pending_path); if let Some(ref old) = snapshot.old { - if let SnapshotContents::Binary(contents) = old.contents() { - try_removing_snapshot(&contents.build_path(&self.target_path)); + if let Some(path) = old.build_binary_path(&self.target_path) { + try_removing_snapshot(&path); } } - if let SnapshotContents::Binary(contents) = snapshot.new.contents() { - try_removing_snapshot(&contents.build_path(&self.pending_path)); + if let Some(path) = snapshot.new.build_binary_path(&self.pending_path) { + try_removing_snapshot(&path); } // We save at the end because we might write a binary file into the same @@ -236,8 +235,8 @@ impl SnapshotContainer { Operation::Reject => { try_removing_snapshot(&self.pending_path); - if let SnapshotContents::Binary(contents) = snapshot.new.contents() { - try_removing_snapshot(&contents.build_path(&self.pending_path)); + if let Some(path) = snapshot.new.build_binary_path(&self.pending_path) { + try_removing_snapshot(&path); } } Operation::Skip => {} diff --git a/insta/src/output.rs b/insta/src/output.rs index 24cb2d0c..76d22c86 100644 --- a/insta/src/output.rs +++ b/insta/src/output.rs @@ -119,12 +119,16 @@ impl<'a> SnapshotPrinter<'a> { } println!("──────┴{:─^1$}", "", width.saturating_sub(7)); } - SnapshotContents::Binary(new_contents) => { + SnapshotContents::Binary(_) => { println!( "{}", encode_file_link_escape( - &new_contents - .build_path(self.snapshot_file.unwrap().with_extension("snap.new")) + &self + .new_snapshot + .build_binary_path( + self.snapshot_file.unwrap().with_extension("snap.new") + ) + .unwrap() ) ); } @@ -139,30 +143,37 @@ impl<'a> SnapshotPrinter<'a> { self.print_info(); } - let old_contents = self.old_snapshot.as_ref().map(|o| o.contents()); - let new_contents = self.new_snapshot.contents(); - - if let Some(SnapshotContents::Binary(contents)) = old_contents { - println!( - "{}", - style(format_args!( - "-{}: {}", - self.old_snapshot_hint, - encode_file_link_escape(&contents.build_path(self.snapshot_file.unwrap())), - )) - .red() - ); + if let Some(old_snapshot) = self.old_snapshot { + if old_snapshot.contents().is_binary() { + println!( + "{}", + style(format_args!( + "-{}: {}", + self.old_snapshot_hint, + encode_file_link_escape( + &old_snapshot + .build_binary_path(self.snapshot_file.unwrap()) + .unwrap() + ), + )) + .red() + ); + } } - if let SnapshotContents::Binary(contents) = new_contents { + if self.new_snapshot.contents().is_binary() { println!( "{}", style(format_args!( "+{}: {}", self.new_snapshot_hint, encode_file_link_escape( - &contents - .build_path(self.snapshot_file.unwrap().with_extension("snap.new")) + &self + .new_snapshot + .build_binary_path( + self.snapshot_file.unwrap().with_extension("snap.new") + ) + .unwrap() ), )) .green() diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 171fe1da..088f8ab2 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -11,8 +11,7 @@ use std::{borrow::Cow, env}; use crate::settings::Settings; use crate::snapshot::{ - BinarySnapshotContents, MetaData, PendingInlineSnapshot, Snapshot, SnapshotContents, - SnapshotType, TextSnapshotContents, + MetaData, PendingInlineSnapshot, Snapshot, SnapshotContents, SnapshotType, TextSnapshotContents, }; use crate::utils::{path_to_storage, style}; use crate::{env::get_tool_config, output::SnapshotPrinter}; @@ -256,6 +255,7 @@ struct SnapshotAssertionContext<'a> { assertion_file: &'a str, assertion_line: u32, is_doctest: bool, + snapshot_type: SnapshotType, } impl<'a> SnapshotAssertionContext<'a> { @@ -340,6 +340,13 @@ impl<'a> SnapshotAssertionContext<'a> { } }; + let snapshot_type = match new_snapshot_value { + SnapshotValue::FileText { .. } | SnapshotValue::InlineText { .. } => SnapshotType::Text, + &SnapshotValue::Binary { extension, .. } => SnapshotType::Binary { + extension: extension.to_string(), + }, + }; + Ok(SnapshotAssertionContext { tool_config, workspace, @@ -352,6 +359,7 @@ impl<'a> SnapshotAssertionContext<'a> { assertion_line, duplication_key, is_doctest, + snapshot_type, }) } @@ -364,6 +372,11 @@ impl<'a> SnapshotAssertionContext<'a> { /// Creates the new snapshot from input values. pub fn new_snapshot(&self, contents: SnapshotContents, expr: &str) -> Snapshot { + assert_eq!( + contents.is_binary(), + matches!(self.snapshot_type, SnapshotType::Binary { .. }) + ); + Snapshot::from_components( self.module_path.replace("::", "__"), self.snapshot_name.as_ref().map(|x| x.to_string()), @@ -381,12 +394,7 @@ impl<'a> SnapshotAssertionContext<'a> { .input_file() .and_then(|x| self.localize_path(x)) .map(|x| path_to_storage(&x)), - snapshot_type: match contents { - SnapshotContents::Text(_) => SnapshotType::Text, - SnapshotContents::Binary(ref contents) => SnapshotType::Binary { - extension: contents.extension.clone(), - }, - }, + snapshot_type: self.snapshot_type.clone(), }), contents, ) @@ -735,11 +743,7 @@ pub fn assert_snapshot( "file extensions starting with 'new.' are not allowed", ); - BinarySnapshotContents { - contents: Rc::new(content), - extension: extension.to_string(), - } - .into() + SnapshotContents::Binary(Rc::new(content)) } }; diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index 41e1d1c2..21d3a889 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -410,11 +410,7 @@ impl Snapshot { let path = build_binary_path(extension, p); let contents = fs::read(path)?; - BinarySnapshotContents { - contents: Rc::new(contents), - extension: extension.clone(), - } - .into() + SnapshotContents::Binary(Rc::new(contents)) } }; @@ -522,6 +518,8 @@ impl Snapshot { /// Snapshot contents match another snapshot's. pub fn matches(&self, other: &Self) -> bool { self.contents() == other.contents() + // For binary snapshots the extension also need to be the same: + && self.metadata.snapshot_type == other.metadata.snapshot_type } /// Both the exact snapshot contents and the persisted metadata match another snapshot's. @@ -540,7 +538,10 @@ impl Snapshot { TextSnapshotKind::Inline => contents_match_exact, } } - (SnapshotContents::Binary(a), SnapshotContents::Binary(b)) => a == b, + (SnapshotContents::Binary(a), SnapshotContents::Binary(b)) => { + // For binary snapshots the extension also need to be the same: + a == b && self.metadata.snapshot_type == other.metadata.snapshot_type + } _ => false, } } @@ -574,12 +575,20 @@ impl Snapshot { fs::write(path, serialized_snapshot)?; if let SnapshotContents::Binary(ref contents) = self.snapshot { - fs::write(contents.build_path(path), &*contents.contents)?; + fs::write(self.build_binary_path(path).unwrap(), &**contents)?; } Ok(()) } + pub fn build_binary_path(&self, path: impl Into) -> Option { + if let SnapshotType::Binary { ref extension } = self.metadata.snapshot_type { + Some(build_binary_path(extension, path)) + } else { + None + } + } + /// Saves the snapshot. /// /// Returns `true` if the snapshot was saved. This will return `false` if there @@ -604,7 +613,12 @@ impl Snapshot { #[derive(Debug, Clone)] pub enum SnapshotContents { Text(TextSnapshotContents), - Binary(BinarySnapshotContents), + + // This is in an `Rc` because we need to be able to clone this struct cheaply and the contents + // of the `Vec` could be rather large. The reason it's not an `Rc<[u8]>` is because creating one + // of those would require re-allocating because of the additional size needed for the reference + // count. + Binary(Rc>), } // Could be Cow, but I think limited savings @@ -614,28 +628,12 @@ pub struct TextSnapshotContents { pub kind: TextSnapshotKind, } -#[derive(Debug, PartialEq, Eq, Clone)] -pub struct BinarySnapshotContents { - // This is in an `Rc` because we need to be able to clone this struct cheaply and the contents - // of the `Vec` could be rather large. The reason it's not an `Rc<[u8]>` is because creating one - // of those would require re-allocating because of the additional size needed for the reference - // count. - pub contents: Rc>, - pub extension: String, -} - impl From for SnapshotContents { fn from(value: TextSnapshotContents) -> Self { SnapshotContents::Text(value) } } -impl From for SnapshotContents { - fn from(value: BinarySnapshotContents) -> Self { - SnapshotContents::Binary(value) - } -} - impl SnapshotContents { pub fn is_binary(&self) -> bool { matches!(self, SnapshotContents::Binary(_)) @@ -742,12 +740,6 @@ impl TextSnapshotContents { } } -impl BinarySnapshotContents { - pub fn build_path(&self, path: impl Into) -> PathBuf { - build_binary_path(&self.extension, path) - } -} - impl fmt::Display for TextSnapshotContents { /// Returns the snapshot contents as a normalized string (for example, /// removing surrounding whitespace) From bd24f02fc028b07cd0b4263cdc0005d3794a3f7d Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Mon, 30 Sep 2024 21:09:53 +0200 Subject: [PATCH 23/41] Change binary snapshot macro syntax The format is now "name.extension" in one string. --- cargo-insta/tests/main.rs | 14 +++++++------- insta/src/lib.rs | 3 ++- insta/src/macros.rs | 18 +++++++----------- insta/src/runtime.rs | 30 ++++++++++++++++++++++++++++++ insta/tests/test_binary.rs | 18 +++++++++++++----- 5 files changed, 59 insertions(+), 24 deletions(-) diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index 97ce5c44..e67c6dea 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -1208,7 +1208,7 @@ insta = { path = '$PROJECT_PATH' } r#" #[test] fn test_binary_snapshot() { - insta::assert_binary_snapshot!("txt", b"test".to_vec()); + insta::assert_binary_snapshot!(".txt", b"test".to_vec()); } "# .to_string(), @@ -1256,7 +1256,7 @@ insta = { path = '$PROJECT_PATH' } r#" #[test] fn test_binary_snapshot() { - insta::assert_binary_snapshot!("txt", b"test".to_vec()); + insta::assert_binary_snapshot!(".txt", b"test".to_vec()); } "# .to_string(), @@ -1310,7 +1310,7 @@ insta = { path = '$PROJECT_PATH' } r#" #[test] fn test_binary_snapshot() { - insta::assert_binary_snapshot!("txt", b"test".to_vec()); + insta::assert_binary_snapshot!(".txt", b"test".to_vec()); } "# .to_string(), @@ -1330,7 +1330,7 @@ fn test_binary_snapshot() { r#" #[test] fn test_binary_snapshot() { - insta::assert_binary_snapshot!("json", b"test".to_vec()); + insta::assert_binary_snapshot!(".json", b"test".to_vec()); } "# .to_string(), @@ -1402,7 +1402,7 @@ insta = { path = '$PROJECT_PATH' } r#" #[test] fn test_binary_snapshot() { - insta::assert_binary_snapshot!("txt", b"test".to_vec()); + insta::assert_binary_snapshot!(".txt", b"test".to_vec()); } "# .to_string(), @@ -1487,7 +1487,7 @@ fn test() { r#" #[test] fn test() { - insta::assert_binary_snapshot!("txt", b"test".to_vec()); + insta::assert_binary_snapshot!(".txt", b"test".to_vec()); } "# .to_string(), @@ -1538,7 +1538,7 @@ insta = { path = '$PROJECT_PATH' } r#" #[test] fn test() { - insta::assert_binary_snapshot!("json", "some_name", b"{}".to_vec()); + insta::assert_binary_snapshot!("some_name.json", b"{}".to_vec()); } "# .to_string(), diff --git a/insta/src/lib.rs b/insta/src/lib.rs index b6b13ee7..7dbd5251 100644 --- a/insta/src/lib.rs +++ b/insta/src/lib.rs @@ -342,7 +342,8 @@ pub mod _macro_support { pub use crate::content::Content; pub use crate::env::get_cargo_workspace; pub use crate::runtime::{ - assert_snapshot, with_allow_duplicates, AutoName, SnapshotName, SnapshotValue, + assert_snapshot, with_allow_duplicates, AutoName, BinarySnapshotValue, SnapshotName, + SnapshotValue, }; #[cfg(feature = "serde")] diff --git a/insta/src/macros.rs b/insta/src/macros.rs index aebc832d..c0a47b54 100644 --- a/insta/src/macros.rs +++ b/insta/src/macros.rs @@ -385,21 +385,17 @@ macro_rules! _assert_snapshot_base { #[macro_export] macro_rules! assert_binary_snapshot { - ($extension:expr, $value:expr $(,)?) => { - $crate::assert_binary_snapshot!($extension, $crate::_macro_support::AutoName, $value); + ($name_and_extension:expr, $value:expr $(,)?) => { + $crate::assert_binary_snapshot!($name_and_extension, $value, stringify!($value)); }; - ($extension:expr, $name:expr, $value:expr $(,)?) => { - $crate::assert_binary_snapshot!($extension, $name, $value, stringify!($value)); - }; - - ($extension:expr, $name:expr, $value:expr, $debug_expr:expr $(,)?) => { + ($name_and_extension:expr, $value:expr, $debug_expr:expr $(,)?) => { $crate::_macro_support::assert_snapshot( - $crate::_macro_support::SnapshotValue::Binary { - name: $name.into(), + $crate::_macro_support::BinarySnapshotValue { + name_and_extension: $name_and_extension, content: $value, - extension: $extension, - }, + } + .into(), $crate::_get_workspace_root!().as_path(), $crate::_function_name!(), module_path!(), diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index df64e5b6..301b266d 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -68,6 +68,11 @@ pub struct AutoName; /// The name of a snapshot, from which the path is derived. pub struct SnapshotName<'a>(pub Option>); +pub struct BinarySnapshotValue<'a> { + pub name_and_extension: &'a str, + pub content: Vec, +} + pub enum SnapshotValue<'a> { /// A text snapshot that gets stored along with the metadata in the same file. FileText { @@ -98,6 +103,31 @@ pub enum SnapshotValue<'a> { }, } +impl<'a> From> for SnapshotValue<'a> { + fn from( + BinarySnapshotValue { + name_and_extension, + content, + }: BinarySnapshotValue<'a>, + ) -> Self { + let (name, extension) = name_and_extension + .split_once('.') + .expect("the snapshot name and extension should be in the format \"name.extension\""); + + let name = SnapshotName(if name == "" { + None + } else { + Some(Cow::Borrowed(name)) + }); + + SnapshotValue::Binary { + name, + extension, + content, + } + } +} + impl From for SnapshotName<'static> { fn from(_value: AutoName) -> SnapshotName<'static> { SnapshotName(None) diff --git a/insta/tests/test_binary.rs b/insta/tests/test_binary.rs index 9e0346ac..9927b5dd 100644 --- a/insta/tests/test_binary.rs +++ b/insta/tests/test_binary.rs @@ -1,26 +1,34 @@ #[test] fn test_binary_snapshot() { - insta::assert_binary_snapshot!("txt", b"test".to_vec()); + insta::assert_binary_snapshot!(".txt", b"test".to_vec()); } #[test] #[should_panic(expected = "'.new' is not allowed as a file extension")] fn test_new_extension() { - insta::assert_binary_snapshot!("new", b"test".to_vec()); + insta::assert_binary_snapshot!(".new", b"test".to_vec()); +} + +#[test] +#[should_panic( + expected = "the snapshot name and extension should be in the format \"name.extension\"" +)] +fn test_malformed_name_and_extension() { + insta::assert_binary_snapshot!("test", b"test".to_vec()); } #[test] #[should_panic(expected = "file extensions starting with 'new.' are not allowed")] fn test_extension_starting_with_new() { - insta::assert_binary_snapshot!("new.gz", b"test".to_vec()); + insta::assert_binary_snapshot!(".new.gz", b"test".to_vec()); } #[test] fn test_multipart_extension() { - insta::assert_binary_snapshot!("tar.gz", b"test".to_vec()); + insta::assert_binary_snapshot!(".tar.gz", b"test".to_vec()); } #[test] fn test_named() { - insta::assert_binary_snapshot!("json", "name", b"null".to_vec()); + insta::assert_binary_snapshot!("name.json", b"null".to_vec()); } From 9a80d873604cc06eb6569c6be8d08cbdd420582a Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Mon, 30 Sep 2024 23:17:47 -0700 Subject: [PATCH 24/41] --- cargo-insta/tests/main.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index 5c740252..f31fe2bb 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -1823,6 +1823,7 @@ fn test_hello() { --- source: "../tests/lib.rs" expression: hello() + snapshot_type: text --- Hello, world! "#); From d015be10da044c058541ecb66aacdb24b3e4efd2 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Tue, 1 Oct 2024 14:32:11 +0200 Subject: [PATCH 25/41] Fix clippy warning --- insta/src/runtime.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 2d4aa6eb..92af5d26 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -114,7 +114,7 @@ impl<'a> From> for SnapshotValue<'a> { .split_once('.') .expect("the snapshot name and extension should be in the format \"name.extension\""); - let name = SnapshotName(if name == "" { + let name = SnapshotName(if name.is_empty() { None } else { Some(Cow::Borrowed(name)) From a501dfbbf6f91016a22160fe7b0bd6aafa0c3aef Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Tue, 1 Oct 2024 17:23:03 +0200 Subject: [PATCH 26/41] Change back snapshot macro internals This partially undoes 7d82e108. This means we're back to relying on `From` impls for two element tuples to make the macros work. --- insta/src/lib.rs | 2 +- insta/src/macros.rs | 44 +++++++++------------ insta/src/runtime.rs | 94 ++++++++++++++++++++++++++++---------------- 3 files changed, 79 insertions(+), 61 deletions(-) diff --git a/insta/src/lib.rs b/insta/src/lib.rs index 7dbd5251..410c271d 100644 --- a/insta/src/lib.rs +++ b/insta/src/lib.rs @@ -342,7 +342,7 @@ pub mod _macro_support { pub use crate::content::Content; pub use crate::env::get_cargo_workspace; pub use crate::runtime::{ - assert_snapshot, with_allow_duplicates, AutoName, BinarySnapshotValue, SnapshotName, + assert_snapshot, with_allow_duplicates, AutoName, BinarySnapshotValue, InlineValue, SnapshotValue, }; diff --git a/insta/src/macros.rs b/insta/src/macros.rs index c0a47b54..d2f2b09b 100644 --- a/insta/src/macros.rs +++ b/insta/src/macros.rs @@ -333,45 +333,37 @@ macro_rules! assert_compact_debug_snapshot { #[doc(hidden)] #[macro_export] macro_rules! _assert_snapshot_base { - // If there's an inline literal value, wrap the literal and value in a - // `SnapshotValue::InlineText`, call self. - (transform=$transform:expr, $value:expr $(, $debug_expr:expr)?, @$snapshot:literal $(,)?) => { + // If there's an inline literal value, wrap the literal in a + // `ReferenceValue::Inline`, call self. + (transform=$transform:expr, $($arg:expr),*, @$snapshot:literal $(,)?) => { $crate::_assert_snapshot_base!( + transform = $transform, #[allow(clippy::needless_raw_string_hashes)] - $crate::_macro_support::SnapshotValue::InlineText { - reference_content: $snapshot, - content: $transform(&$value).as_str(), - }, - $($debug_expr,)? - stringify!($value), + $crate::_macro_support::InlineValue($snapshot), + $($arg),* ) }; + // If there's no debug_expr, use the stringified value, call self. + (transform=$transform:expr, $name:expr, $value:expr $(,)?) => { + $crate::_assert_snapshot_base!(transform = $transform, $name, $value, stringify!($value)) + }; // If there's no name (and necessarily no debug expr), auto generate the // name, call self. (transform=$transform:expr, $value:expr $(,)?) => { $crate::_assert_snapshot_base!( - transform=$transform, + transform = $transform, $crate::_macro_support::AutoName, - $value, - ) - }; - // If there's a name, wrap the name and value in a SnapshotValue::FileText, call self. - (transform=$transform:expr, $name:expr, $value:expr $(, $debug_expr:expr)? $(,)?) => { - $crate::_assert_snapshot_base!( - $crate::_macro_support::SnapshotValue::FileText { - name: $name.into(), - content: $transform(&$value).as_str(), - }, - $($debug_expr,)? - stringify!($value), + $value ) }; // The main macro body — every call to this macro should end up here. - // The extra value gets ignored here because above we can't only pass $value if there's no - // $debug_expr due to macro_rules not having anything like an else. - ($snapshot_value:expr, $debug_expr:expr $(, $_:expr)? $(,)?) => { + (transform=$transform:expr, $name:expr, $value:expr, $debug_expr:expr $(,)?) => { $crate::_macro_support::assert_snapshot( - $snapshot_value, + ( + $name, + #[allow(clippy::redundant_closure_call)] + $transform(&$value).as_str(), + ).into(), $crate::_get_workspace_root!().as_path(), $crate::_function_name!(), module_path!(), diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 92af5d26..bb5b6aec 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -65,8 +65,10 @@ macro_rules! print_or_panic { #[derive(Debug)] pub struct AutoName; +pub struct InlineValue<'a>(pub &'a str); + /// The name of a snapshot, from which the path is derived. -pub struct SnapshotName<'a>(pub Option>); +type SnapshotName<'a> = Option>; pub struct BinarySnapshotValue<'a> { pub name_and_extension: &'a str, @@ -103,6 +105,60 @@ pub enum SnapshotValue<'a> { }, } +impl<'a> From<(AutoName, &'a str)> for SnapshotValue<'a> { + fn from((_, content): (AutoName, &'a str)) -> Self { + SnapshotValue::FileText { + name: None, + content, + } + } +} + +impl<'a> From<(Option, &'a str)> for SnapshotValue<'a> { + fn from((name, content): (Option, &'a str)) -> Self { + SnapshotValue::FileText { + name: name.map(Cow::Owned), + content, + } + } +} + +impl<'a> From<(String, &'a str)> for SnapshotValue<'a> { + fn from((name, content): (String, &'a str)) -> Self { + SnapshotValue::FileText { + name: Some(Cow::Owned(name)), + content, + } + } +} + +impl<'a> From<(Option<&'a str>, &'a str)> for SnapshotValue<'a> { + fn from((name, content): (Option<&'a str>, &'a str)) -> Self { + SnapshotValue::FileText { + name: name.map(Cow::Borrowed), + content, + } + } +} + +impl<'a> From<(&'a str, &'a str)> for SnapshotValue<'a> { + fn from((name, content): (&'a str, &'a str)) -> Self { + SnapshotValue::FileText { + name: Some(Cow::Borrowed(name)), + content, + } + } +} + +impl<'a> From<(InlineValue<'a>, &'a str)> for SnapshotValue<'a> { + fn from((InlineValue(reference_content), content): (InlineValue<'a>, &'a str)) -> Self { + SnapshotValue::InlineText { + reference_content, + content, + } + } +} + impl<'a> From> for SnapshotValue<'a> { fn from( BinarySnapshotValue { @@ -114,11 +170,11 @@ impl<'a> From> for SnapshotValue<'a> { .split_once('.') .expect("the snapshot name and extension should be in the format \"name.extension\""); - let name = SnapshotName(if name.is_empty() { + let name = if name.is_empty() { None } else { Some(Cow::Borrowed(name)) - }); + }; SnapshotValue::Binary { name, @@ -128,36 +184,6 @@ impl<'a> From> for SnapshotValue<'a> { } } -impl From for SnapshotName<'static> { - fn from(_value: AutoName) -> SnapshotName<'static> { - SnapshotName(None) - } -} - -impl From for SnapshotName<'static> { - fn from(value: String) -> SnapshotName<'static> { - SnapshotName(Some(Cow::Owned(value))) - } -} - -impl From> for SnapshotName<'static> { - fn from(value: Option) -> Self { - SnapshotName(value.map(Cow::Owned)) - } -} - -impl<'a> From<&'a str> for SnapshotName<'a> { - fn from(value: &'a str) -> SnapshotName<'a> { - SnapshotName(Some(Cow::Borrowed(value))) - } -} - -impl<'a> From> for SnapshotName<'a> { - fn from(value: Option<&'a str>) -> Self { - SnapshotName(value.map(Cow::Borrowed)) - } -} - fn is_doctest(function_name: &str) -> bool { function_name.starts_with("rust_out::main::_doctest") } @@ -305,7 +331,7 @@ impl<'a> SnapshotAssertionContext<'a> { match new_snapshot_value { SnapshotValue::FileText { name, .. } | SnapshotValue::Binary { name, .. } => { - let name = match &name.0 { + let name = match &name { Some(name) => add_suffix_to_snapshot_name(name.clone()), None => { if is_doctest { From 070471b54ab4ef4430289844ffbeecd3fa85c080 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Tue, 1 Oct 2024 10:28:03 -0700 Subject: [PATCH 27/41] --- cargo-insta/tests/main.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index 5213d8f7..f53b3519 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -1209,7 +1209,7 @@ fn test_binary_snapshot() { ) .create_project(); - let output = test_project.cmd().args(["test"]).output().unwrap(); + let output = test_project.insta_cmd().args(["test"]).output().unwrap(); assert_failure(&output); @@ -1258,7 +1258,7 @@ fn test_binary_snapshot() { .create_project(); let output = test_project - .cmd() + .insta_cmd() .args(["test", "--accept"]) .output() .unwrap(); @@ -1312,7 +1312,7 @@ fn test_binary_snapshot() { .create_project(); let output = test_project - .cmd() + .insta_cmd() .args(["test", "--accept"]) .output() .unwrap(); @@ -1330,7 +1330,7 @@ fn test_binary_snapshot() { .to_string(), ); - let output = test_project.cmd().args(["test"]).output().unwrap(); + let output = test_project.insta_cmd().args(["test"]).output().unwrap(); assert_failure(&output); @@ -1351,7 +1351,7 @@ fn test_binary_snapshot() { "); let output = test_project - .cmd() + .insta_cmd() .args(["test", "--accept"]) .output() .unwrap(); @@ -1403,13 +1403,13 @@ fn test_binary_snapshot() { ) .create_project(); - let output = test_project.cmd().args(["test"]).output().unwrap(); + let output = test_project.insta_cmd().args(["test"]).output().unwrap(); assert_failure(&output); test_project.update_file("src/main.rs", "".to_string()); - let output = test_project.cmd().args(["test"]).output().unwrap(); + let output = test_project.insta_cmd().args(["test"]).output().unwrap(); assert_success(&output); @@ -1457,7 +1457,7 @@ fn test() { .create_project(); let output = test_project - .cmd() + .insta_cmd() .args(["test", "--accept"]) .output() .unwrap(); @@ -1488,7 +1488,7 @@ fn test() { ); let output = test_project - .cmd() + .insta_cmd() .args(["test", "--accept"]) .output() .unwrap(); @@ -1540,7 +1540,7 @@ fn test() { .create_project(); let output = test_project - .cmd() + .insta_cmd() .args(["test", "--accept"]) .output() .unwrap(); @@ -1572,7 +1572,7 @@ fn test() { ); let output = test_project - .cmd() + .insta_cmd() .args(["test", "--accept"]) .output() .unwrap(); From 08be2641175ec77d21e56df4bf0b981ee65aa1a1 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Thu, 3 Oct 2024 16:49:38 +0200 Subject: [PATCH 28/41] Change snapshot_type to snapshot_kind in metadata Types are also renamed. --- cargo-insta/tests/main.rs | 6 +-- insta/src/runtime.rs | 14 +++--- insta/src/snapshot.rs | 44 +++++++++---------- .../test_binary__binary_snapshot.snap | 2 +- .../test_binary__multipart_extension.snap | 2 +- insta/tests/snapshots/test_binary__name.snap | 2 +- 6 files changed, 35 insertions(+), 35 deletions(-) diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index f53b3519..99d3a569 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -914,7 +914,7 @@ Hello, world! source: src/lib.rs -expression: +expression: "\"Hello, world!\"" - +snapshot_type: text + +snapshot_kind: text --- Hello, world! - @@ -930,7 +930,7 @@ Hello, world! source: src/lib.rs -expression: +expression: "\"Hello, world!\"" - +snapshot_type: text + +snapshot_kind: text --- Hello, world! - @@ -1824,7 +1824,7 @@ fn test_hello() { --- source: "../tests/lib.rs" expression: hello() - snapshot_type: text + snapshot_kind: text --- Hello, world! "#); diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index bb5b6aec..2f3c97ae 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -11,7 +11,7 @@ use std::{borrow::Cow, env}; use crate::settings::Settings; use crate::snapshot::{ - MetaData, PendingInlineSnapshot, Snapshot, SnapshotContents, SnapshotType, TextSnapshotContents, + MetaData, PendingInlineSnapshot, Snapshot, SnapshotContents, SnapshotKind, TextSnapshotContents, }; use crate::utils::{path_to_storage, style}; use crate::{env::get_tool_config, output::SnapshotPrinter}; @@ -309,7 +309,7 @@ struct SnapshotAssertionContext<'a> { assertion_file: &'a str, assertion_line: u32, is_doctest: bool, - snapshot_type: SnapshotType, + snapshot_kind: SnapshotKind, } impl<'a> SnapshotAssertionContext<'a> { @@ -394,8 +394,8 @@ impl<'a> SnapshotAssertionContext<'a> { }; let snapshot_type = match new_snapshot_value { - SnapshotValue::FileText { .. } | SnapshotValue::InlineText { .. } => SnapshotType::Text, - &SnapshotValue::Binary { extension, .. } => SnapshotType::Binary { + SnapshotValue::FileText { .. } | SnapshotValue::InlineText { .. } => SnapshotKind::Text, + &SnapshotValue::Binary { extension, .. } => SnapshotKind::Binary { extension: extension.to_string(), }, }; @@ -412,7 +412,7 @@ impl<'a> SnapshotAssertionContext<'a> { assertion_line, duplication_key, is_doctest, - snapshot_type, + snapshot_kind: snapshot_type, }) } @@ -427,7 +427,7 @@ impl<'a> SnapshotAssertionContext<'a> { pub fn new_snapshot(&self, contents: SnapshotContents, expr: &str) -> Snapshot { assert_eq!( contents.is_binary(), - matches!(self.snapshot_type, SnapshotType::Binary { .. }) + matches!(self.snapshot_kind, SnapshotKind::Binary { .. }) ); Snapshot::from_components( @@ -447,7 +447,7 @@ impl<'a> SnapshotAssertionContext<'a> { .input_file() .and_then(|x| self.localize_path(x)) .map(|x| path_to_storage(&x)), - snapshot_type: self.snapshot_type.clone(), + snapshot_kind: self.snapshot_kind.clone(), }), contents, ) diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index 9f830988..cee080c8 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -140,14 +140,14 @@ impl PendingInlineSnapshot { } #[derive(Debug, Clone, PartialEq)] -pub enum SnapshotType { +pub enum SnapshotKind { Text, Binary { extension: String }, } -impl Default for SnapshotType { +impl Default for SnapshotKind { fn default() -> Self { - SnapshotType::Text + SnapshotKind::Text } } @@ -168,7 +168,7 @@ pub struct MetaData { /// Reference to the input file. pub(crate) input_file: Option, /// The type of the snapshot (string or binary). - pub(crate) snapshot_type: SnapshotType, + pub(crate) snapshot_kind: SnapshotKind, } impl MetaData { @@ -222,10 +222,10 @@ impl MetaData { let mut expression = None; let mut info = None; let mut input_file = None; - let mut snapshot_type = TmpSnapshotType::Text; + let mut snapshot_type = TmpSnapshotKind::Text; let mut extension = None; - enum TmpSnapshotType { + enum TmpSnapshotKind { Text, Binary, } @@ -238,10 +238,10 @@ impl MetaData { Some("expression") => expression = value.as_str().map(Into::into), Some("info") if !value.is_nil() => info = Some(value), Some("input_file") => input_file = value.as_str().map(Into::into), - Some("snapshot_type") => { + Some("snapshot_kind") => { snapshot_type = match value.as_str() { - Some("binary") => TmpSnapshotType::Binary, - _ => TmpSnapshotType::Text, + Some("binary") => TmpSnapshotKind::Binary, + _ => TmpSnapshotKind::Text, } } Some("extension") => { @@ -258,9 +258,9 @@ impl MetaData { expression, info, input_file, - snapshot_type: match snapshot_type { - TmpSnapshotType::Text => SnapshotType::Text, - TmpSnapshotType::Binary => SnapshotType::Binary { + snapshot_kind: match snapshot_type { + TmpSnapshotKind::Text => SnapshotKind::Text, + TmpSnapshotKind::Binary => SnapshotKind::Binary { extension: extension.ok_or(content::Error::MissingField)?, }, }, @@ -291,15 +291,15 @@ impl MetaData { fields.push(("input_file", Content::from(input_file))); } - let snapshot_type = Content::from(match self.snapshot_type { - SnapshotType::Text => "text", - SnapshotType::Binary { ref extension } => { + let snapshot_type = Content::from(match self.snapshot_kind { + SnapshotKind::Text => "text", + SnapshotKind::Binary { ref extension } => { fields.push(("extension", Content::from(extension.clone()))); "binary" } }); - fields.push(("snapshot_type", snapshot_type)); + fields.push(("snapshot_kind", snapshot_type)); Content::Struct("MetaData", fields) } @@ -389,8 +389,8 @@ impl Snapshot { rv }; - let contents = match metadata.snapshot_type { - SnapshotType::Text => { + let contents = match metadata.snapshot_kind { + SnapshotKind::Text => { buf.clear(); for (idx, line) in f.lines().enumerate() { let line = line?; @@ -406,7 +406,7 @@ impl Snapshot { } .into() } - SnapshotType::Binary { ref extension } => { + SnapshotKind::Binary { ref extension } => { let path = build_binary_path(extension, p); let contents = fs::read(path)?; @@ -519,7 +519,7 @@ impl Snapshot { pub fn matches(&self, other: &Self) -> bool { self.contents() == other.contents() // For binary snapshots the extension also need to be the same: - && self.metadata.snapshot_type == other.metadata.snapshot_type + && self.metadata.snapshot_kind == other.metadata.snapshot_kind } /// Both the exact snapshot contents and the persisted metadata match another snapshot's. @@ -540,7 +540,7 @@ impl Snapshot { } (SnapshotContents::Binary(a), SnapshotContents::Binary(b)) => { // For binary snapshots the extension also need to be the same: - a == b && self.metadata.snapshot_type == other.metadata.snapshot_type + a == b && self.metadata.snapshot_kind == other.metadata.snapshot_kind } _ => false, } @@ -584,7 +584,7 @@ impl Snapshot { } pub fn build_binary_path(&self, path: impl Into) -> Option { - if let SnapshotType::Binary { ref extension } = self.metadata.snapshot_type { + if let SnapshotKind::Binary { ref extension } = self.metadata.snapshot_kind { Some(build_binary_path(extension, path)) } else { None diff --git a/insta/tests/snapshots/test_binary__binary_snapshot.snap b/insta/tests/snapshots/test_binary__binary_snapshot.snap index 0d22a087..82cee89a 100644 --- a/insta/tests/snapshots/test_binary__binary_snapshot.snap +++ b/insta/tests/snapshots/test_binary__binary_snapshot.snap @@ -2,5 +2,5 @@ source: insta/tests/test_binary.rs expression: "b\"test\".to_vec()" extension: txt -snapshot_type: binary +snapshot_kind: binary --- diff --git a/insta/tests/snapshots/test_binary__multipart_extension.snap b/insta/tests/snapshots/test_binary__multipart_extension.snap index 8e2b383a..970caa1b 100644 --- a/insta/tests/snapshots/test_binary__multipart_extension.snap +++ b/insta/tests/snapshots/test_binary__multipart_extension.snap @@ -2,5 +2,5 @@ source: insta/tests/test_binary.rs expression: "b\"test\".to_vec()" extension: tar.gz -snapshot_type: binary +snapshot_kind: binary --- diff --git a/insta/tests/snapshots/test_binary__name.snap b/insta/tests/snapshots/test_binary__name.snap index a99b42ee..99207dc0 100644 --- a/insta/tests/snapshots/test_binary__name.snap +++ b/insta/tests/snapshots/test_binary__name.snap @@ -2,5 +2,5 @@ source: insta/tests/test_binary.rs expression: "b\"null\".to_vec()" extension: json -snapshot_type: binary +snapshot_kind: binary --- From 644b49117998910a1b759159c29205a5ebfe4bf4 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Fri, 4 Oct 2024 15:32:19 +0200 Subject: [PATCH 29/41] Add doc comment for assert_binary_snapshot macro --- insta/src/macros.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/insta/src/macros.rs b/insta/src/macros.rs index d2f2b09b..6e3f2241 100644 --- a/insta/src/macros.rs +++ b/insta/src/macros.rs @@ -375,6 +375,15 @@ macro_rules! _assert_snapshot_base { }; } +/// (Experimental) +/// Asserts a binary snapshot in the form of a [`Vec`]. +/// +/// The contents get stored in a separate file next to the metadata file. The extension for this +/// file must be passed as part of the name. For an implicit snapshot name just an extension can be +/// passed starting with a `.`. +/// +/// This feature is considered experimental: we may make incompatible changes for the next couple +/// of versions after 1.41. #[macro_export] macro_rules! assert_binary_snapshot { ($name_and_extension:expr, $value:expr $(,)?) => { From 966242fef4b7023a0684f5a137a01ec5e2951949 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Fri, 4 Oct 2024 15:33:09 +0200 Subject: [PATCH 30/41] Add changelog entry for binary snapshots --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 910f005c..c857e7c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ All notable changes to insta and cargo-insta are documented here. - Inline snapshots only use `#` characters as delimiters when required. #603 +- Experimental support for binary snapshots + ## 1.40.0 - `cargo-insta` no longer panics when running `cargo insta test --accept --workspace` From ae0dba60de608536cb8137cbb38f1772da61b69e Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Fri, 4 Oct 2024 17:20:12 +0200 Subject: [PATCH 31/41] Improve error message for wrong name format for binary snapshots --- insta/src/runtime.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 2f3c97ae..75d5d12e 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -166,9 +166,12 @@ impl<'a> From> for SnapshotValue<'a> { content, }: BinarySnapshotValue<'a>, ) -> Self { - let (name, extension) = name_and_extension - .split_once('.') - .expect("the snapshot name and extension should be in the format \"name.extension\""); + let (name, extension) = name_and_extension.split_once('.').unwrap_or_else(|| { + panic!( + "{} does not match the format \"name.extension\"", + name_and_extension, + ) + }); let name = if name.is_empty() { None From dc2fcd1522034aa3a84157633d93d2f59fe7b3c9 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Fri, 4 Oct 2024 17:23:32 +0200 Subject: [PATCH 32/41] Improve assert_snapshot paramter naming In the inline case this value also contains the old snapshot value so new_snapshot_value is not correct. --- insta/src/runtime.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 75d5d12e..9402c0ef 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -755,7 +755,7 @@ where /// assertion with a panic if needed. #[allow(clippy::too_many_arguments)] pub fn assert_snapshot( - new_snapshot_value: SnapshotValue<'_>, + snapshot_value: SnapshotValue<'_>, workspace: &Path, function_name: &str, module_path: &str, @@ -764,7 +764,7 @@ pub fn assert_snapshot( expr: &str, ) -> Result<(), Box> { let ctx = SnapshotAssertionContext::prepare( - &new_snapshot_value, + &snapshot_value, workspace, function_name, module_path, @@ -774,7 +774,7 @@ pub fn assert_snapshot( ctx.cleanup_previous_pending_binary_snapshots()?; - let content = match new_snapshot_value { + let content = match snapshot_value { SnapshotValue::FileText { content, .. } | SnapshotValue::InlineText { content, .. } => { // apply filters if they are available #[cfg(feature = "filters")] From 988aed11bc6cb7c5f460098064d1ad81e0eac619 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Fri, 4 Oct 2024 17:35:17 +0200 Subject: [PATCH 33/41] Improve error message for wrong name format for binary snapshots Additional quotes around the input and adjusted test. --- insta/src/runtime.rs | 2 +- insta/tests/test_binary.rs | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 9402c0ef..5bc0d745 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -168,7 +168,7 @@ impl<'a> From> for SnapshotValue<'a> { ) -> Self { let (name, extension) = name_and_extension.split_once('.').unwrap_or_else(|| { panic!( - "{} does not match the format \"name.extension\"", + "\"{}\" does not match the format \"name.extension\"", name_and_extension, ) }); diff --git a/insta/tests/test_binary.rs b/insta/tests/test_binary.rs index 9927b5dd..c160a306 100644 --- a/insta/tests/test_binary.rs +++ b/insta/tests/test_binary.rs @@ -10,9 +10,7 @@ fn test_new_extension() { } #[test] -#[should_panic( - expected = "the snapshot name and extension should be in the format \"name.extension\"" -)] +#[should_panic(expected = "\"test\" does not match the format \"name.extension\"")] fn test_malformed_name_and_extension() { insta::assert_binary_snapshot!("test", b"test".to_vec()); } From 4cd477b7af30272465a288bce7968c7aa27e5a4e Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Fri, 4 Oct 2024 17:37:30 +0200 Subject: [PATCH 34/41] Call matches in matches_fully for binary snapshots --- insta/src/snapshot.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index cee080c8..08037b84 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -538,11 +538,7 @@ impl Snapshot { TextSnapshotKind::Inline => contents_match_exact, } } - (SnapshotContents::Binary(a), SnapshotContents::Binary(b)) => { - // For binary snapshots the extension also need to be the same: - a == b && self.metadata.snapshot_kind == other.metadata.snapshot_kind - } - _ => false, + _ => self.matches(other), } } From 090fe5c30810d0a415c0abce753505202ab6cf85 Mon Sep 17 00:00:00 2001 From: Florian Plattner <64537872+lasernoises@users.noreply.github.com> Date: Fri, 4 Oct 2024 19:57:54 +0200 Subject: [PATCH 35/41] Update CHANGELOG.md Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c857e7c6..4eedbe66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ All notable changes to insta and cargo-insta are documented here. - Inline snapshots only use `#` characters as delimiters when required. #603 -- Experimental support for binary snapshots +- Experimental support for binary snapshots. #610 (Florian Plattner) ## 1.40.0 From 102010ff92a426ae9e22593b42879c2e9377c74b Mon Sep 17 00:00:00 2001 From: Florian Plattner <64537872+lasernoises@users.noreply.github.com> Date: Fri, 4 Oct 2024 19:58:54 +0200 Subject: [PATCH 36/41] Update insta/src/runtime.rs Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> --- insta/src/runtime.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 5bc0d745..3c73c6fd 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -792,7 +792,7 @@ pub fn assert_snapshot( } => { assert!( extension != "new", - "'.new' is not allowed as a file extension" + "'new' is not allowed as a file extension" ); assert!( !extension.starts_with("new."), From f59925ed8de604abdc4de281637fa58a1be144c2 Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Fri, 4 Oct 2024 11:02:06 -0700 Subject: [PATCH 37/41] Update insta/src/runtime.rs --- insta/src/runtime.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 3c73c6fd..5bc0d745 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -792,7 +792,7 @@ pub fn assert_snapshot( } => { assert!( extension != "new", - "'new' is not allowed as a file extension" + "'.new' is not allowed as a file extension" ); assert!( !extension.starts_with("new."), From 0a639ae1977b2954714780fb2a48edac4135bae1 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Fri, 4 Oct 2024 20:34:16 +0200 Subject: [PATCH 38/41] Fix deletion of unreferenced binary snapshots Previously it only deleted the .snap file, but not the binary file next to it. --- cargo-insta/src/cli.rs | 6 +++ cargo-insta/tests/main.rs | 88 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+) diff --git a/cargo-insta/src/cli.rs b/cargo-insta/src/cli.rs index 7c7c89af..811815cd 100644 --- a/cargo-insta/src/cli.rs +++ b/cargo-insta/src/cli.rs @@ -844,6 +844,12 @@ fn handle_unreferenced_snapshots( } eprintln!(" {}", path.display()); if matches!(action, Action::Delete) { + let snapshot = Snapshot::from_file(&path)?; + + if let Some(binary_path) = snapshot.build_binary_path(&path) { + fs::remove_file(&binary_path).ok(); + } + fs::remove_file(&path).ok(); } } diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index 6bac5d9f..1ea21df7 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -1976,3 +1976,91 @@ Unused snapshot + src/snapshots/test_unreferenced_delete__tests__snapshot.snap "); } + +#[test] +fn test_binary_unreferenced_delete() { + let test_project = TestFiles::new() + .add_file( + "Cargo.toml", + r#" +[package] +name = "test_binary_unreferenced_delete" +version = "0.1.0" +edition = "2021" + +[lib] +doctest = false + +[dependencies] +insta = { path = '$PROJECT_PATH' } +"# + .to_string(), + ) + .add_file( + "src/lib.rs", + r#" +#[cfg(test)] +mod tests { + #[test] + fn test_snapshot() { + insta::assert_binary_snapshot!(".txt", b"abcd".to_vec()); + } +} +"# + .to_string(), + ) + .create_project(); + + // Run tests to create snapshots + let output = test_project + .insta_cmd() + .args(["test", "--accept"]) + .output() + .unwrap(); + + assert!(&output.status.success()); + + test_project.update_file("src/lib.rs", "".to_string()); + + assert_snapshot!(test_project.file_tree_diff(), @r" + --- Original file tree + +++ Updated file tree + @@ -1,4 +1,8 @@ + + + Cargo.lock + Cargo.toml + src + src/lib.rs + + src/snapshots + + src/snapshots/test_binary_unreferenced_delete__tests__snapshot.snap + + src/snapshots/test_binary_unreferenced_delete__tests__snapshot.snap.txt + "); + + // Run cargo insta test with --unreferenced=delete + let output = test_project + .insta_cmd() + .args([ + "test", + "--unreferenced=delete", + "--accept", + "--", + "--nocapture", + ]) + .output() + .unwrap(); + + assert!(&output.status.success()); + + // We should now see the unreferenced snapshot deleted + assert_snapshot!(test_project.file_tree_diff(), @r" + --- Original file tree + +++ Updated file tree + @@ -1,4 +1,6 @@ + + + Cargo.lock + Cargo.toml + src + src/lib.rs + + src/snapshots + "); +} From f437077c74ba2698cd9c40a103488432e0c03429 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Fri, 4 Oct 2024 21:07:00 +0200 Subject: [PATCH 39/41] Fix snapshot printing bug when changing between binary and text The message "snapshots are matching" was previously printed when replacing a binary snapshot with an empty string text snapshot. --- insta/src/output.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/insta/src/output.rs b/insta/src/output.rs index 76d22c86..d63d30c2 100644 --- a/insta/src/output.rs +++ b/insta/src/output.rs @@ -219,7 +219,11 @@ impl<'a> SnapshotPrinter<'a> { } println!("────────────┬{:─^1$}", "", width.saturating_sub(13)); - let mut has_changes = false; + + // This is to make sure that binary and text snapshots are never reported as being + // equal (that would otherwise happen if the text snapshot is an empty string). + let mut has_changes = old.is_none() || new.is_none(); + for (idx, group) in diff.grouped_ops(4).iter().enumerate() { if idx > 0 { println!("┈┈┈┈┈┈┈┈┈┈┈┈┼{:┈^1$}", "", width.saturating_sub(13)); From e0a61c96a41edc9f2d90ff0851fb97a64ae8c7b4 Mon Sep 17 00:00:00 2001 From: Florian Plattner Date: Fri, 4 Oct 2024 21:08:22 +0200 Subject: [PATCH 40/41] Add examples to assert_binary_snapshot doc comment --- insta/src/macros.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/insta/src/macros.rs b/insta/src/macros.rs index 6e3f2241..af1e3fea 100644 --- a/insta/src/macros.rs +++ b/insta/src/macros.rs @@ -384,6 +384,16 @@ macro_rules! _assert_snapshot_base { /// /// This feature is considered experimental: we may make incompatible changes for the next couple /// of versions after 1.41. +/// +/// Examples: +/// +/// ```no_run +/// // implicit name: +/// insta::assert_binary_snapshot!(".txt", b"abcd".to_vec()); +/// +/// // named: +/// insta::assert_binary_snapshot!("my_snapshot.bin", [0, 1, 2, 3].to_vec()); +/// ``` #[macro_export] macro_rules! assert_binary_snapshot { ($name_and_extension:expr, $value:expr $(,)?) => { From 62637b258245123716d0ea0b8e7a25ccb92b975f Mon Sep 17 00:00:00 2001 From: Florian Plattner <64537872+lasernoises@users.noreply.github.com> Date: Fri, 4 Oct 2024 23:08:39 +0200 Subject: [PATCH 41/41] Improve error handling for deletion of unreferenced snapshots --- cargo-insta/src/cli.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/cargo-insta/src/cli.rs b/cargo-insta/src/cli.rs index 811815cd..d9bbe449 100644 --- a/cargo-insta/src/cli.rs +++ b/cargo-insta/src/cli.rs @@ -844,7 +844,13 @@ fn handle_unreferenced_snapshots( } eprintln!(" {}", path.display()); if matches!(action, Action::Delete) { - let snapshot = Snapshot::from_file(&path)?; + let snapshot = match Snapshot::from_file(&path) { + Ok(snapshot) => snapshot, + Err(e) => { + eprintln!("Error loading snapshot at {:?}: {}", &path, e); + continue; + } + }; if let Some(binary_path) = snapshot.build_binary_path(&path) { fs::remove_file(&binary_path).ok();