From c235d6320e3c6985b0eaa0c37f8e2917a1989327 Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Sun, 22 Sep 2024 11:18:47 -0700 Subject: [PATCH 01/19] Add a test and comments for selecting indentation (#606) --- cargo-insta/src/inline.rs | 5 ++++- insta/tests/test_inline.rs | 11 +++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/cargo-insta/src/inline.rs b/cargo-insta/src/inline.rs index b0ee5150..5aee2f49 100644 --- a/cargo-insta/src/inline.rs +++ b/cargo-insta/src/inline.rs @@ -149,19 +149,22 @@ impl FilePatcher { impl Visitor { fn scan_nested_macros(&mut self, tokens: &[TokenTree]) { for idx in 0..tokens.len() { + // Look for the start of a macro (potential snapshot location) if let Some(TokenTree::Ident(_)) = tokens.get(idx) { if let Some(TokenTree::Punct(ref punct)) = tokens.get(idx + 1) { if punct.as_char() == '!' { if let Some(TokenTree::Group(ref group)) = tokens.get(idx + 2) { + // Found a macro, determine its indentation let indentation = scan_for_path_start(tokens, idx); + // Extract tokens from the macro arguments let tokens: Vec<_> = group.stream().into_iter().collect(); + // Try to extract a snapshot, passing the calculated indentation self.try_extract_snapshot(&tokens, indentation); } } } } } - for token in tokens { // recurse into groups if let TokenTree::Group(group) = token { diff --git a/insta/tests/test_inline.rs b/insta/tests/test_inline.rs index 93f70126..bdee1fed 100644 --- a/insta/tests/test_inline.rs +++ b/insta/tests/test_inline.rs @@ -304,3 +304,14 @@ fn test_inline_snapshot_whitespace() { "###); } + +#[test] +fn test_indentation() { + assert_snapshot!("aaa\nbbb\nccc\nddd", @r" + aaa + bbb + ccc + ddd + " + ); +} From 8baf9a39ba818c44b838c1b794aa3f86c777e3e6 Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Sun, 22 Sep 2024 11:44:22 -0700 Subject: [PATCH 02/19] Add a couple tests for multiline strings with empty lines (#607) --- insta/src/snapshot.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index 7d487eaa..45d3f0df 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -1065,3 +1065,31 @@ fn test_ownership() { assert_debug_snapshot!(r, @"0..10"); assert_debug_snapshot!(r, @"0..10"); } + +#[test] +fn test_empty_lines() { + assert_snapshot!(r#"single line should fit on a single line"#, @"single line should fit on a single line"); + assert_snapshot!(r#"single line should fit on a single line, even if it's really really really really really really really really really long"#, @"single line should fit on a single line, even if it's really really really really really really really really really long"); + + assert_snapshot!(r#"multiline content starting on first line + + final line + "#, @r###" + multiline content starting on first line + + final line + + "###); + + assert_snapshot!(r#" + multiline content starting on second line + + final line + "#, @r###" + + multiline content starting on second line + + final line + + "###); +} From caa1e8f96c8ef4e33c028d0f24c4e4fa36d8e45f Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Sun, 22 Sep 2024 12:12:15 -0700 Subject: [PATCH 03/19] Consolidate a couple of test files (#608) Not as extreme as https://github.com/mitsuhiko/insta/pull/459, but consolidates a few files that were separate binaries and 1-2 functions. --- .../test_advanced__basic_suffixes@1.snap | 5 +++++ .../test_advanced__basic_suffixes@2.snap | 5 +++++ .../test_advanced__basic_suffixes@3.snap | 5 +++++ ..._bugs__crlf.snap => test_basic__crlf.snap} | 2 +- ...lf.snap => test_basic__trailing_crlf.snap} | 2 +- .../test_suffixes__basic_suffixes@1.snap | 5 ----- .../test_suffixes__basic_suffixes@2.snap | 5 ----- .../test_suffixes__basic_suffixes@3.snap | 5 ----- ...t_allow_duplicates.rs => test_advanced.rs} | 21 +++++++++++++++++++ insta/tests/test_basic.rs | 19 +++++++++++++++++ insta/tests/test_bugs.rs | 18 ---------------- insta/tests/test_filters.rs | 12 ----------- insta/tests/test_suffixes.rs | 9 -------- 13 files changed, 57 insertions(+), 56 deletions(-) create mode 100644 insta/tests/snapshots/test_advanced__basic_suffixes@1.snap create mode 100644 insta/tests/snapshots/test_advanced__basic_suffixes@2.snap create mode 100644 insta/tests/snapshots/test_advanced__basic_suffixes@3.snap rename insta/tests/snapshots/{test_bugs__crlf.snap => test_basic__crlf.snap} (63%) rename insta/tests/snapshots/{test_bugs__trailing_crlf.snap => test_basic__trailing_crlf.snap} (66%) delete mode 100644 insta/tests/snapshots/test_suffixes__basic_suffixes@1.snap delete mode 100644 insta/tests/snapshots/test_suffixes__basic_suffixes@2.snap delete mode 100644 insta/tests/snapshots/test_suffixes__basic_suffixes@3.snap rename insta/tests/{test_allow_duplicates.rs => test_advanced.rs} (53%) delete mode 100644 insta/tests/test_bugs.rs delete mode 100644 insta/tests/test_filters.rs delete mode 100644 insta/tests/test_suffixes.rs diff --git a/insta/tests/snapshots/test_advanced__basic_suffixes@1.snap b/insta/tests/snapshots/test_advanced__basic_suffixes@1.snap new file mode 100644 index 00000000..1f01b3f4 --- /dev/null +++ b/insta/tests/snapshots/test_advanced__basic_suffixes@1.snap @@ -0,0 +1,5 @@ +--- +source: insta/tests/test_advanced.rs +expression: "&value" +--- +1 diff --git a/insta/tests/snapshots/test_advanced__basic_suffixes@2.snap b/insta/tests/snapshots/test_advanced__basic_suffixes@2.snap new file mode 100644 index 00000000..9f7dce28 --- /dev/null +++ b/insta/tests/snapshots/test_advanced__basic_suffixes@2.snap @@ -0,0 +1,5 @@ +--- +source: insta/tests/test_advanced.rs +expression: "&value" +--- +2 diff --git a/insta/tests/snapshots/test_advanced__basic_suffixes@3.snap b/insta/tests/snapshots/test_advanced__basic_suffixes@3.snap new file mode 100644 index 00000000..1cab7cd9 --- /dev/null +++ b/insta/tests/snapshots/test_advanced__basic_suffixes@3.snap @@ -0,0 +1,5 @@ +--- +source: insta/tests/test_advanced.rs +expression: "&value" +--- +3 diff --git a/insta/tests/snapshots/test_bugs__crlf.snap b/insta/tests/snapshots/test_basic__crlf.snap similarity index 63% rename from insta/tests/snapshots/test_bugs__crlf.snap rename to insta/tests/snapshots/test_basic__crlf.snap index e3c3075f..bd6aba5c 100644 --- a/insta/tests/snapshots/test_bugs__crlf.snap +++ b/insta/tests/snapshots/test_basic__crlf.snap @@ -1,5 +1,5 @@ --- -source: insta/tests/test_bugs.rs +source: insta/tests/test_basic.rs expression: "\"foo\\r\\nbar\\r\\nbaz\"" --- foo diff --git a/insta/tests/snapshots/test_bugs__trailing_crlf.snap b/insta/tests/snapshots/test_basic__trailing_crlf.snap similarity index 66% rename from insta/tests/snapshots/test_bugs__trailing_crlf.snap rename to insta/tests/snapshots/test_basic__trailing_crlf.snap index e61fca0b..cd7d27c4 100644 --- a/insta/tests/snapshots/test_bugs__trailing_crlf.snap +++ b/insta/tests/snapshots/test_basic__trailing_crlf.snap @@ -1,5 +1,5 @@ --- -source: insta/tests/test_bugs.rs +source: insta/tests/test_basic.rs expression: "\"foo\\r\\nbar\\r\\nbaz\\r\\n\"" --- foo diff --git a/insta/tests/snapshots/test_suffixes__basic_suffixes@1.snap b/insta/tests/snapshots/test_suffixes__basic_suffixes@1.snap deleted file mode 100644 index 0ebfdecc..00000000 --- a/insta/tests/snapshots/test_suffixes__basic_suffixes@1.snap +++ /dev/null @@ -1,5 +0,0 @@ ---- -source: insta/tests/test_suffixes.rs -expression: "&value" ---- -1 diff --git a/insta/tests/snapshots/test_suffixes__basic_suffixes@2.snap b/insta/tests/snapshots/test_suffixes__basic_suffixes@2.snap deleted file mode 100644 index 11ca6d60..00000000 --- a/insta/tests/snapshots/test_suffixes__basic_suffixes@2.snap +++ /dev/null @@ -1,5 +0,0 @@ ---- -source: insta/tests/test_suffixes.rs -expression: "&value" ---- -2 diff --git a/insta/tests/snapshots/test_suffixes__basic_suffixes@3.snap b/insta/tests/snapshots/test_suffixes__basic_suffixes@3.snap deleted file mode 100644 index 94346c75..00000000 --- a/insta/tests/snapshots/test_suffixes__basic_suffixes@3.snap +++ /dev/null @@ -1,5 +0,0 @@ ---- -source: insta/tests/test_suffixes.rs -expression: "&value" ---- -3 diff --git a/insta/tests/test_allow_duplicates.rs b/insta/tests/test_advanced.rs similarity index 53% rename from insta/tests/test_allow_duplicates.rs rename to insta/tests/test_advanced.rs index 3a862de6..f8b46645 100644 --- a/insta/tests/test_allow_duplicates.rs +++ b/insta/tests/test_advanced.rs @@ -1,5 +1,26 @@ use insta::{allow_duplicates, assert_debug_snapshot}; +#[cfg(feature = "filters")] +#[test] +fn test_basic_filter() { + use insta::{assert_snapshot, with_settings}; + with_settings!({filters => vec![ + (r"\b[[:xdigit:]]{8}\b", "[SHORT_HEX]") + ]}, { + assert_snapshot!("Hello DEADBEEF!", @"Hello [SHORT_HEX]!"); + }) +} + +#[cfg(feature = "json")] +#[test] +fn test_basic_suffixes() { + for value in [1, 2, 3] { + insta::with_settings!({snapshot_suffix => value.to_string()}, { + insta::assert_json_snapshot!(&value); + }); + } +} + #[test] fn test_basic_duplicates_passes() { allow_duplicates! { diff --git a/insta/tests/test_basic.rs b/insta/tests/test_basic.rs index 010612f9..33b67611 100644 --- a/insta/tests/test_basic.rs +++ b/insta/tests/test_basic.rs @@ -116,3 +116,22 @@ fn insta_sort_order() { insta::assert_yaml_snapshot!(m); }); } + +#[test] +fn test_crlf() { + insta::assert_snapshot!("foo\r\nbar\r\nbaz"); +} + +#[test] +fn test_trailing_crlf() { + insta::assert_snapshot!("foo\r\nbar\r\nbaz\r\n"); +} + +#[test] +fn test_trailing_crlf_inline() { + insta::assert_snapshot!("foo\r\nbar\r\nbaz\r\n", @r#" + foo + bar + baz + "#); +} diff --git a/insta/tests/test_bugs.rs b/insta/tests/test_bugs.rs deleted file mode 100644 index b0700fe7..00000000 --- a/insta/tests/test_bugs.rs +++ /dev/null @@ -1,18 +0,0 @@ -#[test] -fn test_crlf() { - insta::assert_snapshot!("foo\r\nbar\r\nbaz"); -} - -#[test] -fn test_trailing_crlf() { - insta::assert_snapshot!("foo\r\nbar\r\nbaz\r\n"); -} - -#[test] -fn test_trailing_crlf_inline() { - insta::assert_snapshot!("foo\r\nbar\r\nbaz\r\n", @r###" - foo - bar - baz - "###); -} diff --git a/insta/tests/test_filters.rs b/insta/tests/test_filters.rs deleted file mode 100644 index 148e2961..00000000 --- a/insta/tests/test_filters.rs +++ /dev/null @@ -1,12 +0,0 @@ -#![cfg(feature = "filters")] - -use insta::{assert_snapshot, with_settings}; - -#[test] -fn test_basic_filter() { - with_settings!({filters => vec![ - (r"\b[[:xdigit:]]{8}\b", "[SHORT_HEX]") - ]}, { - assert_snapshot!("Hello DEADBEEF!", @"Hello [SHORT_HEX]!"); - }) -} diff --git a/insta/tests/test_suffixes.rs b/insta/tests/test_suffixes.rs deleted file mode 100644 index af0eb916..00000000 --- a/insta/tests/test_suffixes.rs +++ /dev/null @@ -1,9 +0,0 @@ -#[cfg(feature = "json")] -#[test] -fn test_basic_suffixes() { - for value in [1, 2, 3] { - insta::with_settings!({snapshot_suffix => value.to_string()}, { - insta::assert_json_snapshot!(&value); - }); - } -} From 92b0cafe8bc3db215d011f9f68c22ed69c87ba14 Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Mon, 23 Sep 2024 17:29:41 -0700 Subject: [PATCH 04/19] Couple of small refinements to prefix / suffixes (#611) Spotted when reviewing code; this uses native rust methods rather than specific slicing --- cargo-insta/src/walk.rs | 28 +++++++++++++++------------- insta/src/runtime.rs | 15 +++++++-------- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/cargo-insta/src/walk.rs b/cargo-insta/src/walk.rs index d94c651a..02751990 100644 --- a/cargo-insta/src/walk.rs +++ b/cargo-insta/src/walk.rs @@ -28,23 +28,25 @@ pub(crate) fn find_pending_snapshots<'a>( flags: FindFlags, ) -> impl Iterator>> + 'a { make_snapshot_walker(package_root, extensions, flags) - .filter_map(|e| e.ok()) - .filter_map(move |e| { - let fname = e.file_name().to_string_lossy(); - if fname.ends_with(".new") { - let new_path = e.into_path(); - let old_path = new_path.clone().with_extension(""); + .filter_map(Result::ok) + .filter_map(|entry| { + let fname = entry.file_name().to_string_lossy(); + let path = entry.clone().into_path(); + + #[allow(clippy::manual_map)] + if let Some(new_fname) = fname.strip_suffix(".new") { Some(SnapshotContainer::load( - new_path, - old_path, + path.clone(), + path.with_file_name(new_fname), SnapshotKind::File, )) - } else if fname.starts_with('.') && fname.ends_with(".pending-snap") { - let mut target_path = e.path().to_path_buf(); - target_path.set_file_name(&fname[1..fname.len() - 13]); + } else if let Some(new_fname) = fname + .strip_prefix('.') + .and_then(|f| f.strip_suffix(".pending-snap")) + { Some(SnapshotContainer::load( - e.path().to_path_buf(), - target_path, + path.clone(), + path.with_file_name(new_fname), SnapshotKind::Inline, )) } else { diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 38cb3a4e..73392edf 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -95,15 +95,14 @@ fn is_doctest(function_name: &str) -> bool { } fn detect_snapshot_name(function_name: &str, module_path: &str) -> Result { - let mut name = function_name; - // clean test name first - name = name.rsplit("::").next().unwrap(); - let mut test_prefixed = false; - if name.starts_with("test_") { - name = &name[5..]; - test_prefixed = true; - } + let name = function_name.rsplit("::").next().unwrap(); + + let (name, test_prefixed) = if let Some(stripped) = name.strip_prefix("test_") { + (stripped, true) + } else { + (name, false) + }; // next check if we need to add a suffix let name = add_suffix_to_snapshot_name(Cow::Borrowed(name)); From 8753f2cc59284bd8bc8c645594e524fd16495423 Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Tue, 24 Sep 2024 00:45:39 -0700 Subject: [PATCH 05/19] Add a couple of docstrings / small refactors (#612) Helpful to me, hopefully helpful to others --- cargo-insta/src/container.rs | 30 ++++++++++++++++-------------- insta/src/output.rs | 18 +++++++----------- insta/src/runtime.rs | 5 +++-- insta/src/settings.rs | 8 ++++---- insta/src/snapshot.rs | 2 ++ 5 files changed, 32 insertions(+), 31 deletions(-) diff --git a/cargo-insta/src/container.rs b/cargo-insta/src/container.rs index 6a2ded50..125b2fec 100644 --- a/cargo-insta/src/container.rs +++ b/cargo-insta/src/container.rs @@ -47,7 +47,9 @@ impl PendingSnapshot { /// single rust file. #[derive(Debug)] pub(crate) struct SnapshotContainer { - snapshot_path: PathBuf, + // Path of the pending snapshot file (generally a `.new` or `.pending-snap` file) + pending_path: PathBuf, + // Path of the target snapshot file (generally a `.snap` file) target_path: PathBuf, kind: SnapshotKind, snapshots: Vec, @@ -56,7 +58,7 @@ pub(crate) struct SnapshotContainer { impl SnapshotContainer { pub(crate) fn load( - snapshot_path: PathBuf, + pending_path: PathBuf, target_path: PathBuf, kind: SnapshotKind, ) -> Result> { @@ -68,7 +70,7 @@ impl SnapshotContainer { } else { Some(Snapshot::from_file(&target_path)?) }; - let new = Snapshot::from_file(&snapshot_path)?; + let new = Snapshot::from_file(&pending_path)?; snapshots.push(PendingSnapshot { id: 0, old, @@ -79,7 +81,7 @@ impl SnapshotContainer { None } SnapshotKind::Inline => { - let mut pending_vec = PendingInlineSnapshot::load_batch(&snapshot_path)?; + let mut pending_vec = PendingInlineSnapshot::load_batch(&pending_path)?; let mut have_new = false; let rv = if fs::metadata(&target_path).is_ok() { @@ -111,8 +113,8 @@ impl SnapshotContainer { // The runtime code will issue something like this: // PendingInlineSnapshot::new(None, None, line).save(pending_snapshots)?; if !have_new { - fs::remove_file(&snapshot_path) - .map_err(|e| ContentError::FileIo(e, snapshot_path.to_path_buf()))?; + fs::remove_file(&pending_path) + .map_err(|e| ContentError::FileIo(e, pending_path.to_path_buf()))?; } rv @@ -120,7 +122,7 @@ impl SnapshotContainer { }; Ok(SnapshotContainer { - snapshot_path, + pending_path, target_path, kind, snapshots, @@ -141,7 +143,7 @@ impl SnapshotContainer { pub(crate) fn snapshot_sort_key(&self) -> impl Ord + '_ { let path = self - .snapshot_path + .pending_path .file_name() .and_then(|x| x.to_str()) .unwrap_or_default(); @@ -201,9 +203,9 @@ impl SnapshotContainer { patcher.save()?; } if did_skip { - PendingInlineSnapshot::save_batch(&self.snapshot_path, &new_pending)?; + PendingInlineSnapshot::save_batch(&self.pending_path, &new_pending)?; } else { - try_removing_snapshot(&self.snapshot_path); + try_removing_snapshot(&self.pending_path); } } else { // should only be one or this is weird @@ -211,22 +213,22 @@ impl SnapshotContainer { for snapshot in self.snapshots.iter() { match snapshot.op { Operation::Accept => { - let snapshot = Snapshot::from_file(&self.snapshot_path).map_err(|e| { + 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.snapshot_path.to_path_buf(), + self.pending_path.to_path_buf(), )), Err(other_error) => other_error, } })?; snapshot.save(&self.target_path)?; - try_removing_snapshot(&self.snapshot_path); + try_removing_snapshot(&self.pending_path); } Operation::Reject => { - try_removing_snapshot(&self.snapshot_path); + try_removing_snapshot(&self.pending_path); } Operation::Skip => {} } diff --git a/insta/src/output.rs b/insta/src/output.rs index 5c9d103b..089af78f 100644 --- a/insta/src/output.rs +++ b/insta/src/output.rs @@ -226,13 +226,8 @@ pub fn print_snapshot_summary( workspace_root: &Path, snapshot: &Snapshot, snapshot_file: Option<&Path>, - mut line: Option, + line: Option, ) { - // default to old assertion line from snapshot. - if line.is_none() { - line = snapshot.metadata().assertion_line(); - } - if let Some(snapshot_file) = snapshot_file { let snapshot_file = workspace_root .join(snapshot_file) @@ -255,11 +250,12 @@ pub fn print_snapshot_summary( println!( "Source: {}{}", style(value.display()).cyan(), - if let Some(line) = line { - format!(":{}", style(line).bold()) - } else { - "".to_string() - } + line.or( + // default to old assertion line from snapshot. + snapshot.metadata().assertion_line() + ) + .map(|line| format!(":{}", style(line).bold())) + .unwrap_or_default() ); } diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 73392edf..22dca6e3 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -201,7 +201,8 @@ fn get_snapshot_filename( }) } -/// A single snapshot including surrounding context which asserts and save the +/// The context around a snapshot, such as the reference value, location, etc. +/// (but not including the generated value). Responsible for saving the /// snapshot. #[derive(Debug)] struct SnapshotAssertionContext<'a> { @@ -620,7 +621,7 @@ where /// assertion with a panic if needed. #[allow(clippy::too_many_arguments)] pub fn assert_snapshot( - refval: ReferenceValue<'_>, + refval: ReferenceValue, new_snapshot_value: &str, manifest_dir: &str, function_name: &str, diff --git a/insta/src/settings.rs b/insta/src/settings.rs index 7128cfe3..fbf5b3a8 100644 --- a/insta/src/settings.rs +++ b/insta/src/settings.rs @@ -269,10 +269,10 @@ impl Settings { /// Sets the input file reference. /// - /// This value is completely unused by the snapshot testing system but - /// it lets you store some meta data with a snapshot that refers you back - /// to the input file. The path stored here is made relative to the - /// workspace root before storing with the snapshot. + /// This value is completely unused by the snapshot testing system but it + /// allows storing some metadata with a snapshot that refers back to the + /// input file. The path stored here is made relative to the workspace root + /// before storing with the snapshot. pub fn set_input_file>(&mut self, p: P) { self._private_inner_mut().input_file(p); } diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index 45d3f0df..74e6ac12 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -418,6 +418,8 @@ impl Snapshot { fn as_content(&self) -> Content { let mut fields = vec![("module_name", Content::from(self.module_name.as_str()))]; + // Note this is currently never used, since this method is only used for + // inline snapshots if let Some(name) = self.snapshot_name.as_deref() { fields.push(("snapshot_name", Content::from(name))); } From 0c239ca1f5eab0a14c35f25401cd81b1158b155e Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Tue, 24 Sep 2024 23:55:52 -0700 Subject: [PATCH 06/19] Avoid collecting `tool_config` twice (#615) --- insta/src/runtime.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 22dca6e3..85af0dba 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -639,8 +639,6 @@ pub fn assert_snapshot( assertion_line, )?; - let tool_config = get_tool_config(manifest_dir); - // apply filters if they are available #[cfg(feature = "filters")] let new_snapshot_value = @@ -671,7 +669,7 @@ pub fn assert_snapshot( .old_snapshot .as_ref() .map(|x| { - if tool_config.require_full_match() { + if ctx.tool_config.require_full_match() { x.matches_fully(&new_snapshot) } else { x.matches(&new_snapshot) @@ -683,7 +681,7 @@ pub fn assert_snapshot( ctx.cleanup_passing()?; if matches!( - tool_config.snapshot_update(), + ctx.tool_config.snapshot_update(), crate::env::SnapshotUpdate::Force ) { // Avoid creating new files if contents match exactly. In From afd4d2010405564b720b89931e288c18cb8aaba1 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 07/19] 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 85af0dba..32bc4292 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -651,7 +651,7 @@ pub fn assert_snapshot( let new_snapshot = ctx.new_snapshot(SnapshotContents::new(new_snapshot_value.into(), kind), 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 57ee4406575fcc2a754c6fee08fef9e15c7bddf1 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 08/19] 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 075e5cef..754c2389 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -82,6 +82,7 @@ dependencies = [ "similar", "syn", "tempfile", + "termcolor", "uuid", "walkdir", ] diff --git a/cargo-insta/Cargo.toml b/cargo-insta/Cargo.toml index 9e95c6fd..06954a72 100644 --- a/cargo-insta/Cargo.toml +++ b/cargo-insta/Cargo.toml @@ -35,3 +35,4 @@ clap = { workspace=true } 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 db0ca74c..893e4849 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); } struct TestProject { @@ -230,7 +251,7 @@ fn test_json_snapshot() { let output = test_project .cmd() - .args(["test", "--accept"]) + .args(["test", "--accept", "--", "--nocapture"]) .output() .unwrap(); From 90e429b5b0ee11a320d53fa62253fcc50bb8ae98 Mon Sep 17 00:00:00 2001 From: Frank Elsinga Date: Fri, 27 Sep 2024 00:02:24 +0200 Subject: [PATCH 09/19] 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 a2e70ae221df0f52f1d0b63c3f49b2b580d99316 Mon Sep 17 00:00:00 2001 From: Frank Elsinga Date: Fri, 27 Sep 2024 05:24:33 +0200 Subject: [PATCH 10/19] docs: improved monospace quoting and gramar fixes (#619) --- cargo-insta/src/cli.rs | 4 ++-- cargo-insta/src/utils.rs | 2 +- insta/src/content/yaml/vendored/emitter.rs | 16 ++++++++-------- insta/src/glob.rs | 4 ++-- insta/src/lib.rs | 5 +++-- insta/src/settings.rs | 4 ++-- 6 files changed, 18 insertions(+), 17 deletions(-) diff --git a/cargo-insta/src/cli.rs b/cargo-insta/src/cli.rs index e3eafafe..ac00787a 100644 --- a/cargo-insta/src/cli.rs +++ b/cargo-insta/src/cli.rs @@ -83,7 +83,7 @@ enum Command { #[derive(Args, Debug, Clone)] struct TargetArgs { - /// Path to Cargo.toml + /// Path to `Cargo.toml` #[arg(long, value_name = "PATH")] manifest_path: Option, /// Explicit path to the workspace root @@ -95,7 +95,7 @@ struct TargetArgs { /// Work on all packages in the workspace #[arg(long)] workspace: bool, - /// Alias for --workspace (deprecated) + /// Alias for `--workspace` (deprecated) #[arg(long)] all: bool, /// Also walk into ignored paths. diff --git a/cargo-insta/src/utils.rs b/cargo-insta/src/utils.rs index 74930394..53d5b621 100644 --- a/cargo-insta/src/utils.rs +++ b/cargo-insta/src/utils.rs @@ -48,7 +48,7 @@ lazy_static! { pub static ref INSTA_VERSION: Version = read_insta_version().unwrap(); } -/// cargo-insta version +/// `cargo-insta` version // We could put this in a lazy_static pub(crate) fn cargo_insta_version() -> String { env!("CARGO_PKG_VERSION").to_string() diff --git a/insta/src/content/yaml/vendored/emitter.rs b/insta/src/content/yaml/vendored/emitter.rs index eddcaae4..7c8260f8 100644 --- a/insta/src/content/yaml/vendored/emitter.rs +++ b/insta/src/content/yaml/vendored/emitter.rs @@ -1,6 +1,5 @@ use crate::content::yaml::vendored::yaml::{Hash, Yaml}; -use std::convert::From; use std::error::Error; use std::fmt::{self, Display}; @@ -250,19 +249,20 @@ impl<'a> YamlEmitter<'a> { } /// Check if the string requires quoting. +/// /// Strings starting with any of the following characters must be quoted. -/// :, &, *, ?, |, -, <, >, =, !, %, @ +/// `:`, `&`, `*`, `?`, `|`, `-`, `<`, `>`, `=`, `!`, `%`, `@` /// Strings containing any of the following characters must be quoted. -/// {, }, [, ], ,, #, ` +/// `{`, `}`, `[`, `]`, `,`, `#`, `\`` /// /// If the string contains any of the following control characters, it must be escaped with double quotes: -/// \0, \x01, \x02, \x03, \x04, \x05, \x06, \a, \b, \t, \n, \v, \f, \r, \x0e, \x0f, \x10, \x11, \x12, \x13, \x14, \x15, \x16, \x17, \x18, \x19, \x1a, \e, \x1c, \x1d, \x1e, \x1f, \N, \_, \L, \P +/// `\0`, `\x01`, `\x02`, `\x03`, `\x04`, `\x05`, `\x06`, `\a`, `\b`, `\t`, `\n, `\v, `\f`, `\r`, `\x0e`, `\x0f`, `\x10`, `\x11`, `\x12`, `\x13`, `\x14`, `\x15`, `\x16`, `\x17`, `\x18`, `\x19`, `\x1a`, `\e`, `\x1c`, `\x1d`, `\x1e`, `\x1f`, `\N`, `\_`, `\L`, `\P` /// /// Finally, there are other cases when the strings must be quoted, no matter if you're using single or double quotes: -/// * When the string is true or false (otherwise, it would be treated as a boolean value); -/// * When the string is null or ~ (otherwise, it would be considered as a null value); -/// * When the string looks like a number, such as integers (e.g. 2, 14, etc.), floats (e.g. 2.6, 14.9) and exponential numbers (e.g. 12e7, etc.) (otherwise, it would be treated as a numeric value); -/// * When the string looks like a date (e.g. 2014-12-31) (otherwise it would be automatically converted into a Unix timestamp). +/// * When the string is `true` or `false` (otherwise, it would be treated as a boolean value); +/// * When the string is `null` or `~` (otherwise, it would be considered as a null value); +/// * When the string looks like a number, such as integers (e.g. `2`, `14`, etc.), floats (e.g. `2.6`, `14.9`) and exponential numbers (e.g. `12e7`, etc.) (otherwise, it would be treated as a numeric value); +/// * When the string looks like a date (e.g. `2014-12-31`) (otherwise it would be automatically converted into a Unix timestamp). fn need_quotes(string: &str) -> bool { fn need_quotes_spaces(string: &str) -> bool { string.starts_with(' ') || string.ends_with(' ') diff --git a/insta/src/glob.rs b/insta/src/glob.rs index 53579de4..e901702d 100644 --- a/insta/src/glob.rs +++ b/insta/src/glob.rs @@ -15,9 +15,9 @@ pub(crate) struct GlobCollector { pub(crate) show_insta_hint: bool, } -// the glob stack holds failure count + an indication if cargo insta review -// should be run. lazy_static::lazy_static! { + /// the glob stack holds failure count and an indication if `cargo insta review` + /// should be run. pub(crate) static ref GLOB_STACK: Mutex> = Mutex::default(); } diff --git a/insta/src/lib.rs b/insta/src/lib.rs index 574d3807..0d115ecf 100644 --- a/insta/src/lib.rs +++ b/insta/src/lib.rs @@ -46,7 +46,8 @@ //! ``` //! //! The recommended flow is to run the tests once, have them fail and check -//! if the result is okay. By default the new snapshots are stored next +//! if the result is okay. +//! By default, the new snapshots are stored next //! to the old ones with the extra `.new` extension. Once you are satisfied //! move the new files over. To simplify this workflow you can use //! `cargo insta review` (requires @@ -190,7 +191,7 @@ //! # Dependencies //! //! `insta` tries to be light in dependencies but this is tricky to accomplish -//! given what it tries to do. By default it currently depends on `serde` for +//! given what it tries to do. By default, it currently depends on `serde` for //! the [`assert_toml_snapshot!`] and [`assert_yaml_snapshot!`] macros. In the //! future this default dependencies will be removed. To already benefit from //! this optimization you can disable the default features and manually opt into diff --git a/insta/src/settings.rs b/insta/src/settings.rs index fbf5b3a8..74048818 100644 --- a/insta/src/settings.rs +++ b/insta/src/settings.rs @@ -210,7 +210,7 @@ impl Settings { /// Disables prepending of modules to the snapshot filename. /// - /// By default the filename of a snapshot is `__.snap`. + /// By default, the filename of a snapshot is `__.snap`. /// Setting this flag to `false` changes the snapshot filename to just /// `.snap`. /// @@ -226,7 +226,7 @@ impl Settings { /// Allows the [`glob!`] macro to succeed if it matches no files. /// - /// By default the glob macro will fail the test if it does not find + /// By default, the glob macro will fail the test if it does not find /// any files to prevent accidental typos. This can be disabled when /// fixtures should be conditional. /// From ed63aa9b20d7242669cb4545d138b12c81835251 Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Fri, 27 Sep 2024 00:16:30 -0700 Subject: [PATCH 11/19] Move a few functions onto context object (#622) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Small refactor — standardizes functions operating on a snapshot context as methods --- 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 32bc4292..c2c363ed 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -42,6 +42,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. /// @@ -440,127 +451,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( @@ -698,9 +698,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 3df868556bafa81f04f800015429703bdf5a6c93 Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Fri, 27 Sep 2024 01:39:48 -0700 Subject: [PATCH 12/19] Consolidate onto `INSTA_WORKSPACE_ROOT` (#614) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Intended to fix #575. Though after some iterations, it ends up being a code-clean-up. I'm not sure how `CARGO_MANIFEST_DIR` is leaking through — it's only referenced by `env!` rather than `env::var`, so should only matter at compile time. Added a pretty thorough integration test (albeit maybe a bit too complicated) --- cargo-insta/tests/main.rs | 153 ++++++++++++++++++++++++++++++++++++-- insta/src/env.rs | 103 +++++++++++++------------ insta/src/glob.rs | 4 +- insta/src/macros.rs | 28 ++++++- insta/src/runtime.rs | 37 ++++----- 5 files changed, 245 insertions(+), 80 deletions(-) diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index 893e4849..38b3a6ea 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -27,10 +27,10 @@ /// the same...). This also causes issues when running the same tests /// concurrently. use std::collections::HashMap; -use std::env; use std::fs; use std::path::{Path, PathBuf}; use std::process::Command; +use std::{env, fs::remove_dir_all}; use console::style; use ignore::WalkBuilder; @@ -102,6 +102,16 @@ fn assert_success(output: &std::process::Output) { eprint!("{}", stderr); } +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 unexpectedly 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, @@ -131,23 +141,26 @@ impl TestProject { workspace_dir, } } - fn cmd(&self) -> Command { - let mut command = Command::new(env!("CARGO_BIN_EXE_cargo-insta")); + fn clean_env(cmd: &mut Command) { // Remove environment variables so we don't inherit anything (such as // `INSTA_FORCE_PASS` or `CARGO_INSTA_*`) from a cargo-insta process // which runs this integration test. for (key, _) in env::vars() { if key.starts_with("CARGO_INSTA") || key.starts_with("INSTA") { - command.env_remove(&key); + cmd.env_remove(&key); } } // Turn off CI flag so that cargo insta test behaves as we expect // under normal operation - command.env("CI", "0"); + cmd.env("CI", "0"); // And any others that can affect the output - command.env_remove("CARGO_TERM_COLOR"); - command.env_remove("CLICOLOR_FORCE"); - command.env_remove("RUSTDOCFLAGS"); + cmd.env_remove("CARGO_TERM_COLOR"); + cmd.env_remove("CLICOLOR_FORCE"); + cmd.env_remove("RUSTDOCFLAGS"); + } + fn cmd(&self) -> Command { + let mut command = Command::new(env!("CARGO_BIN_EXE_cargo-insta")); + Self::clean_env(&mut command); command.current_dir(self.workspace_dir.as_path()); // Use the same target directory as other tests, consistent across test @@ -1166,3 +1179,127 @@ fn test_hashtag_escape() { } "####); } + +// Can't get the test binary discovery to work, don't have a windows machine to +// hand, others are welcome to fix it. (No specific reason to think that insta +// doesn't work on windows, just that the test doesn't work.) +#[cfg(not(target_os = "windows"))] +#[test] +fn test_insta_workspace_root() { + // This function locates the compiled test binary in the target directory. + // It's necessary because the exact filename of the test binary includes a hash + // that we can't predict, so we need to search for it. + fn find_test_binary(dir: &Path) -> PathBuf { + dir.join("target/debug/deps") + .read_dir() + .unwrap() + .filter_map(Result::ok) + .find(|entry| { + let file_name = entry.file_name(); + let file_name_str = file_name.to_str().unwrap_or(""); + // We're looking for a file that: + file_name_str.starts_with("insta_workspace_root_test-") // Matches our test name + && !file_name_str.contains('.') // Doesn't have an extension (it's the executable, not a metadata file) + && entry.metadata().map(|m| m.is_file()).unwrap_or(false) // Is a file, not a directory + }) + .map(|entry| entry.path()) + .expect("Failed to find test binary") + } + + fn run_test_binary( + binary_path: &Path, + current_dir: &Path, + env: Option<(&str, &str)>, + ) -> std::process::Output { + let mut cmd = Command::new(binary_path); + TestProject::clean_env(&mut cmd); + cmd.current_dir(current_dir); + if let Some((key, value)) = env { + cmd.env(key, value); + } + cmd.output().unwrap() + } + + let test_project = TestFiles::new() + .add_file( + "Cargo.toml", + r#" + [package] + name = "insta_workspace_root_test" + version = "0.1.0" + edition = "2021" + + [dependencies] + insta = { path = '$PROJECT_PATH' } + "# + .to_string(), + ) + .add_file( + "src/lib.rs", + r#" + #[cfg(test)] + mod tests { + use insta::assert_snapshot; + + #[test] + fn test_snapshot() { + assert_snapshot!("Hello, world!"); + } + } + "# + .to_string(), + ) + .create_project(); + + let mut cargo_cmd = Command::new("cargo"); + TestProject::clean_env(&mut cargo_cmd); + let output = cargo_cmd + .args(["test", "--no-run"]) + .current_dir(&test_project.workspace_dir) + .output() + .unwrap(); + assert_success(&output); + + let test_binary_path = find_test_binary(&test_project.workspace_dir); + + // Run the test without snapshot (should fail) + assert_failure(&run_test_binary( + &test_binary_path, + &test_project.workspace_dir, + None, + )); + + // Create the snapshot + assert_success(&run_test_binary( + &test_binary_path, + &test_project.workspace_dir, + Some(("INSTA_UPDATE", "always")), + )); + + // Verify snapshot creation + assert!(test_project.workspace_dir.join("src/snapshots").exists()); + assert!(test_project + .workspace_dir + .join("src/snapshots/insta_workspace_root_test__tests__snapshot.snap") + .exists()); + + // Move the workspace + let moved_workspace = { + let moved_workspace = PathBuf::from("/tmp/cargo-insta-test-moved"); + remove_dir_all(&moved_workspace).ok(); + fs::create_dir(&moved_workspace).unwrap(); + fs::rename(&test_project.workspace_dir, &moved_workspace).unwrap(); + moved_workspace + }; + let moved_binary_path = find_test_binary(&moved_workspace); + + // Run test in moved workspace without INSTA_WORKSPACE_ROOT (should fail) + assert_failure(&run_test_binary(&moved_binary_path, &moved_workspace, None)); + + // Run test in moved workspace with INSTA_WORKSPACE_ROOT (should pass) + assert_success(&run_test_binary( + &moved_binary_path, + &moved_workspace, + Some(("INSTA_WORKSPACE_ROOT", moved_workspace.to_str().unwrap())), + )); +} diff --git a/insta/src/env.rs b/insta/src/env.rs index 776ef8b0..63ea8c4e 100644 --- a/insta/src/env.rs +++ b/insta/src/env.rs @@ -12,18 +12,16 @@ use crate::{ lazy_static::lazy_static! { static ref WORKSPACES: Mutex>> = Mutex::new(BTreeMap::new()); - static ref TOOL_CONFIGS: Mutex>> = Mutex::new(BTreeMap::new()); + static ref TOOL_CONFIGS: Mutex>> = Mutex::new(BTreeMap::new()); } -pub fn get_tool_config(manifest_dir: &str) -> Arc { - let mut configs = TOOL_CONFIGS.lock().unwrap(); - if let Some(rv) = configs.get(manifest_dir) { - return rv.clone(); - } - let config = - Arc::new(ToolConfig::from_manifest_dir(manifest_dir).expect("failed to load tool config")); - configs.insert(manifest_dir.to_string(), config.clone()); - config +pub fn get_tool_config(workspace_dir: &Path) -> Arc { + TOOL_CONFIGS + .lock() + .unwrap() + .entry(workspace_dir.to_path_buf()) + .or_insert_with(|| ToolConfig::from_workspace(workspace_dir).unwrap().into()) + .clone() } /// The test runner to use. @@ -125,11 +123,6 @@ pub struct ToolConfig { } impl ToolConfig { - /// Loads the tool config for a specific manifest. - pub fn from_manifest_dir(manifest_dir: &str) -> Result { - ToolConfig::from_workspace(&get_cargo_workspace(manifest_dir)) - } - /// Loads the tool config from a cargo workspace. pub fn from_workspace(workspace_dir: &Path) -> Result { let mut cfg = None; @@ -411,43 +404,61 @@ pub fn snapshot_update_behavior(tool_config: &ToolConfig, unseen: bool) -> Snaps /// Returns the cargo workspace for a manifest pub fn get_cargo_workspace(manifest_dir: &str) -> Arc { - // we really do not care about poisoning here. - let mut workspaces = WORKSPACES.lock().unwrap_or_else(|x| x.into_inner()); - if let Some(rv) = workspaces.get(manifest_dir) { - rv.clone() - } else { - // If INSTA_WORKSPACE_ROOT environment variable is set, use the value - // as-is. This is useful for those users where the compiled in - // CARGO_MANIFEST_DIR points to some transient location. This can easily - // happen if the user builds the test in one directory but then tries to - // run it in another: even if sources are available in the new - // directory, in the past we would always go with the compiled-in value. - // The compiled-in directory may not even exist anymore. - let path = if let Ok(workspace_root) = std::env::var("INSTA_WORKSPACE_ROOT") { - Arc::new(PathBuf::from(workspace_root)) - } else { + // If INSTA_WORKSPACE_ROOT environment variable is set, use the value as-is. + // This is useful where CARGO_MANIFEST_DIR at compilation points to some + // transient location. This can easily happen when building the test in one + // directory but running it in another. + if let Ok(workspace_root) = env::var("INSTA_WORKSPACE_ROOT") { + return PathBuf::from(workspace_root).into(); + } + + let error_message = || { + format!( + "`cargo metadata --format-version=1 --no-deps` in path `{}`", + manifest_dir + ) + }; + + WORKSPACES + .lock() + // we really do not care about poisoning here. + .unwrap() + .entry(manifest_dir.to_string()) + .or_insert_with(|| { let output = std::process::Command::new( - env::var("CARGO") - .ok() - .unwrap_or_else(|| "cargo".to_string()), + env::var("CARGO").unwrap_or_else(|_| "cargo".to_string()), ) - .arg("metadata") - .arg("--format-version=1") - .arg("--no-deps") + .args(["metadata", "--format-version=1", "--no-deps"]) .current_dir(manifest_dir) .output() - .unwrap(); - let docs = crate::content::yaml::vendored::yaml::YamlLoader::load_from_str( + .unwrap_or_else(|e| panic!("failed to run {}\n\n{}", error_message(), e)); + + crate::content::yaml::vendored::yaml::YamlLoader::load_from_str( std::str::from_utf8(&output.stdout).unwrap(), ) - .unwrap(); - let manifest = docs.first().expect("Unable to parse cargo manifest"); - let workspace_root = PathBuf::from(manifest["workspace_root"].as_str().unwrap()); - Arc::new(workspace_root) - }; - workspaces.insert(manifest_dir.to_string(), path.clone()); - path - } + .map_err(|e| e.to_string()) + .and_then(|docs| { + docs.into_iter() + .next() + .ok_or_else(|| "No content found in yaml".to_string()) + }) + .and_then(|metadata| { + metadata["workspace_root"] + .clone() + .into_string() + .ok_or_else(|| "Couldn't find `workspace_root`".to_string()) + }) + .map(|path| PathBuf::from(path).into()) + .unwrap_or_else(|e| { + panic!( + "failed to parse cargo metadata output from {}: {}\n\n{:?}", + error_message(), + e, + output.stdout + ) + }) + }) + .clone() } #[cfg(feature = "_cargo_insta_internal")] diff --git a/insta/src/glob.rs b/insta/src/glob.rs index e901702d..faec3a8b 100644 --- a/insta/src/glob.rs +++ b/insta/src/glob.rs @@ -38,7 +38,7 @@ lazy_static::lazy_static! { }; } -pub fn glob_exec(manifest_dir: &str, base: &Path, pattern: &str, mut f: F) { +pub fn glob_exec(workspace_dir: &Path, base: &Path, pattern: &str, mut f: F) { // If settings.allow_empty_glob() == true and `base` doesn't exist, skip // everything. This is necessary as `base` is user-controlled via `glob!/3` // and may not exist. @@ -61,7 +61,7 @@ pub fn glob_exec(manifest_dir: &str, base: &Path, pattern: &str GLOB_STACK.lock().unwrap().push(GlobCollector { failed: 0, show_insta_hint: false, - fail_fast: get_tool_config(manifest_dir).glob_fail_fast(), + fail_fast: get_tool_config(workspace_dir).glob_fail_fast(), }); // step 1: collect all matching files diff --git a/insta/src/macros.rs b/insta/src/macros.rs index ba837e40..fd453acb 100644 --- a/insta/src/macros.rs +++ b/insta/src/macros.rs @@ -15,6 +15,19 @@ macro_rules! _function_name { }}; } +#[doc(hidden)] +#[macro_export] +macro_rules! _get_workspace_root { + () => {{ + use std::env; + + // Note the `env!("CARGO_MANIFEST_DIR")` needs to be in the macro (in + // contrast to a function in insta) because the macro needs to capture + // the value in the caller library, an exclusive property of macros. + $crate::_macro_support::get_cargo_workspace(env!("CARGO_MANIFEST_DIR")) + }}; +} + /// Asserts a `Serialize` snapshot in CSV format. /// /// **Feature:** `csv` (disabled by default) @@ -349,7 +362,7 @@ macro_rules! _assert_snapshot_base { $name.into(), #[allow(clippy::redundant_closure_call)] &$transform(&$value), - env!("CARGO_MANIFEST_DIR"), + $crate::_get_workspace_root!().as_path(), $crate::_function_name!(), module_path!(), file!(), @@ -485,9 +498,13 @@ macro_rules! with_settings { #[cfg_attr(docsrs, doc(cfg(feature = "glob")))] #[macro_export] macro_rules! glob { + // TODO: I think we could remove the three-argument version of this macro + // and just support a pattern such as + // `glob!("../test_data/inputs/*.txt"...`. ($base_path:expr, $glob:expr, $closure:expr) => {{ use std::path::Path; - let base = $crate::_macro_support::get_cargo_workspace(env!("CARGO_MANIFEST_DIR")) + + let base = $crate::_get_workspace_root!() .join(Path::new(file!()).parent().unwrap()) .join($base_path) .to_path_buf(); @@ -495,7 +512,12 @@ macro_rules! glob { // we try to canonicalize but on some platforms (eg: wasm) that might not work, so // we instead silently fall back. let base = base.canonicalize().unwrap_or_else(|_| base); - $crate::_macro_support::glob_exec(env!("CARGO_MANIFEST_DIR"), &base, $glob, $closure); + $crate::_macro_support::glob_exec( + $crate::_get_workspace_root!().as_path(), + &base, + $glob, + $closure, + ); }}; ($glob:expr, $closure:expr) => {{ diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index c2c363ed..4b5eba60 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -8,14 +8,14 @@ use std::str; use std::sync::{Arc, Mutex}; use std::{borrow::Cow, env}; -use crate::output::SnapshotPrinter; use crate::settings::Settings; use crate::snapshot::{MetaData, PendingInlineSnapshot, Snapshot, SnapshotContents}; use crate::utils::{path_to_storage, style}; +use crate::{env::get_tool_config, output::SnapshotPrinter}; use crate::{ env::{ - get_cargo_workspace, get_tool_config, memoize_snapshot_file, snapshot_update_behavior, - OutputBehavior, SnapshotUpdateBehavior, ToolConfig, + memoize_snapshot_file, snapshot_update_behavior, OutputBehavior, SnapshotUpdateBehavior, + ToolConfig, }, snapshot::SnapshotKind, }; @@ -218,7 +218,7 @@ fn get_snapshot_filename( #[derive(Debug)] struct SnapshotAssertionContext<'a> { tool_config: Arc, - cargo_workspace: Arc, + workspace: &'a Path, module_path: &'a str, snapshot_name: Option>, snapshot_file: Option, @@ -233,14 +233,13 @@ struct SnapshotAssertionContext<'a> { impl<'a> SnapshotAssertionContext<'a> { fn prepare( refval: ReferenceValue<'a>, - manifest_dir: &'a str, + workspace: &'a Path, function_name: &'a str, module_path: &'a str, assertion_file: &'a str, assertion_line: u32, ) -> Result, Box> { - let tool_config = get_tool_config(manifest_dir); - let cargo_workspace = get_cargo_workspace(manifest_dir); + let tool_config = get_tool_config(workspace); let snapshot_name; let mut duplication_key = None; let mut snapshot_file = None; @@ -268,7 +267,7 @@ impl<'a> SnapshotAssertionContext<'a> { module_path, assertion_file, &name, - &cargo_workspace, + workspace, assertion_file, is_doctest, ); @@ -290,7 +289,7 @@ impl<'a> SnapshotAssertionContext<'a> { snapshot_name = detect_snapshot_name(function_name, module_path) .ok() .map(Cow::Owned); - let mut pending_file = cargo_workspace.join(assertion_file); + let mut pending_file = workspace.join(assertion_file); pending_file.set_file_name(format!( ".{}.pending-snap", pending_file @@ -311,7 +310,7 @@ impl<'a> SnapshotAssertionContext<'a> { Ok(SnapshotAssertionContext { tool_config, - cargo_workspace, + workspace, module_path, snapshot_name, snapshot_file, @@ -326,8 +325,8 @@ impl<'a> SnapshotAssertionContext<'a> { /// Given a path returns the local path within the workspace. pub fn localize_path(&self, p: &Path) -> Option { - let workspace = self.cargo_workspace.canonicalize().ok()?; - let p = self.cargo_workspace.join(p).canonicalize().ok()?; + let workspace = self.workspace.canonicalize().ok()?; + let p = self.workspace.join(p).canonicalize().ok()?; p.strip_prefix(&workspace).ok().map(|x| x.to_path_buf()) } @@ -454,11 +453,8 @@ impl<'a> SnapshotAssertionContext<'a> { /// 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, - ); + let mut printer = + SnapshotPrinter::new(self.workspace, self.old_snapshot.as_ref(), new_snapshot); printer.set_line(Some(self.assertion_line)); printer.set_snapshot_file(self.snapshot_file.as_deref()); printer.set_title(Some("Snapshot Summary")); @@ -572,8 +568,7 @@ fn record_snapshot_duplicate( if let Some(prev_snapshot) = results.get(key) { if prev_snapshot.contents() != snapshot.contents() { println!("Snapshots in allow-duplicates block do not match."); - let mut printer = - SnapshotPrinter::new(ctx.cargo_workspace.as_path(), Some(prev_snapshot), snapshot); + let mut printer = SnapshotPrinter::new(ctx.workspace, Some(prev_snapshot), snapshot); printer.set_line(Some(ctx.assertion_line)); printer.set_snapshot_file(ctx.snapshot_file.as_deref()); printer.set_title(Some("Differences in Block")); @@ -623,7 +618,7 @@ where pub fn assert_snapshot( refval: ReferenceValue, new_snapshot_value: &str, - manifest_dir: &str, + workspace: &Path, function_name: &str, module_path: &str, assertion_file: &str, @@ -632,7 +627,7 @@ pub fn assert_snapshot( ) -> Result<(), Box> { let ctx = SnapshotAssertionContext::prepare( refval, - manifest_dir, + workspace, function_name, module_path, assertion_file, From 628404675ef2b4988506a40c003d34b9e9b315c6 Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Fri, 27 Sep 2024 12:11:22 -0700 Subject: [PATCH 13/19] Add test for inline snapshot with a debug expr (#624) --- insta/tests/test_inline.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/insta/tests/test_inline.rs b/insta/tests/test_inline.rs index bdee1fed..fe936908 100644 --- a/insta/tests/test_inline.rs +++ b/insta/tests/test_inline.rs @@ -61,6 +61,11 @@ fn test_newline() { "###); } +#[test] +fn test_inline_debug_expr() { + assert_snapshot!("hello", "a debug expr", @"hello"); +} + #[cfg(feature = "csv")] #[test] fn test_csv_inline() { From ee829d825b1ff561a4696f42f7d3a7a0b9860b62 Mon Sep 17 00:00:00 2001 From: Frank Elsinga Date: Fri, 27 Sep 2024 21:26:09 +0200 Subject: [PATCH 14/19] docs: improved the linking inside and outside of the crate (#618) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While browsing the docs on docs.rs, I noticed that there are a few items which are not linked. This PR tries to improve this situation. Drawing attention to one thing which might be controversial: In the current codebase, the usage of `[´foo´](Self::foo)` and `[´Self::foo´]` (and related links) are a bit inconsistent. I have changed all to the `[´Self::foo´]` format. Please let me know if you prefer the first and I can change them to that format. I have verified this change via adding the following (somewhat agressive) docs-related lints and running `RUSTDOCFLAGS="--cfg=docsrs" cargo +nightly doc --no-deps --all-features` and `cargo clippy --all-features --all`. ```rust #![warn(clippy::doc_markdown)] #![warn(rustdoc::all)] ``` --------- Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> --- cargo-insta/src/main.rs | 3 ++ cargo-insta/tests/main.rs | 2 +- insta/src/content/json.rs | 2 +- insta/src/content/mod.rs | 10 ++--- insta/src/content/serialization.rs | 2 +- insta/src/content/yaml/vendored/emitter.rs | 5 ++- insta/src/content/yaml/vendored/parser.rs | 6 +-- insta/src/content/yaml/vendored/yaml.rs | 4 +- insta/src/env.rs | 2 +- insta/src/lib.rs | 36 +++++++++------- insta/src/macros.rs | 50 +++++++++++----------- insta/src/redaction.rs | 2 +- insta/src/runtime.rs | 1 + insta/src/settings.rs | 22 +++++----- insta/src/snapshot.rs | 2 +- 15 files changed, 80 insertions(+), 69 deletions(-) diff --git a/cargo-insta/src/main.rs b/cargo-insta/src/main.rs index 033b5706..487b35a5 100644 --- a/cargo-insta/src/main.rs +++ b/cargo-insta/src/main.rs @@ -1,3 +1,6 @@ +#![warn(clippy::doc_markdown)] +#![warn(rustdoc::all)] + //!
//! //!

cargo-insta: review tool for insta, a snapshot testing library for Rust

diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index 38b3a6ea..37e4dd66 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -60,7 +60,7 @@ impl TestFiles { } } -/// Path of the insta crate in this repo, which we use as a dependency in the test project +/// Path of the [`insta`] crate in this repo, which we use as a dependency in the test project fn insta_path() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")) .parent() diff --git a/insta/src/content/json.rs b/insta/src/content/json.rs index 2141226a..e4dd5e67 100644 --- a/insta/src/content/json.rs +++ b/insta/src/content/json.rs @@ -29,7 +29,7 @@ pub struct Serializer { } impl Serializer { - /// Creates a new serializer that writes into the given writer. + /// Creates a new [`Serializer`] that writes into the given writer. pub fn new() -> Serializer { Serializer { out: String::new(), diff --git a/insta/src/content/mod.rs b/insta/src/content/mod.rs index 8fe5e14e..1325f8f1 100644 --- a/insta/src/content/mod.rs +++ b/insta/src/content/mod.rs @@ -57,7 +57,7 @@ impl std::error::Error for Error {} /// /// Some enum variants are intentionally not exposed to user code. /// It's generally recommended to construct content objects by -/// using the [`From`](std::convert::From) trait and by using the +/// using the [`From`] trait and by using the /// accessor methods to assert on it. /// /// While matching on the content is possible in theory it is @@ -65,12 +65,12 @@ impl std::error::Error for Error {} /// enum holds variants that can "wrap" values where it's not /// expected. For instance if a field holds an `Option` /// you cannot use pattern matching to extract the string as it -/// will be contained in an internal `Some` variant that is not -/// exposed. On the other hand the `as_str` method will +/// will be contained in an internal [`Some`] variant that is not +/// exposed. On the other hand the [`Content::as_str`] method will /// automatically resolve such internal wrappers. /// /// If you do need to pattern match you should use the -/// `resolve_inner` method to resolve such internal wrappers. +/// [`Content::resolve_inner`] method to resolve such internal wrappers. #[derive(Debug, Clone, PartialEq, PartialOrd)] pub enum Content { Bool(bool), @@ -196,7 +196,7 @@ impl Content { } } - /// Mutable version of [`resolve_inner`](Self::resolve_inner). + /// Mutable version of [`Self::resolve_inner`]. pub fn resolve_inner_mut(&mut self) -> &mut Content { match *self { Content::Some(ref mut v) diff --git a/insta/src/content/serialization.rs b/insta/src/content/serialization.rs index b0eeea6d..e9223e2a 100644 --- a/insta/src/content/serialization.rs +++ b/insta/src/content/serialization.rs @@ -19,7 +19,7 @@ pub enum Key<'a> { } impl<'a> Key<'a> { - /// Needed because std::mem::discriminant is not Ord + /// Needed because [`std::mem::discriminant`] is not [`Ord`] fn discriminant(&self) -> usize { match self { Key::Bool(_) => 1, diff --git a/insta/src/content/yaml/vendored/emitter.rs b/insta/src/content/yaml/vendored/emitter.rs index 7c8260f8..2ea1f2c2 100644 --- a/insta/src/content/yaml/vendored/emitter.rs +++ b/insta/src/content/yaml/vendored/emitter.rs @@ -38,7 +38,7 @@ pub struct YamlEmitter<'a> { pub type EmitResult = Result<(), EmitError>; -// from serialize::json +/// From [`serialize::json`] fn escape_str(wr: &mut dyn fmt::Write, v: &str) -> Result<(), fmt::Error> { wr.write_str("\"")?; @@ -248,12 +248,13 @@ impl<'a> YamlEmitter<'a> { } } +#[allow(clippy::doc_markdown)] // \` is recognised as unbalanced backticks /// Check if the string requires quoting. /// /// Strings starting with any of the following characters must be quoted. /// `:`, `&`, `*`, `?`, `|`, `-`, `<`, `>`, `=`, `!`, `%`, `@` /// Strings containing any of the following characters must be quoted. -/// `{`, `}`, `[`, `]`, `,`, `#`, `\`` +/// `{`, `}`, `\[`, `\]`, `,`, `#`, `\`` /// /// If the string contains any of the following control characters, it must be escaped with double quotes: /// `\0`, `\x01`, `\x02`, `\x03`, `\x04`, `\x05`, `\x06`, `\a`, `\b`, `\t`, `\n, `\v, `\f`, `\r`, `\x0e`, `\x0f`, `\x10`, `\x11`, `\x12`, `\x13`, `\x14`, `\x15`, `\x16`, `\x17`, `\x18`, `\x19`, `\x1a`, `\e`, `\x1c`, `\x1d`, `\x1e`, `\x1f`, `\N`, `\_`, `\L`, `\P` diff --git a/insta/src/content/yaml/vendored/parser.rs b/insta/src/content/yaml/vendored/parser.rs index 1656bba9..b21ecb07 100644 --- a/insta/src/content/yaml/vendored/parser.rs +++ b/insta/src/content/yaml/vendored/parser.rs @@ -29,8 +29,8 @@ enum State { End, } -/// `Event` is used with the low-level event base parsing API, -/// see `EventReceiver` trait. +/// [`Event`] is used with the low-level event base parsing API, +/// see [`EventReceiver`] trait. #[derive(Clone, PartialEq, Debug, Eq)] pub enum Event { /// Reserved for internal use @@ -40,7 +40,7 @@ pub enum Event { DocumentEnd, /// Refer to an anchor ID Alias(usize), - /// Value, style, anchor_id, tag + /// Value, style, anchor ID, tag Scalar(String, TScalarStyle, usize, Option), /// Anchor ID SequenceStart(usize), diff --git a/insta/src/content/yaml/vendored/yaml.rs b/insta/src/content/yaml/vendored/yaml.rs index df502b2b..96787f76 100644 --- a/insta/src/content/yaml/vendored/yaml.rs +++ b/insta/src/content/yaml/vendored/yaml.rs @@ -14,8 +14,8 @@ use std::vec; /// access your YAML document. #[derive(Clone, PartialEq, PartialOrd, Debug, Eq, Ord, Hash)] pub enum Yaml { - /// Float types are stored as String and parsed on demand. - /// Note that f64 does NOT implement Eq trait and can NOT be stored in BTreeMap. + /// Float types are stored as [`String`] and parsed on demand. + /// Note that [`f64'] does NOT implement [`Eq'] trait and can NOT be stored in [`BTreeMap`]. Real(string::String), /// YAML int is stored as i64. Integer(i64), diff --git a/insta/src/env.rs b/insta/src/env.rs index 63ea8c4e..14f02b93 100644 --- a/insta/src/env.rs +++ b/insta/src/env.rs @@ -321,7 +321,7 @@ impl ToolConfig { self.snapshot_update } - /// Returns the value of glob_fail_fast + /// Returns whether the glob should fail fast, as snapshot failures within the glob macro will appear only at the end of execution unless `glob_fail_fast` is set. #[cfg(feature = "glob")] pub fn glob_fail_fast(&self) -> bool { self.glob_fail_fast diff --git a/insta/src/lib.rs b/insta/src/lib.rs index 0d115ecf..142256b2 100644 --- a/insta/src/lib.rs +++ b/insta/src/lib.rs @@ -1,3 +1,6 @@ +#![warn(clippy::doc_markdown)] +#![warn(rustdoc::all)] + //!
//! //!

