From 0858005269a15ae051e2fce97477b6f10d2cfbb2 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 19 Apr 2024 10:08:10 -0500 Subject: [PATCH 1/5] refactor(snap): Reduce nesting --- crates/snapbox/src/assert.rs | 67 ++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 33 deletions(-) diff --git a/crates/snapbox/src/assert.rs b/crates/snapbox/src/assert.rs index 295fa1e1..63dd9e7e 100644 --- a/crates/snapbox/src/assert.rs +++ b/crates/snapbox/src/assert.rs @@ -170,39 +170,40 @@ impl Assert { actual_name: Option<&dyn std::fmt::Display>, ) { let result = self.try_verify(&expected, &actual, actual_name); - if let Err(err) = result { - match self.action { - Action::Skip => unreachable!("Bailed out earlier"), - Action::Ignore => { - use std::io::Write; - - let _ = writeln!( - stderr(), - "{}: {}", - self.palette.warn("Ignoring failure"), - err - ); - } - Action::Verify => { - let message = if expected.source().is_none() { - crate::report::Styled::new(String::new(), Default::default()) - } else if let Some(action_var) = self.action_var.as_deref() { - self.palette - .hint(format!("Update with {}=overwrite", action_var)) - } else { - crate::report::Styled::new(String::new(), Default::default()) - }; - panic!("{err}{message}"); - } - Action::Overwrite => { - use std::io::Write; - - if let Some(source) = expected.source() { - let _ = writeln!(stderr(), "{}: {}", self.palette.warn("Fixing"), err); - actual.write_to(source).unwrap(); - } else { - panic!("{err}"); - } + let Err(err) = result else { + return; + }; + match self.action { + Action::Skip => unreachable!("Bailed out earlier"), + Action::Ignore => { + use std::io::Write; + + let _ = writeln!( + stderr(), + "{}: {}", + self.palette.warn("Ignoring failure"), + err + ); + } + Action::Verify => { + let message = if expected.source().is_none() { + crate::report::Styled::new(String::new(), Default::default()) + } else if let Some(action_var) = self.action_var.as_deref() { + self.palette + .hint(format!("Update with {}=overwrite", action_var)) + } else { + crate::report::Styled::new(String::new(), Default::default()) + }; + panic!("{err}{message}"); + } + Action::Overwrite => { + use std::io::Write; + + if let Some(source) = expected.source() { + let _ = writeln!(stderr(), "{}: {}", self.palette.warn("Fixing"), err); + actual.write_to(source).unwrap(); + } else { + panic!("{err}"); } } } From 9de29a82ded98759a8d6b1bdefcd906bdc48d5f9 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 19 Apr 2024 10:15:24 -0500 Subject: [PATCH 2/5] refactor(snap): Let caller control panicking --- crates/snapbox/src/assert.rs | 33 ++++++++++++++++++--------------- crates/snapbox/src/cmd.rs | 16 ++++++++++++---- crates/snapbox/src/error.rs | 5 +++++ 3 files changed, 35 insertions(+), 19 deletions(-) diff --git a/crates/snapbox/src/assert.rs b/crates/snapbox/src/assert.rs index 63dd9e7e..aef2e49f 100644 --- a/crates/snapbox/src/assert.rs +++ b/crates/snapbox/src/assert.rs @@ -57,24 +57,25 @@ impl Assert { pub fn eq(&self, expected: impl Into, actual: impl Into) { let expected = expected.into(); let actual = actual.into(); - self.eq_inner(expected, actual); + if let Err(err) = self.eq_inner(expected, actual) { + err.panic(); + } } - #[track_caller] - fn eq_inner(&self, expected: crate::Data, actual: crate::Data) { + fn eq_inner(&self, expected: crate::Data, actual: crate::Data) -> Result<(), crate::Error> { if expected.source().is_none() && actual.source().is_some() { panic!("received `(actual, expected)`, expected `(expected, actual)`"); } match self.action { Action::Skip => { - return; + return Ok(()); } Action::Ignore | Action::Verify | Action::Overwrite => {} } let (expected, actual) = self.normalize_eq(expected, actual); - self.do_action(expected, actual, Some(&"In-memory")); + self.do_action(expected, actual, Some(&"In-memory")) } /// Check if a value matches a pattern @@ -106,24 +107,25 @@ impl Assert { pub fn matches(&self, pattern: impl Into, actual: impl Into) { let pattern = pattern.into(); let actual = actual.into(); - self.matches_inner(pattern, actual); + if let Err(err) = self.matches_inner(pattern, actual) { + err.panic(); + } } - #[track_caller] - fn matches_inner(&self, pattern: crate::Data, actual: crate::Data) { + fn matches_inner(&self, pattern: crate::Data, actual: crate::Data) -> Result<(), crate::Error> { if pattern.source().is_none() && actual.source().is_some() { panic!("received `(actual, expected)`, expected `(expected, actual)`"); } match self.action { Action::Skip => { - return; + return Ok(()); } Action::Ignore | Action::Verify | Action::Overwrite => {} } let (expected, actual) = self.normalize_match(pattern, actual); - self.do_action(expected, actual, Some(&"In-memory")); + self.do_action(expected, actual, Some(&"In-memory")) } pub(crate) fn normalize_eq( @@ -162,16 +164,15 @@ impl Assert { (expected, actual) } - #[track_caller] pub(crate) fn do_action( &self, expected: crate::Data, actual: crate::Data, actual_name: Option<&dyn std::fmt::Display>, - ) { + ) -> Result<(), crate::Error> { let result = self.try_verify(&expected, &actual, actual_name); let Err(err) = result else { - return; + return Ok(()); }; match self.action { Action::Skip => unreachable!("Bailed out earlier"), @@ -184,6 +185,7 @@ impl Assert { self.palette.warn("Ignoring failure"), err ); + Ok(()) } Action::Verify => { let message = if expected.source().is_none() { @@ -194,7 +196,7 @@ impl Assert { } else { crate::report::Styled::new(String::new(), Default::default()) }; - panic!("{err}{message}"); + Err(crate::Error::new(format_args!("{err}{message}"))) } Action::Overwrite => { use std::io::Write; @@ -202,8 +204,9 @@ impl Assert { if let Some(source) = expected.source() { let _ = writeln!(stderr(), "{}: {}", self.palette.warn("Fixing"), err); actual.write_to(source).unwrap(); + Ok(()) } else { - panic!("{err}"); + Err(crate::Error::new(format_args!("{err}"))) } } } diff --git a/crates/snapbox/src/cmd.rs b/crates/snapbox/src/cmd.rs index 354ea153..8ecdcfcd 100644 --- a/crates/snapbox/src/cmd.rs +++ b/crates/snapbox/src/cmd.rs @@ -621,7 +621,9 @@ impl OutputAssert { fn stdout_eq_inner(self, expected: crate::Data) -> Self { let actual = crate::Data::from(self.output.stdout.as_slice()); let (pattern, actual) = self.config.normalize_eq(expected, actual); - self.config.do_action(pattern, actual, Some(&"stdout")); + if let Err(err) = self.config.do_action(pattern, actual, Some(&"stdout")) { + err.panic(); + } self } @@ -661,7 +663,9 @@ impl OutputAssert { fn stdout_matches_inner(self, expected: crate::Data) -> Self { let actual = crate::Data::from(self.output.stdout.as_slice()); let (pattern, actual) = self.config.normalize_match(expected, actual); - self.config.do_action(pattern, actual, Some(&"stdout")); + if let Err(err) = self.config.do_action(pattern, actual, Some(&"stdout")) { + err.panic(); + } self } @@ -701,7 +705,9 @@ impl OutputAssert { fn stderr_eq_inner(self, expected: crate::Data) -> Self { let actual = crate::Data::from(self.output.stderr.as_slice()); let (pattern, actual) = self.config.normalize_eq(expected, actual); - self.config.do_action(pattern, actual, Some(&"stderr")); + if let Err(err) = self.config.do_action(pattern, actual, Some(&"stderr")) { + err.panic(); + } self } @@ -741,7 +747,9 @@ impl OutputAssert { fn stderr_matches_inner(self, expected: crate::Data) -> Self { let actual = crate::Data::from(self.output.stderr.as_slice()); let (pattern, actual) = self.config.normalize_match(expected, actual); - self.config.do_action(pattern, actual, Some(&"stderr")); + if let Err(err) = self.config.do_action(pattern, actual, Some(&"stderr")) { + err.panic(); + } self } diff --git a/crates/snapbox/src/error.rs b/crates/snapbox/src/error.rs index 55e90188..19297714 100644 --- a/crates/snapbox/src/error.rs +++ b/crates/snapbox/src/error.rs @@ -15,6 +15,11 @@ impl Error { backtrace: Backtrace::new(), } } + + #[track_caller] + pub(crate) fn panic(self) -> ! { + panic!("{self}") + } } impl PartialEq for Error { From 3aaeaea634955e3225450083dc3857449a51bec3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 19 Apr 2024 10:19:31 -0500 Subject: [PATCH 3/5] refactor(snap): Generalize inner checks --- crates/snapbox/src/assert.rs | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/crates/snapbox/src/assert.rs b/crates/snapbox/src/assert.rs index aef2e49f..13a36d83 100644 --- a/crates/snapbox/src/assert.rs +++ b/crates/snapbox/src/assert.rs @@ -57,12 +57,17 @@ impl Assert { pub fn eq(&self, expected: impl Into, actual: impl Into) { let expected = expected.into(); let actual = actual.into(); - if let Err(err) = self.eq_inner(expected, actual) { + if let Err(err) = self.eq_inner(expected, actual, Some(&"In-memory")) { err.panic(); } } - fn eq_inner(&self, expected: crate::Data, actual: crate::Data) -> Result<(), crate::Error> { + fn eq_inner( + &self, + expected: crate::Data, + actual: crate::Data, + actual_name: Option<&dyn std::fmt::Display>, + ) -> Result<(), crate::Error> { if expected.source().is_none() && actual.source().is_some() { panic!("received `(actual, expected)`, expected `(expected, actual)`"); } @@ -75,7 +80,7 @@ impl Assert { let (expected, actual) = self.normalize_eq(expected, actual); - self.do_action(expected, actual, Some(&"In-memory")) + self.do_action(expected, actual, actual_name) } /// Check if a value matches a pattern @@ -107,12 +112,17 @@ impl Assert { pub fn matches(&self, pattern: impl Into, actual: impl Into) { let pattern = pattern.into(); let actual = actual.into(); - if let Err(err) = self.matches_inner(pattern, actual) { + if let Err(err) = self.matches_inner(pattern, actual, Some(&"In-memory")) { err.panic(); } } - fn matches_inner(&self, pattern: crate::Data, actual: crate::Data) -> Result<(), crate::Error> { + fn matches_inner( + &self, + pattern: crate::Data, + actual: crate::Data, + actual_name: Option<&dyn std::fmt::Display>, + ) -> Result<(), crate::Error> { if pattern.source().is_none() && actual.source().is_some() { panic!("received `(actual, expected)`, expected `(expected, actual)`"); } @@ -125,7 +135,7 @@ impl Assert { let (expected, actual) = self.normalize_match(pattern, actual); - self.do_action(expected, actual, Some(&"In-memory")) + self.do_action(expected, actual, actual_name) } pub(crate) fn normalize_eq( From 0791d65b4892292bc2e4cb11f4532a8c5f353859 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 19 Apr 2024 10:22:25 -0500 Subject: [PATCH 4/5] refactor(snap): Have cmd leverage more of Asssert --- crates/snapbox/src/assert.rs | 8 ++++---- crates/snapbox/src/cmd.rs | 12 ++++-------- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/crates/snapbox/src/assert.rs b/crates/snapbox/src/assert.rs index 13a36d83..10478386 100644 --- a/crates/snapbox/src/assert.rs +++ b/crates/snapbox/src/assert.rs @@ -57,12 +57,12 @@ impl Assert { pub fn eq(&self, expected: impl Into, actual: impl Into) { let expected = expected.into(); let actual = actual.into(); - if let Err(err) = self.eq_inner(expected, actual, Some(&"In-memory")) { + if let Err(err) = self.try_eq(expected, actual, Some(&"In-memory")) { err.panic(); } } - fn eq_inner( + pub(crate) fn try_eq( &self, expected: crate::Data, actual: crate::Data, @@ -112,12 +112,12 @@ impl Assert { pub fn matches(&self, pattern: impl Into, actual: impl Into) { let pattern = pattern.into(); let actual = actual.into(); - if let Err(err) = self.matches_inner(pattern, actual, Some(&"In-memory")) { + if let Err(err) = self.try_matches(pattern, actual, Some(&"In-memory")) { err.panic(); } } - fn matches_inner( + pub(crate) fn try_matches( &self, pattern: crate::Data, actual: crate::Data, diff --git a/crates/snapbox/src/cmd.rs b/crates/snapbox/src/cmd.rs index 8ecdcfcd..7049d1d9 100644 --- a/crates/snapbox/src/cmd.rs +++ b/crates/snapbox/src/cmd.rs @@ -620,8 +620,7 @@ impl OutputAssert { #[track_caller] fn stdout_eq_inner(self, expected: crate::Data) -> Self { let actual = crate::Data::from(self.output.stdout.as_slice()); - let (pattern, actual) = self.config.normalize_eq(expected, actual); - if let Err(err) = self.config.do_action(pattern, actual, Some(&"stdout")) { + if let Err(err) = self.config.try_eq(expected, actual, Some(&"stdout")) { err.panic(); } @@ -662,8 +661,7 @@ impl OutputAssert { #[track_caller] fn stdout_matches_inner(self, expected: crate::Data) -> Self { let actual = crate::Data::from(self.output.stdout.as_slice()); - let (pattern, actual) = self.config.normalize_match(expected, actual); - if let Err(err) = self.config.do_action(pattern, actual, Some(&"stdout")) { + if let Err(err) = self.config.try_matches(expected, actual, Some(&"stdout")) { err.panic(); } @@ -704,8 +702,7 @@ impl OutputAssert { #[track_caller] fn stderr_eq_inner(self, expected: crate::Data) -> Self { let actual = crate::Data::from(self.output.stderr.as_slice()); - let (pattern, actual) = self.config.normalize_eq(expected, actual); - if let Err(err) = self.config.do_action(pattern, actual, Some(&"stderr")) { + if let Err(err) = self.config.try_eq(expected, actual, Some(&"stderr")) { err.panic(); } @@ -746,8 +743,7 @@ impl OutputAssert { #[track_caller] fn stderr_matches_inner(self, expected: crate::Data) -> Self { let actual = crate::Data::from(self.output.stderr.as_slice()); - let (pattern, actual) = self.config.normalize_match(expected, actual); - if let Err(err) = self.config.do_action(pattern, actual, Some(&"stderr")) { + if let Err(err) = self.config.try_matches(expected, actual, Some(&"stderr")) { err.panic(); } From 587119899fb81b1a10d00c6b8fb20580597ad14c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 19 Apr 2024 10:44:21 -0500 Subject: [PATCH 5/5] refactor(harness): Reuse more Assert logic --- crates/snapbox/src/assert.rs | 2 +- crates/snapbox/src/harness.rs | 110 ++++++---------------------------- 2 files changed, 18 insertions(+), 94 deletions(-) diff --git a/crates/snapbox/src/assert.rs b/crates/snapbox/src/assert.rs index 10478386..5d57088c 100644 --- a/crates/snapbox/src/assert.rs +++ b/crates/snapbox/src/assert.rs @@ -22,7 +22,7 @@ use crate::Action; /// ``` #[derive(Clone, Debug)] pub struct Assert { - action: Action, + pub(crate) action: Action, action_var: Option, normalize_paths: bool, substitutions: crate::Substitutions, diff --git a/crates/snapbox/src/harness.rs b/crates/snapbox/src/harness.rs index 9e723992..4210f5ba 100644 --- a/crates/snapbox/src/harness.rs +++ b/crates/snapbox/src/harness.rs @@ -36,7 +36,7 @@ //! } //! ``` -use crate::data::{DataFormat, NormalizeNewlines}; +use crate::data::DataFormat; use crate::Action; use libtest_mimic::Trial; @@ -49,7 +49,7 @@ pub struct Harness { overrides: Option, setup: S, test: T, - action: Action, + config: crate::Assert, } impl Harness @@ -71,7 +71,7 @@ where overrides: None, setup, test, - action: Action::Verify, + config: crate::Assert::new().action_env(crate::DEFAULT_ACTION_ENV), } } @@ -89,14 +89,19 @@ where /// Read the failure action from an environment variable pub fn action_env(mut self, var_name: &str) -> Self { - let action = Action::with_env_var(var_name); - self.action = action.unwrap_or(self.action); + self.config = self.config.action_env(var_name); self } /// Override the failure action pub fn action(mut self, action: Action) -> Self { - self.action = action; + self.config = self.config.action(action); + self + } + + /// Customize the assertion behavior + pub fn with_assert(mut self, config: crate::Assert) -> Self { + self.config = config; self } @@ -118,26 +123,22 @@ where } }); + let shared_config = std::sync::Arc::new(self.config); let tests: Vec<_> = tests .into_iter() .map(|path| { let case = (self.setup)(path); let test = self.test.clone(); + let config = shared_config.clone(); Trial::test(case.name.clone(), move || { + let expected = crate::Data::read_from(&case.expected, case.format); let actual = (test)(&case.fixture)?; let actual = actual.to_string(); - let mut actual = crate::Data::text(actual).normalize(NormalizeNewlines); - if let Some(format) = case.format { - actual = actual.coerce_to(format); - } - #[allow(deprecated)] - let verify = Verifier::new() - .palette(crate::report::Palette::auto()) - .action(self.action); - verify.verify(&case.expected, actual)?; + let actual = crate::Data::text(actual); + config.try_eq(expected, actual, Some(&case.name))?; Ok(()) }) - .with_ignored_flag(self.action == Action::Ignore) + .with_ignored_flag(shared_config.action == Action::Ignore) }) .collect(); @@ -146,83 +147,6 @@ where } } -struct Verifier { - palette: crate::report::Palette, - action: Action, -} - -impl Verifier { - fn new() -> Self { - Default::default() - } - - fn palette(mut self, palette: crate::report::Palette) -> Self { - self.palette = palette; - self - } - - fn action(mut self, action: Action) -> Self { - self.action = action; - self - } - - fn verify(&self, expected_path: &std::path::Path, actual: crate::Data) -> crate::Result<()> { - match self.action { - Action::Skip => Ok(()), - Action::Ignore => { - let _ = self.try_verify(expected_path, actual); - Ok(()) - } - Action::Verify => self.try_verify(expected_path, actual), - Action::Overwrite => self.try_overwrite(expected_path, actual), - } - } - - fn try_overwrite( - &self, - expected_path: &std::path::Path, - actual: crate::Data, - ) -> crate::Result<()> { - actual.write_to_path(expected_path)?; - Ok(()) - } - - fn try_verify( - &self, - expected_path: &std::path::Path, - actual: crate::Data, - ) -> crate::Result<()> { - let expected = crate::Data::read_from(expected_path, Some(DataFormat::Text)) - .normalize(NormalizeNewlines); - - if expected != actual { - let mut buf = String::new(); - crate::report::write_diff( - &mut buf, - &expected, - &actual, - Some(&expected_path.display()), - None, - self.palette, - ) - .map_err(|e| e.to_string())?; - Err(buf.into()) - } else { - Ok(()) - } - } -} - -impl Default for Verifier { - fn default() -> Self { - Self { - #[allow(deprecated)] - palette: crate::report::Palette::auto(), - action: Action::Verify, - } - } -} - /// A test case enumerated by the [`Harness`] with data from the `setup` function /// /// See [`harness`][crate::harness] for more details