From 05c139b65fbc7399ab728b69f4979b49abcb2ed9 Mon Sep 17 00:00:00 2001 From: Marc Herbert Date: Wed, 6 May 2026 10:35:05 -0400 Subject: [PATCH] tests: validate "patch" and "ed" commands once, print meaningful messages macOS' /usr/bin/patch and GNU patch have very subtle incompatibilities that cause only some "more advanced" tests to fail in obscure and very time-consuming ways - while other tests pass. In some cases (depending on test threads racing), the lack of newlines in some test data even causes the whole test suite to stall. This fix runs `patch -version` (only once), makes sure the output starts with "GNU patch" and shows a meaningful assert message when not. It also looks for `gpatch` instead of `patch` on macOS and shows a meaningful assert message if either is missing. Fixes: #225 This also provides faster and better feedback when `ed` is missing (see #39) and implements a portable and basic check. Last but not least, this new code is generic enough to support the validation of any other test dependency in the future. --- src/context_diff.rs | 19 +++++---- src/ed_diff.rs | 15 +++++--- src/normal_diff.rs | 18 +++++---- src/unified_diff.rs | 24 +++++++----- src/utils.rs | 93 +++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 137 insertions(+), 32 deletions(-) diff --git a/src/context_diff.rs b/src/context_diff.rs index 873fc3d..598a8fa 100644 --- a/src/context_diff.rs +++ b/src/context_diff.rs @@ -381,6 +381,9 @@ pub fn diff(expected: &[u8], actual: &[u8], params: &Params) -> Vec { mod tests { use super::*; use pretty_assertions::assert_eq; + + use crate::utils::testcmds::PATCH_CMD; + #[test] fn test_permutations() { // test all possible six-line files. @@ -394,7 +397,6 @@ mod tests { for &f in &[0, 1, 2] { use std::fs::{self, File}; use std::io::Write; - use std::process::Command; let mut alef = Vec::new(); let mut bet = Vec::new(); alef.write_all(if a == 0 { b"a\n" } else { b"b\n" }) @@ -449,7 +451,8 @@ mod tests { fb.write_all(&bet[..]).unwrap(); let _ = fa; let _ = fb; - let output = Command::new("patch") + let output = PATCH_CMD + .new() .arg("-p0") .arg("--context") .stdin(File::open(format!("{target}/ab.diff")).unwrap()) @@ -481,7 +484,6 @@ mod tests { for &f in &[0, 1, 2] { use std::fs::{self, File}; use std::io::Write; - use std::process::Command; let mut alef = Vec::new(); let mut bet = Vec::new(); alef.write_all(if a == 0 { b"\n" } else { b"b\n" }).unwrap(); @@ -530,7 +532,8 @@ mod tests { fb.write_all(&bet[..]).unwrap(); let _ = fa; let _ = fb; - let output = Command::new("patch") + let output = PATCH_CMD + .new() .arg("-p0") .arg("--context") .stdin(File::open(format!("{target}/ab_.diff")).unwrap()) @@ -562,7 +565,6 @@ mod tests { for &f in &[0, 1, 2] { use std::fs::{self, File}; use std::io::Write; - use std::process::Command; let mut alef = Vec::new(); let mut bet = Vec::new(); alef.write_all(if a == 0 { b"a\n" } else { b"" }).unwrap(); @@ -614,7 +616,8 @@ mod tests { fb.write_all(&bet[..]).unwrap(); let _ = fa; let _ = fb; - let output = Command::new("patch") + let output = PATCH_CMD + .new() .arg("-p0") .arg("--context") .stdin(File::open(format!("{target}/abx.diff")).unwrap()) @@ -646,7 +649,6 @@ mod tests { for &f in &[0, 1, 2] { use std::fs::{self, File}; use std::io::Write; - use std::process::Command; let mut alef = Vec::new(); let mut bet = Vec::new(); alef.write_all(if a == 0 { b"a\n" } else { b"f\n" }) @@ -701,7 +703,8 @@ mod tests { fb.write_all(&bet[..]).unwrap(); let _ = fa; let _ = fb; - let output = Command::new("patch") + let output = PATCH_CMD + .new() .arg("-p0") .arg("--context") .stdin(File::open(format!("{target}/abr.diff")).unwrap()) diff --git a/src/ed_diff.rs b/src/ed_diff.rs index b8cdbc5..81ddfb4 100644 --- a/src/ed_diff.rs +++ b/src/ed_diff.rs @@ -162,6 +162,9 @@ pub fn diff(expected: &[u8], actual: &[u8], params: &Params) -> Result, mod tests { use super::*; use pretty_assertions::assert_eq; + + use crate::utils::testcmds::ED_CMD; + pub fn diff_w(expected: &[u8], actual: &[u8], filename: &str) -> Result, DiffError> { let mut output = diff(expected, actual, &Params::default())?; writeln!(&mut output, "w {filename}").unwrap(); @@ -237,8 +240,8 @@ mod tests { let _ = fb; #[cfg(not(windows))] // there's no ed on windows { - use std::process::Command; - let output = Command::new("ed") + let output = ED_CMD + .new() .arg(format!("{target}/alef")) .stdin(File::open(format!("{target}/ab.ed")).unwrap()) .output() @@ -311,8 +314,8 @@ mod tests { let _ = fb; #[cfg(not(windows))] // there's no ed on windows { - use std::process::Command; - let output = Command::new("ed") + let output = ED_CMD + .new() .arg(format!("{target}/alef_")) .stdin(File::open(format!("{target}/ab_.ed")).unwrap()) .output() @@ -391,8 +394,8 @@ mod tests { let _ = fb; #[cfg(not(windows))] // there's no ed on windows { - use std::process::Command; - let output = Command::new("ed") + let output = ED_CMD + .new() .arg(format!("{target}/alefr")) .stdin(File::open(format!("{target}/abr.ed")).unwrap()) .output() diff --git a/src/normal_diff.rs b/src/normal_diff.rs index 002cd01..652dc84 100644 --- a/src/normal_diff.rs +++ b/src/normal_diff.rs @@ -215,6 +215,8 @@ mod tests { use super::*; use pretty_assertions::assert_eq; + use crate::utils::testcmds::PATCH_CMD; + #[test] fn test_basic() { let mut a = Vec::new(); @@ -239,7 +241,6 @@ mod tests { for &f in &[0, 1, 2] { use std::fs::{self, File}; use std::io::Write; - use std::process::Command; let mut alef = Vec::new(); let mut bet = Vec::new(); alef.write_all(if a == 0 { b"a\n" } else { b"b\n" }) @@ -285,7 +286,8 @@ mod tests { fb.write_all(&bet[..]).unwrap(); let _ = fa; let _ = fb; - let output = Command::new("patch") + let output = PATCH_CMD + .new() .arg("-p0") .arg(format!("{target}/alef")) .stdin(File::open(format!("{target}/ab.diff")).unwrap()) @@ -318,7 +320,6 @@ mod tests { for &g in &[0, 1, 2] { use std::fs::{self, File}; use std::io::Write; - use std::process::Command; let mut alef = Vec::new(); let mut bet = Vec::new(); alef.write_all(if a == 0 { b"a\n" } else { b"b\n" }) @@ -377,7 +378,8 @@ mod tests { fb.write_all(&bet[..]).unwrap(); let _ = fa; let _ = fb; - let output = Command::new("patch") + let output = PATCH_CMD + .new() .arg("-p0") .arg("--normal") .arg(format!("{target}/alefn")) @@ -411,7 +413,6 @@ mod tests { for &f in &[0, 1, 2] { use std::fs::{self, File}; use std::io::Write; - use std::process::Command; let mut alef = Vec::new(); let mut bet = Vec::new(); alef.write_all(if a == 0 { b"\n" } else { b"b\n" }).unwrap(); @@ -451,7 +452,8 @@ mod tests { fb.write_all(&bet[..]).unwrap(); let _ = fa; let _ = fb; - let output = Command::new("patch") + let output = PATCH_CMD + .new() .arg("-p0") .arg(format!("{target}/alef_")) .stdin(File::open(format!("{target}/ab_.diff")).unwrap()) @@ -483,7 +485,6 @@ mod tests { for &f in &[0, 1, 2] { use std::fs::{self, File}; use std::io::Write; - use std::process::Command; let mut alef = Vec::new(); let mut bet = Vec::new(); alef.write_all(if a == 0 { b"a\n" } else { b"f\n" }) @@ -529,7 +530,8 @@ mod tests { fb.write_all(&bet[..]).unwrap(); let _ = fa; let _ = fb; - let output = Command::new("patch") + let output = PATCH_CMD + .new() .arg("-p0") .arg(format!("{target}/alefr")) .stdin(File::open(format!("{target}/abr.diff")).unwrap()) diff --git a/src/unified_diff.rs b/src/unified_diff.rs index 0f504a8..83f28cb 100644 --- a/src/unified_diff.rs +++ b/src/unified_diff.rs @@ -408,6 +408,8 @@ mod tests { use super::*; use pretty_assertions::assert_eq; + use crate::utils::testcmds::PATCH_CMD; + #[test] fn test_permutations() { let target = "target/unified-diff/"; @@ -421,7 +423,6 @@ mod tests { for &f in &[0, 1, 2] { use std::fs::{self, File}; use std::io::Write; - use std::process::Command; let mut alef = Vec::new(); let mut bet = Vec::new(); alef.write_all(if a == 0 { b"a\n" } else { b"b\n" }) @@ -492,7 +493,10 @@ mod tests { .unwrap_or_else(|_| String::from("[Invalid UTF-8]")) ); - let output = Command::new("patch") + use crate::utils::testcmds::PATCH_CMD; + + let output = PATCH_CMD + .new() .arg("-p0") .stdin(File::open(format!("{target}/ab.diff")).unwrap()) .output() @@ -524,7 +528,6 @@ mod tests { for &g in &[0, 1, 2] { use std::fs::{self, File}; use std::io::Write; - use std::process::Command; let mut alef = Vec::new(); let mut bet = Vec::new(); alef.write_all(if a == 0 { b"a\n" } else { b"b\n" }) @@ -592,7 +595,8 @@ mod tests { fb.write_all(&bet[..]).unwrap(); let _ = fa; let _ = fb; - let output = Command::new("patch") + let output = PATCH_CMD + .new() .arg("-p0") .stdin(File::open(format!("{target}/abn.diff")).unwrap()) .output() @@ -625,7 +629,6 @@ mod tests { for &g in &[0, 1, 2, 3] { use std::fs::{self, File}; use std::io::Write; - use std::process::Command; let mut alef = Vec::new(); let mut bet = Vec::new(); alef.write_all(if a == 0 { b"\n" } else { b"b\n" }).unwrap(); @@ -688,7 +691,8 @@ mod tests { fb.write_all(&bet[..]).unwrap(); let _ = fa; let _ = fb; - let output = Command::new("patch") + let output = PATCH_CMD + .new() .arg("-p0") .stdin(File::open(format!("{target}/ab_.diff")).unwrap()) .output() @@ -720,7 +724,6 @@ mod tests { for &f in &[0, 1, 2] { use std::fs::{self, File}; use std::io::Write; - use std::process::Command; let mut alef = Vec::new(); let mut bet = Vec::new(); alef.write_all(if a == 0 { b"a\n" } else { b"" }).unwrap(); @@ -769,7 +772,8 @@ mod tests { fb.write_all(&bet[..]).unwrap(); let _ = fa; let _ = fb; - let output = Command::new("patch") + let output = PATCH_CMD + .new() .arg("-p0") .stdin(File::open(format!("{target}/abx.diff")).unwrap()) .output() @@ -800,7 +804,6 @@ mod tests { for &f in &[0, 1, 2] { use std::fs::{self, File}; use std::io::Write; - use std::process::Command; let mut alef = Vec::new(); let mut bet = Vec::new(); alef.write_all(if a == 0 { b"a\n" } else { b"f\n" }) @@ -855,7 +858,8 @@ mod tests { fb.write_all(&bet[..]).unwrap(); let _ = fa; let _ = fb; - let output = Command::new("patch") + let output = PATCH_CMD + .new() .arg("-p0") .stdin(File::open(format!("{target}/abr.diff")).unwrap()) .output() diff --git a/src/utils.rs b/src/utils.rs index daca18d..5e09bde 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -98,6 +98,99 @@ pub fn report_failure_to_read_input_file( ); } +#[cfg(test)] +pub mod testcmds { + // Command construction wrapper that provides some validation and non-obscure, "fail fast" + // feedback and error messages. + use std::any::Any; + use std::io::Write; + use std::panic::catch_unwind; + use std::process::{Command, Stdio}; + use std::sync::LazyLock; + + pub struct CmdFactory { + cmd: &'static str, + validated_once: LazyLock>, + validate: fn(&CmdFactory) -> (), + } + + impl CmdFactory { + pub fn new(&self) -> Command { + match &*self.validated_once { + Ok(()) => Command::new(self.cmd), + Err(errmsg) => panic!( + "'{}' validation failed in earlier thread/test: {}", + self.cmd, errmsg + ), + } + } + // "self" is not compatible with static initialization + fn try_catch_validate(cmd: &CmdFactory) -> Result<(), String> { + // Note catch_unwind() does _not_ hide error messages, stack traces, etc. + catch_unwind(|| { + let _ = (cmd.validate)(cmd); + }) + .map_err(find_panic_message) + } + } + + fn find_panic_message(payload: Box) -> String { + // https://github.com/rust-lang/rust/blob/1.95.0/library/std/src/panicking.rs#L771 + if let Some(&s) = payload.downcast_ref::<&'static str>() { + String::from(s) + } else if let Some(s) = payload.downcast_ref::() { + s.clone() + } else { + format!( + "Unusual panic payload type {:?}, look for the first thread/test that failed", + payload.type_id(), + ) + } + } + + pub static PATCH_CMD: CmdFactory = CmdFactory { + cmd: if cfg!(target_os = "macos") { + "gpatch" // brew install patch + } else { + "patch" + }, + validated_once: LazyLock::new(|| CmdFactory::try_catch_validate(&PATCH_CMD)), + + validate: (|myself| { + let output = Command::new(myself.cmd) + .arg("--version") + .output() + .expect("`patch --version` failed"); + // Non-GNU versions have subtle differences. When some newlines are missing in some test + // patches, the macOS version can even stall the whole test run. + assert!(output.stdout.starts_with(b"GNU patch")); + assert!(output.status.success()); + }), + }; + + pub static ED_CMD: CmdFactory = CmdFactory { + cmd: "ed", + validated_once: LazyLock::new(|| CmdFactory::try_catch_validate(&ED_CMD)), + + validate: (|myself| { + let mut child = Command::new(myself.cmd) + .arg("!echo hello_ed") + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .spawn() + .expect("Failed to start 'ed' command"); + + let mut stdin = child.stdin.take().unwrap(); + writeln!(stdin, "1p\nq").expect("Failed to send command to 'ed'"); + + let output = child + .wait_with_output() + .expect("Failed to read 'ed' stdout"); + assert_eq!(String::from_utf8_lossy(&output.stdout), "9\nhello_ed\n"); + }), + }; +} + #[cfg(test)] mod tests { use super::*;