insta: a snapshot testing library for Rust

@@ -7,7 +10,7 @@ //! //! Snapshots tests (also sometimes called approval tests) are tests that //! assert values against a reference value (the snapshot). This is similar -//! to how `assert_eq!` lets you compare a value against a reference value but +//! to how [`assert_eq!`] lets you compare a value against a reference value but //! unlike simple string assertions, snapshot tests let you test against complex //! values and come with comprehensive tools to review changes. //! @@ -81,7 +84,7 @@ //! [`Display`](std::fmt::Display) outputs, often strings. //! - [`assert_debug_snapshot!`] for comparing [`Debug`] outputs of values. //! -//! The following macros require the use of serde's [`Serialize`](serde::Serialize): +//! The following macros require the use of [`serde::Serialize`]: //! #![cfg_attr( feature = "csv", @@ -171,11 +174,11 @@ //! //! The following features exist: //! -//! * `csv`: enables CSV support (via serde) -//! * `json`: enables JSON support (via serde) -//! * `ron`: enables RON support (via serde) -//! * `toml`: enables TOML support (via serde) -//! * `yaml`: enables YAML support (via serde) +//! * `csv`: enables CSV support (via [`serde`]) +//! * `json`: enables JSON support (via [`serde`]) +//! * `ron`: enables RON support (via [`serde`]) +//! * `toml`: enables TOML support (via [`serde`]) +//! * `yaml`: enables YAML support (via [`serde`]) //! * `redactions`: enables support for redactions //! * `filters`: enables support for filters //! * `glob`: enables support for globbing ([`glob!`]) @@ -185,13 +188,14 @@ //! limited capacity. You will receive a deprecation warning if you are not //! opting into them but for now the macros will continue to function. //! -//! Enabling any of the serde based formats enables the hidden `serde` feature -//! which gates some serde specific APIs such as [`Settings::set_info`]. +//! Enabling any of the [`serde`] based formats enables the hidden `serde` feature +//! which gates some [`serde`] specific APIs such as [`Settings::set_info`]. //! //! # Dependencies //! -//! `insta` tries to be light in dependencies but this is tricky to accomplish -//! given what it tries to do. By default, it currently depends on `serde` for +//! [`insta`] tries to be light in dependencies but this is tricky to accomplish +//! given what it tries to do. +//! By default, it currently depends on [`serde`] for //! the [`assert_toml_snapshot!`] and [`assert_yaml_snapshot!`] macros. In the //! future this default dependencies will be removed. To already benefit from //! this optimization you can disable the default features and manually opt into @@ -202,7 +206,7 @@ //! There are some settings that can be changed on a per-thread (and thus //! per-test) basis. For more information see [Settings]. //! -//! Additionally Insta will load a YAML config file with settings that change +//! Additionally, Insta will load a YAML config file with settings that change //! the behavior of insta between runs. It's loaded from any of the following //! locations: `.config/insta.yaml`, `insta.yaml` and `.insta.yaml` from the //! workspace root. The following config options exist: @@ -247,8 +251,8 @@ //! //! Insta benefits from being compiled in release mode, even as dev dependency. //! It will compile slightly slower once, but use less memory, have faster diffs -//! and just generally be more fun to use. To achieve that, opt `insta` and -//! `similar` (the diffing library) into higher optimization in your +//! and just generally be more fun to use. To achieve that, opt [`insta`] and +//! [`similar`] (the diffing library) into higher optimization in your //! `Cargo.toml`: //! //! ```yaml @@ -259,8 +263,10 @@ //! opt-level = 3 //! ``` //! -//! You can also disable the default features of `insta` which will cut down on +//! You can also disable the default features of [`insta`] which will cut down on //! the compile time a bit by removing some quality of life features. +//! +//! [`insta`]: https://docs.rs/insta #![cfg_attr(docsrs, feature(doc_cfg))] #[macro_use] diff --git a/insta/src/macros.rs b/insta/src/macros.rs index fd453acb..222ef3c6 100644 --- a/insta/src/macros.rs +++ b/insta/src/macros.rs @@ -28,11 +28,11 @@ macro_rules! _get_workspace_root { }}; } -/// Asserts a `Serialize` snapshot in CSV format. +/// Asserts a [`serde::Serialize`] snapshot in CSV format. /// /// **Feature:** `csv` (disabled by default) /// -/// This works exactly like [`crate::assert_yaml_snapshot!`] +/// This works exactly like [`assert_yaml_snapshot!`](crate::assert_yaml_snapshot!) /// but serializes in [CSV](https://github.com/burntsushi/rust-csv) format instead of /// YAML. /// @@ -57,11 +57,11 @@ macro_rules! assert_csv_snapshot { }; } -/// Asserts a `Serialize` snapshot in TOML format. +/// Asserts a [`serde::Serialize`] snapshot in TOML format. /// /// **Feature:** `toml` (disabled by default) /// -/// This works exactly like [`crate::assert_yaml_snapshot!`] +/// This works exactly like [`assert_yaml_snapshot!`](crate::assert_yaml_snapshot!) /// but serializes in [TOML](https://github.com/alexcrichton/toml-rs) format instead of /// YAML. Note that TOML cannot represent all values due to limitations in the /// format. @@ -87,14 +87,14 @@ macro_rules! assert_toml_snapshot { }; } -/// Asserts a `Serialize` snapshot in YAML format. +/// Asserts a [`serde::Serialize`] snapshot in YAML format. /// /// **Feature:** `yaml` /// -/// The value needs to implement the `serde::Serialize` trait and the snapshot +/// The value needs to implement the [`serde::Serialize`] trait and the snapshot /// will be serialized in YAML format. This does mean that unlike the debug /// snapshot variant the type of the value does not appear in the output. -/// You can however use the `assert_ron_snapshot!` macro to dump out +/// You can however use the [`assert_ron_snapshot!`](crate::assert_ron_snapshot!) macro to dump out /// the value in [RON](https://github.com/ron-rs/ron/) format which retains some /// type information for more accurate comparisons. /// @@ -105,7 +105,7 @@ macro_rules! assert_toml_snapshot { /// assert_yaml_snapshot!(vec![1, 2, 3]); /// ``` /// -/// Unlike the [`crate::assert_debug_snapshot!`] +/// Unlike the [`assert_debug_snapshot!`](crate::assert_debug_snapshot!) /// macro, this one has a secondary mode where redactions can be defined. /// /// The third argument to the macro can be an object expression for redaction. @@ -141,11 +141,11 @@ macro_rules! assert_yaml_snapshot { }; } -/// Asserts a `Serialize` snapshot in RON format. +/// Asserts a [`serde::Serialize`] snapshot in RON format. /// /// **Feature:** `ron` (disabled by default) /// -/// This works exactly like [`assert_yaml_snapshot!`] +/// This works exactly like [`assert_yaml_snapshot!`](crate::assert_yaml_snapshot!) /// but serializes in [RON](https://github.com/ron-rs/ron/) format instead of /// YAML which retains some type information for more accurate comparisons. /// @@ -171,11 +171,11 @@ macro_rules! assert_ron_snapshot { }; } -/// Asserts a `Serialize` snapshot in JSON format. +/// Asserts a [`serde::Serialize`] snapshot in JSON format. /// /// **Feature:** `json` /// -/// This works exactly like [`assert_yaml_snapshot!`] but serializes in JSON format. +/// This works exactly like [`assert_yaml_snapshot!`](crate::assert_yaml_snapshot!) but serializes in JSON format. /// This is normally not recommended because it makes diffs less reliable, but it can /// be useful for certain specialized situations. /// @@ -201,11 +201,11 @@ macro_rules! assert_json_snapshot { }; } -/// Asserts a `Serialize` snapshot in compact JSON format. +/// Asserts a [`serde::Serialize`] snapshot in compact JSON format. /// /// **Feature:** `json` /// -/// This works exactly like [`assert_json_snapshot!`] but serializes into a single +/// This works exactly like [`assert_json_snapshot!`](crate::assert_json_snapshot!) but serializes into a single /// line for as long as the output is less than 120 characters. This can be useful /// in cases where you are working with small result outputs but comes at the cost /// of slightly worse diffing behavior. @@ -297,10 +297,10 @@ macro_rules! _prepare_snapshot_for_redaction { }; } -/// Asserts a `Debug` snapshot. +/// Asserts a [`Debug`] snapshot. /// -/// The value needs to implement the `fmt::Debug` trait. This is useful for -/// simple values that do not implement the `Serialize` trait, but does not +/// The value needs to implement the [`Debug`] trait. This is useful for +/// simple values that do not implement the [`serde::Serialize`] trait, but does not /// permit redactions. /// /// Debug is called with `"{:#?}"`, which means this uses pretty-print. @@ -311,10 +311,10 @@ macro_rules! assert_debug_snapshot { }; } -/// Asserts a `Debug` snapshot in compact format. +/// Asserts a [`Debug`] snapshot in compact format. /// -/// The value needs to implement the `fmt::Debug` trait. This is useful for -/// simple values that do not implement the `Serialize` trait, but does not +/// The value needs to implement the [`Debug`] trait. This is useful for +/// simple values that do not implement the [`serde::Serialize`] trait, but does not /// permit redactions. /// /// Debug is called with `"{:?}"`, which means this does not use pretty-print. @@ -373,9 +373,9 @@ macro_rules! _assert_snapshot_base { }; } -/// Asserts a `Display` snapshot. +/// Asserts a [`Display`](std::fmt::Display) snapshot. /// -/// This is now deprecated, replaced by the more generic `assert_snapshot!()` +/// This is now deprecated, replaced by the more generic [`assert_snapshot!`](crate::assert_snapshot!) #[macro_export] #[deprecated = "use assert_snapshot!() instead"] macro_rules! assert_display_snapshot { @@ -384,10 +384,10 @@ macro_rules! assert_display_snapshot { }; } -/// Asserts a string snapshot. +/// Asserts a [`String`] snapshot. /// -/// This is the simplest of all assertion methods. It accepts any value that -/// implements `fmt::Display`. +/// This is the simplest of all assertion methods. +/// It accepts any value that implements [`Display`](std::fmt::Display). /// /// ```no_run /// # use insta::*; diff --git a/insta/src/redaction.rs b/insta/src/redaction.rs index 525b3294..423e7fec 100644 --- a/insta/src/redaction.rs +++ b/insta/src/redaction.rs @@ -136,7 +136,7 @@ where /// /// This is useful to force something like a set or map to be ordered to make /// it deterministic. This is necessary as insta's serialization support is -/// based on serde which does not have native set support. As a result vectors +/// based on [`serde`] which does not have native set support. As a result vectors /// (which need to retain order) and sets (which should be given a stable order) /// look the same. /// diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 4b5eba60..3f9c8f6a 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -701,6 +701,7 @@ pub fn assert_snapshot( Ok(()) } +#[allow(rustdoc::private_doc_tests)] /// Test snapshots in doctests. /// /// ``` diff --git a/insta/src/settings.rs b/insta/src/settings.rs index 74048818..1a1f4425 100644 --- a/insta/src/settings.rs +++ b/insta/src/settings.rs @@ -176,7 +176,7 @@ impl Default for Settings { impl Settings { /// Returns the default settings. /// - /// It's recommended to use `clone_current` instead so that + /// It's recommended to use [`Self::clone_current`] instead so that /// already applied modifications are not discarded. pub fn new() -> Settings { Settings::default() @@ -196,7 +196,7 @@ impl Settings { /// Enables forceful sorting of maps before serialization. /// /// Note that this only applies to snapshots that undergo serialization - /// (eg: does not work for `assert_debug_snapshot!`.) + /// (eg: does not work for [`assert_debug_snapshot!`](crate::assert_debug_snapshot!).) /// /// The default value is `false`. pub fn set_sort_maps(&mut self, value: bool) { @@ -295,7 +295,7 @@ impl Settings { /// super useful by itself, particularly when working with loops and generated /// tests. In that case the `description` can be set as extra information. /// - /// See also [`set_info`](Self::set_info). + /// See also [`Self::set_info`]. pub fn set_description>(&mut self, value: S) { self._private_inner_mut().description(value); } @@ -320,7 +320,7 @@ impl Settings { /// As an example the input parameters to the function that creates the snapshot /// can be persisted here. /// - /// Alternatively you can use [`set_raw_info`](Self::set_raw_info) instead. + /// Alternatively you can use [`Self::set_raw_info`] instead. #[cfg(feature = "serde")] #[cfg_attr(docsrs, doc(cfg(feature = "serde")))] pub fn set_info(&mut self, s: &S) { @@ -329,7 +329,7 @@ impl Settings { /// Sets the info from a content object. /// - /// This works like [`set_info`](Self::set_info) but does not require `serde`. + /// This works like [`Self::set_info`] but does not require [`serde`]. pub fn set_raw_info(&mut self, content: &Content) { self._private_inner_mut().raw_info(content); } @@ -365,7 +365,7 @@ impl Settings { /// snapshots. /// /// Note that this only applies to snapshots that undergo serialization - /// (eg: does not work for `assert_debug_snapshot!`.) + /// (eg: does not work for [`assert_debug_snapshot!`](crate::assert_debug_snapshot!).) #[cfg(feature = "redactions")] #[cfg_attr(docsrs, doc(cfg(feature = "redactions")))] pub fn add_redaction>(&mut self, selector: &str, replacement: R) { @@ -384,7 +384,7 @@ impl Settings { /// /// This works similar to a redaction but instead of changing the value it /// asserts the value at a certain place. This function is internally - /// supposed to call things like `assert_eq!`. + /// supposed to call things like [`assert_eq!`]. /// /// This is a shortcut to `add_redaction(selector, dynamic_redaction(...))`; #[cfg(feature = "redactions")] @@ -437,7 +437,7 @@ impl Settings { /// /// The first argument is the [`regex`] pattern to apply, the second is a replacement /// string. The replacement string has the same functionality as the second argument - /// to [`Regex::replace`](regex::Regex::replace). + /// to [`regex::Regex::replace`]. /// /// This is useful to perform some cleanup procedures on the snapshot for unstable values. /// @@ -493,7 +493,7 @@ impl Settings { /// Runs a function with the current settings bound to the thread. /// - /// This is an alternative to [`bind_to_scope`](Settings::bind_to_scope) + /// This is an alternative to [`Self::bind_to_scope`]() /// which does not require holding on to a drop guard. The return value /// of the closure is passed through. /// @@ -510,7 +510,7 @@ impl Settings { f() } - /// Like `bind` but for futures. + /// Like [`Self::bind`] but for futures. /// /// This lets you bind settings for the duration of a future like this: /// @@ -579,7 +579,7 @@ impl Settings { } } -/// Returned from [`bind_to_scope`](Settings::bind_to_scope) +/// Returned from [`Settings::bind_to_scope`] #[must_use = "The guard is immediately dropped so binding has no effect. Use `let _guard = ...` to bind it."] pub struct SettingsBindDropGuard(Option>); diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index 74e6ac12..f9928c78 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -504,7 +504,7 @@ impl Snapshot { self.save_with_metadata(path, &self.metadata.trim_for_persistence()) } - /// Same as `save` but instead of writing a normal snapshot file this will write + /// Same as [`Self::save`] but instead of writing a normal snapshot file this will write /// a `.snap.new` file with additional information. /// /// The name of the new snapshot file is returned. From 6318b6abee654e1537b764d459e86d00ed81ce6c Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Mon, 30 Sep 2024 23:06:15 -0700 Subject: [PATCH 15/19] Canonicalize snapshot paths before reducing (#626) Fixes https://github.com/mitsuhiko/insta/issues/625 As ever, the reason is a bit different from the one I initially understood. It's actually that we're reducing the paths by whether one prefixes the other. But in this case the path is only a prefix before it's canonicalized! On the upside, this case will now work without `--workspace-root`. --- cargo-insta/Cargo.toml | 1 + cargo-insta/src/cargo.rs | 28 ++++++---- cargo-insta/tests/main.rs | 113 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 10 deletions(-) diff --git a/cargo-insta/Cargo.toml b/cargo-insta/Cargo.toml index 06954a72..2258d64f 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 } +itertools = "0.10.0" [dev-dependencies] walkdir = "2.3.1" diff --git a/cargo-insta/src/cargo.rs b/cargo-insta/src/cargo.rs index 15e2f07f..7e15f890 100644 --- a/cargo-insta/src/cargo.rs +++ b/cargo-insta/src/cargo.rs @@ -1,9 +1,11 @@ use std::path::PathBuf; pub(crate) use cargo_metadata::Package; +use itertools::Itertools; /// Find snapshot roots within a package -// (I'm not sure how necessary this is; relative to just using all paths?) +// We need this because paths are not always conventional — for example cargo +// can reference artifacts that are outside of the package root. pub(crate) fn find_snapshot_roots(package: &Package) -> Vec { let mut roots = Vec::new(); @@ -38,13 +40,19 @@ pub(crate) fn find_snapshot_roots(package: &Package) -> Vec { // we would encounter paths twice. If we don't skip them here we run // into issues where the existence of a build script causes a snapshot // to be picked up twice since the same path is determined. (GH-15) - roots.sort_by_key(|x| x.as_os_str().len()); - let mut reduced_roots = vec![]; - for root in roots { - if !reduced_roots.iter().any(|x| root.starts_with(x)) { - reduced_roots.push(root); - } - } - - reduced_roots + let canonical_roots: Vec<_> = roots + .iter() + .filter_map(|x| x.canonicalize().ok()) + .sorted_by_key(|x| x.as_os_str().len()) + .collect(); + + canonical_roots + .clone() + .into_iter() + .filter(|root| { + !canonical_roots + .iter() + .any(|x| root.starts_with(x) && root != x) + }) + .collect() } diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index 37e4dd66..e96cd083 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -1303,3 +1303,116 @@ fn test_insta_workspace_root() { Some(("INSTA_WORKSPACE_ROOT", moved_workspace.to_str().unwrap())), )); } + +#[test] +fn test_external_test_path() { + let test_project = TestFiles::new() + .add_file( + "proj/Cargo.toml", + r#" +[package] +name = "external_test_path" +version = "0.1.0" +edition = "2021" + +[dependencies] +insta = { path = '$PROJECT_PATH' } + +[[test]] +name = "tlib" +path = "../tests/lib.rs" +"# + .to_string(), + ) + .add_file( + "proj/src/lib.rs", + r#" +pub fn hello() -> String { + "Hello, world!".to_string() +} +"# + .to_string(), + ) + .add_file( + "tests/lib.rs", + r#" +use external_test_path::hello; + +#[test] +fn test_hello() { + insta::assert_snapshot!(hello()); +} +"# + .to_string(), + ) + .create_project(); + + // Change to the proj directory for running cargo commands + let proj_dir = test_project.workspace_dir.join("proj"); + + // Initially, the test should fail + let output = test_project + .cmd() + .current_dir(&proj_dir) + .args(["test", "--"]) + .output() + .unwrap(); + + assert_failure(&output); + + // Verify that the snapshot was created in the correct location + assert_snapshot!(TestProject::current_file_tree(&test_project.workspace_dir), @r" + proj + proj/Cargo.lock + proj/Cargo.toml + proj/src + proj/src/lib.rs + tests + tests/lib.rs + tests/snapshots + tests/snapshots/tlib__hello.snap.new + "); + + // Run cargo insta accept + let output = test_project + .cmd() + .current_dir(&proj_dir) + .args(["test", "--accept"]) + .output() + .unwrap(); + + assert_success(&output); + + // Verify that the snapshot was created in the correct location + assert_snapshot!(TestProject::current_file_tree(&test_project.workspace_dir), @r" + proj + proj/Cargo.lock + proj/Cargo.toml + proj/src + proj/src/lib.rs + tests + tests/lib.rs + tests/snapshots + tests/snapshots/tlib__hello.snap + "); + + // Run the test again, it should pass now + let output = Command::new(env!("CARGO_BIN_EXE_cargo-insta")) + .current_dir(&proj_dir) + .args(["test"]) + .output() + .unwrap(); + + assert_success(&output); + + let snapshot_path = test_project + .workspace_dir + .join("tests/snapshots/tlib__hello.snap"); + assert_snapshot!(fs::read_to_string(snapshot_path).unwrap(), @r#" + --- + source: "../tests/lib.rs" + expression: hello() + --- + Hello, world! + "#); +} From 98842e70c6611fcb8afa3f99a9a2a67007cf4744 Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Mon, 30 Sep 2024 23:11:56 -0700 Subject: [PATCH 16/19] Improve output of integration tests when debugging (#627) --- cargo-insta/tests/main.rs | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index e96cd083..f8d8247b 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -12,6 +12,9 @@ /// `--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. +/// - If a test fails on `assert_success` or `assert_failure`, we print the +/// output of the inner test, bypassing the `--nocapture` flag. We color it to +/// discriminate it from the output of the outer test. /// /// 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 @@ -80,36 +83,26 @@ fn target_dir() -> PathBuf { } fn assert_success(output: &std::process::Output) { - // 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{}", - stdout, - stderr + // 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? + format_args!("{}", style(String::from_utf8_lossy(&output.stdout)).green()), + format_args!("{}", style(String::from_utf8_lossy(&output.stderr)).red()) ); - - // 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) { - eprint!("{}", String::from_utf8_lossy(&output.stderr)); - eprint!("{}", String::from_utf8_lossy(&output.stdout)); assert!( !output.status.success(), "Tests unexpectedly succeeded: {}\n{}", - String::from_utf8_lossy(&output.stdout), - String::from_utf8_lossy(&output.stderr) + // 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? + format_args!("{}", style(String::from_utf8_lossy(&output.stdout)).green()), + format_args!("{}", style(String::from_utf8_lossy(&output.stderr)).red()) ); } struct TestProject { From bf776f54d52d78a1865a27fe126a152d31dd9945 Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Mon, 30 Sep 2024 23:15:15 -0700 Subject: [PATCH 17/19] Remove duplicate arg in internal function (#628) --- insta/src/runtime.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/insta/src/runtime.rs b/insta/src/runtime.rs index 3f9c8f6a..e852cf28 100644 --- a/insta/src/runtime.rs +++ b/insta/src/runtime.rs @@ -174,11 +174,10 @@ fn get_snapshot_filename( assertion_file: &str, snapshot_name: &str, cargo_workspace: &Path, - base: &str, is_doctest: bool, ) -> PathBuf { let root = Path::new(cargo_workspace); - let base = Path::new(base); + let base = Path::new(assertion_file); Settings::with(|settings| { root.join(base.parent().unwrap()) .join(settings.snapshot_path()) @@ -190,8 +189,7 @@ fn get_snapshot_filename( write!( &mut f, "doctest_{}__", - Path::new(assertion_file) - .file_name() + base.file_name() .unwrap() .to_string_lossy() .replace('.', "_") @@ -268,7 +266,6 @@ impl<'a> SnapshotAssertionContext<'a> { assertion_file, &name, workspace, - assertion_file, is_doctest, ); if fs::metadata(&file).is_ok() { From 9c7561307490d32ea8b2db4673e3273a2f447f7d Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Tue, 1 Oct 2024 10:26:45 -0700 Subject: [PATCH 18/19] Rename function in integration tests for clarity (#629) --- cargo-insta/tests/main.rs | 65 ++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 32 deletions(-) diff --git a/cargo-insta/tests/main.rs b/cargo-insta/tests/main.rs index f8d8247b..acce0dc4 100644 --- a/cargo-insta/tests/main.rs +++ b/cargo-insta/tests/main.rs @@ -13,22 +13,22 @@ /// "--accept", "--", "--nocapture"])`. We then also need to pass /// `--nocapture` to the outer test to forward that to the terminal. /// - If a test fails on `assert_success` or `assert_failure`, we print the -/// output of the inner test, bypassing the `--nocapture` flag. We color it to +/// output of the inner test (bypassing the capturing). We color it to /// discriminate it from the output of the outer test. /// /// 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. /// -/// (That seems to be because they all share the same `target` directory, which -/// cargo will confuse for each other if they share the same name. I haven't -/// worked out why — this is the case even if the files are the same between two -/// tests but with different commands — and those files exist in different -/// temporary workspace dirs. (We could try to enforce different names, or give -/// up using a consistent target directory for a cache, but it would slow down -/// repeatedly running the tests locally. To demonstrate the effect, name crates -/// the same...). This also causes issues when running the same tests -/// concurrently. +/// > That seems to be because they all share the same `target` directory, which +/// > cargo will confuse for each other if they share the same name. I haven't +/// > worked out why — this is the case even if the files are the same between two +/// > tests but with different commands — and those files exist in different +/// > temporary workspace dirs. (We could try to enforce different names, or give +/// > up using a consistent target directory for a cache, but it would slow down +/// > repeatedly running the tests locally. To demonstrate the effect, name crates +/// > the same... This also causes issues when running the same tests +/// > concurrently. use std::collections::HashMap; use std::fs; use std::path::{Path, PathBuf}; @@ -151,7 +151,8 @@ impl TestProject { cmd.env_remove("CLICOLOR_FORCE"); cmd.env_remove("RUSTDOCFLAGS"); } - fn cmd(&self) -> Command { + + fn insta_cmd(&self) -> Command { let mut command = Command::new(env!("CARGO_BIN_EXE_cargo-insta")); Self::clean_env(&mut command); @@ -256,7 +257,7 @@ fn test_json_snapshot() { .create_project(); let output = test_project - .cmd() + .insta_cmd() .args(["test", "--accept", "--", "--nocapture"]) .output() .unwrap(); @@ -325,7 +326,7 @@ fn test_yaml_snapshot() { .create_project(); let output = test_project - .cmd() + .insta_cmd() .args(["test", "--accept"]) .output() .unwrap(); @@ -400,7 +401,7 @@ fn test_trailing_comma_in_inline_snapshot() { .create_project(); let output = test_project - .cmd() + .insta_cmd() .args(["test", "--accept"]) .output() .unwrap(); @@ -515,7 +516,7 @@ fn test_root() { } /// Check that in a workspace with a default root crate, running `cargo insta -/// test --workspace --accept` will update snapsnots in both the root crate and the +/// test --workspace --accept` will update snapshots in both the root crate and the /// member crate. #[test] fn test_root_crate_workspace_accept() { @@ -523,7 +524,7 @@ fn test_root_crate_workspace_accept() { workspace_with_root_crate("root-crate-workspace-accept".to_string()).create_project(); let output = test_project - .cmd() + .insta_cmd() .args(["test", "--accept", "--workspace"]) .output() .unwrap(); @@ -558,7 +559,7 @@ fn test_root_crate_workspace() { workspace_with_root_crate("root-crate-workspace".to_string()).create_project(); let output = test_project - .cmd() + .insta_cmd() // Need to disable colors to assert the output below .args(["test", "--workspace", "--color=never"]) .output() @@ -573,13 +574,13 @@ fn test_root_crate_workspace() { } /// Check that in a workspace with a default root crate, running `cargo insta -/// test --accept` will only update snapsnots in the root crate +/// test --accept` will only update snapshots in the root crate #[test] fn test_root_crate_no_all() { let test_project = workspace_with_root_crate("root-crate-no-all".to_string()).create_project(); let output = test_project - .cmd() + .insta_cmd() .args(["test", "--accept"]) .output() .unwrap(); @@ -681,7 +682,7 @@ fn test_virtual_manifest_all() { workspace_with_virtual_manifest("virtual-manifest-all".to_string()).create_project(); let output = test_project - .cmd() + .insta_cmd() .args(["test", "--accept", "--workspace"]) .output() .unwrap(); @@ -718,7 +719,7 @@ fn test_virtual_manifest_default() { workspace_with_virtual_manifest("virtual-manifest-default".to_string()).create_project(); let output = test_project - .cmd() + .insta_cmd() .args(["test", "--accept"]) .output() .unwrap(); @@ -755,7 +756,7 @@ fn test_virtual_manifest_single_crate() { workspace_with_virtual_manifest("virtual-manifest-single".to_string()).create_project(); let output = test_project - .cmd() + .insta_cmd() .args(["test", "--accept", "-p", "virtual-manifest-single-member-1"]) .output() .unwrap(); @@ -818,7 +819,7 @@ fn test_old_yaml_format() { // Run the test with --force-update-snapshots and --accept let output = test_project - .cmd() + .insta_cmd() .args(["test", "--accept", "--", "--nocapture"]) .output() .unwrap(); @@ -883,7 +884,7 @@ Hello, world! // Test with current insta version let output_current = test_current_insta - .cmd() + .insta_cmd() .args(["test", "--accept", "--force-update-snapshots"]) .output() .unwrap(); @@ -892,7 +893,7 @@ Hello, world! // Test with insta 1.40.0 let output_1_40_0 = test_insta_1_40_0 - .cmd() + .insta_cmd() .args(["test", "--accept", "--force-update-snapshots"]) .output() .unwrap(); @@ -964,7 +965,7 @@ fn test_linebreaks() { // Run the test with --force-update-snapshots and --accept let output = test_project - .cmd() + .insta_cmd() .args([ "test", "--force-update-snapshots", @@ -1023,7 +1024,7 @@ fn test_excessive_hashes() { // Run the test with --force-update-snapshots and --accept let output = test_project - .cmd() + .insta_cmd() .args([ "test", "--force-update-snapshots", @@ -1078,7 +1079,7 @@ fn test_wrong_indent_force() { // Confirm the test passes despite the indent let output = test_project - .cmd() + .insta_cmd() .args(["test", "--check", "--", "--nocapture"]) .output() .unwrap(); @@ -1087,7 +1088,7 @@ fn test_wrong_indent_force() { // Then run the test with --force-update-snapshots and --accept to confirm // the new snapshot is written let output = test_project - .cmd() + .insta_cmd() .args([ "test", "--force-update-snapshots", @@ -1150,7 +1151,7 @@ fn test_hashtag_escape() { .create_project(); let output = test_project - .cmd() + .insta_cmd() .args(["test", "--accept"]) .output() .unwrap(); @@ -1345,7 +1346,7 @@ fn test_hello() { // Initially, the test should fail let output = test_project - .cmd() + .insta_cmd() .current_dir(&proj_dir) .args(["test", "--"]) .output() @@ -1368,7 +1369,7 @@ fn test_hello() { // Run cargo insta accept let output = test_project - .cmd() + .insta_cmd() .current_dir(&proj_dir) .args(["test", "--accept"]) .output() From 916eed7edf4384c2c4467d6938fca46ae523db0b Mon Sep 17 00:00:00 2001 From: Maximilian Roos <5635139+max-sixty@users.noreply.github.com> Date: Tue, 1 Oct 2024 10:29:10 -0700 Subject: [PATCH 19/19] Improve error message on file write error (#631) Will help to diagnose #621 --- insta/src/content/mod.rs | 2 -- insta/src/snapshot.rs | 3 ++- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/insta/src/content/mod.rs b/insta/src/content/mod.rs index 1325f8f1..3be7144c 100644 --- a/insta/src/content/mod.rs +++ b/insta/src/content/mod.rs @@ -24,7 +24,6 @@ pub enum Error { UnexpectedDataType, #[cfg(feature = "_cargo_insta_internal")] MissingField, - #[cfg(feature = "_cargo_insta_internal")] FileIo(std::io::Error, std::path::PathBuf), } @@ -39,7 +38,6 @@ impl fmt::Display for Error { } #[cfg(feature = "_cargo_insta_internal")] Error::MissingField => f.write_str("A required field was missing"), - #[cfg(feature = "_cargo_insta_internal")] Error::FileIo(e, p) => { f.write_str(format!("File error for {:?}: {}", p.display(), e).as_str()) } diff --git a/insta/src/snapshot.rs b/insta/src/snapshot.rs index f9928c78..e1df2c3e 100644 --- a/insta/src/snapshot.rs +++ b/insta/src/snapshot.rs @@ -491,7 +491,8 @@ impl Snapshot { } let serialized_snapshot = self.serialize_snapshot(md); - fs::write(path, serialized_snapshot)?; + fs::write(path, serialized_snapshot) + .map_err(|e| content::Error::FileIo(e, path.to_path_buf()))?; Ok(()) }