From 132551a57cea75995863cc1cfe59707bb1503458 Mon Sep 17 00:00:00 2001 From: Denis Cornehl Date: Thu, 16 Apr 2026 06:03:22 +0200 Subject: [PATCH 1/2] derive some traits for ProcessStatistics to make it usable --- src/cmd/mod.rs | 118 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 117 insertions(+), 1 deletion(-) diff --git a/src/cmd/mod.rs b/src/cmd/mod.rs index c411e37..86fb6f9 100644 --- a/src/cmd/mod.rs +++ b/src/cmd/mod.rs @@ -16,6 +16,8 @@ use log::{error, info}; use process_lines_actions::InnerState; use std::env::consts::EXE_SUFFIX; use std::ffi::{OsStr, OsString}; +use std::mem; +use std::ops; use std::path::{Path, PathBuf}; use std::process::{ExitStatus, Stdio}; use std::time::{Duration, Instant}; @@ -557,7 +559,7 @@ impl From for ProcessOutput { } /// collected statistics about the process execution. -#[derive(Default)] +#[derive(Debug, Default, Clone)] pub struct ProcessStatistics { /// peak memory usage in bytes. /// This is populated for sandboxed commands on systems @@ -565,6 +567,25 @@ pub struct ProcessStatistics { pub memory_peak: Option, } +impl ops::Add for ProcessStatistics { + type Output = Self; + + fn add(self, rhs: Self) -> Self { + Self { + memory_peak: match (self.memory_peak, rhs.memory_peak) { + (Some(a), Some(b)) => Some(a.max(b)), + (a, b) => a.or(b), + }, + } + } +} + +impl ops::AddAssign for ProcessStatistics { + fn add_assign(&mut self, rhs: Self) { + *self = mem::take(self) + rhs; + } +} + /// Output of a [`Command`](struct.Command.html) when it was executed with the /// [`run_capture`](struct.Command.html#method.run_capture) method. pub struct ProcessOutput { @@ -732,3 +753,98 @@ fn exe_suffix(file: &OsStr) -> OsString { path.push(EXE_SUFFIX); path } + +#[cfg(test)] +mod tests { + use super::ProcessStatistics; + + // helpers + fn stats(peak: Option) -> ProcessStatistics { + ProcessStatistics { memory_peak: peak } + } + + // --- Add --- + + #[test] + fn add_both_none() { + let result = stats(None) + stats(None); + assert_eq!(result.memory_peak, None); + } + + #[test] + fn add_lhs_some_rhs_none() { + let result = stats(Some(100)) + stats(None); + assert_eq!(result.memory_peak, Some(100)); + } + + #[test] + fn add_lhs_none_rhs_some() { + let result = stats(None) + stats(Some(200)); + assert_eq!(result.memory_peak, Some(200)); + } + + #[test] + fn add_both_some_lhs_greater() { + let result = stats(Some(300)) + stats(Some(100)); + assert_eq!(result.memory_peak, Some(300)); + } + + #[test] + fn add_both_some_rhs_greater() { + let result = stats(Some(100)) + stats(Some(300)); + assert_eq!(result.memory_peak, Some(300)); + } + + #[test] + fn add_both_some_equal() { + let result = stats(Some(42)) + stats(Some(42)); + assert_eq!(result.memory_peak, Some(42)); + } + + // --- AddAssign --- + + #[test] + fn add_assign_both_none() { + let mut s = stats(None); + s += stats(None); + assert_eq!(s.memory_peak, None); + } + + #[test] + fn add_assign_lhs_some_rhs_none() { + let mut s = stats(Some(100)); + s += stats(None); + assert_eq!(s.memory_peak, Some(100)); + } + + #[test] + fn add_assign_lhs_none_rhs_some() { + let mut s = stats(None); + s += stats(Some(200)); + assert_eq!(s.memory_peak, Some(200)); + } + + #[test] + fn add_assign_both_some_lhs_greater() { + let mut s = stats(Some(300)); + s += stats(Some(100)); + assert_eq!(s.memory_peak, Some(300)); + } + + #[test] + fn add_assign_both_some_rhs_greater() { + let mut s = stats(Some(100)); + s += stats(Some(300)); + assert_eq!(s.memory_peak, Some(300)); + } + + #[test] + fn add_assign_accumulate_over_multiple() { + let mut s = stats(None); + s += stats(Some(50)); + s += stats(Some(200)); + s += stats(None); + s += stats(Some(150)); + assert_eq!(s.memory_peak, Some(200)); + } +} From 145b632b23e80d7dd4eeaa4321c1fbbdd6c589bb Mon Sep 17 00:00:00 2001 From: Denis Cornehl Date: Sun, 19 Apr 2026 09:32:09 +0200 Subject: [PATCH 2/2] replace ProcessStats Add / AddAssign with merge/merge_mut fns --- Cargo.toml | 1 + src/cmd/mod.rs | 127 +++++++++++++++---------------------------------- 2 files changed, 39 insertions(+), 89 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 50fbc61..6481242 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -50,4 +50,5 @@ windows-sys = {version = "0.52.0", features = ["Win32_Foundation", "Win32_System [dev-dependencies] env_logger = "0.11.3" rand = "0.10.0" +test-case = "3.3.1" tiny_http = "0.12.0" diff --git a/src/cmd/mod.rs b/src/cmd/mod.rs index 86fb6f9..f55e99c 100644 --- a/src/cmd/mod.rs +++ b/src/cmd/mod.rs @@ -17,7 +17,6 @@ use process_lines_actions::InnerState; use std::env::consts::EXE_SUFFIX; use std::ffi::{OsStr, OsString}; use std::mem; -use std::ops; use std::path::{Path, PathBuf}; use std::process::{ExitStatus, Stdio}; use std::time::{Duration, Instant}; @@ -560,6 +559,7 @@ impl From for ProcessOutput { /// collected statistics about the process execution. #[derive(Debug, Default, Clone)] +#[cfg_attr(test, derive(PartialEq, Eq))] pub struct ProcessStatistics { /// peak memory usage in bytes. /// This is populated for sandboxed commands on systems @@ -567,22 +567,26 @@ pub struct ProcessStatistics { pub memory_peak: Option, } -impl ops::Add for ProcessStatistics { - type Output = Self; - - fn add(self, rhs: Self) -> Self { +impl ProcessStatistics { + /// Merge two `ProcessStatistics` into one, following a fixed set of aggregation rules: + /// + /// - `memory_peak`: the maximum of the two values is kept, since a merged peak + /// should reflect the highest peak observed across all runs. If only one side + /// has a value and the other is `None`, that value is used as-is. + pub fn merge(self, other: Self) -> Self { Self { - memory_peak: match (self.memory_peak, rhs.memory_peak) { + memory_peak: match (self.memory_peak, other.memory_peak) { (Some(a), Some(b)) => Some(a.max(b)), (a, b) => a.or(b), }, } } -} -impl ops::AddAssign for ProcessStatistics { - fn add_assign(&mut self, rhs: Self) { - *self = mem::take(self) + rhs; + /// Merge another `ProcessStatistics` into `self` in place. + /// + /// See [`merge`](Self::merge) for the aggregation rules. + pub fn merge_mut(&mut self, other: Self) { + *self = mem::take(self).merge(other); } } @@ -757,94 +761,39 @@ fn exe_suffix(file: &OsStr) -> OsString { #[cfg(test)] mod tests { use super::ProcessStatistics; + use test_case::test_case; - // helpers - fn stats(peak: Option) -> ProcessStatistics { + const fn stats(peak: Option) -> ProcessStatistics { ProcessStatistics { memory_peak: peak } } - // --- Add --- - - #[test] - fn add_both_none() { - let result = stats(None) + stats(None); - assert_eq!(result.memory_peak, None); - } - - #[test] - fn add_lhs_some_rhs_none() { - let result = stats(Some(100)) + stats(None); - assert_eq!(result.memory_peak, Some(100)); - } - - #[test] - fn add_lhs_none_rhs_some() { - let result = stats(None) + stats(Some(200)); - assert_eq!(result.memory_peak, Some(200)); - } - - #[test] - fn add_both_some_lhs_greater() { - let result = stats(Some(300)) + stats(Some(100)); - assert_eq!(result.memory_peak, Some(300)); - } - - #[test] - fn add_both_some_rhs_greater() { - let result = stats(Some(100)) + stats(Some(300)); - assert_eq!(result.memory_peak, Some(300)); - } - - #[test] - fn add_both_some_equal() { - let result = stats(Some(42)) + stats(Some(42)); - assert_eq!(result.memory_peak, Some(42)); - } - - // --- AddAssign --- - - #[test] - fn add_assign_both_none() { - let mut s = stats(None); - s += stats(None); - assert_eq!(s.memory_peak, None); - } - - #[test] - fn add_assign_lhs_some_rhs_none() { - let mut s = stats(Some(100)); - s += stats(None); - assert_eq!(s.memory_peak, Some(100)); - } - - #[test] - fn add_assign_lhs_none_rhs_some() { - let mut s = stats(None); - s += stats(Some(200)); - assert_eq!(s.memory_peak, Some(200)); - } - - #[test] - fn add_assign_both_some_lhs_greater() { - let mut s = stats(Some(300)); - s += stats(Some(100)); - assert_eq!(s.memory_peak, Some(300)); - } + #[test_case(stats(None), stats(None), stats(None))] + #[test_case(stats(Some(100)), stats(None), stats(Some(100)))] + #[test_case(stats(None), stats(Some(100)), stats(Some(100)))] + #[test_case(stats(Some(300)), stats(Some(100)), stats(Some(300)))] + #[test_case(stats(Some(100)), stats(Some(300)), stats(Some(300)))] + #[test_case(stats(Some(42)), stats(Some(42)), stats(Some(42)))] + fn test_merge(lhs: ProcessStatistics, rhs: ProcessStatistics, expected: ProcessStatistics) { + { + let lhs = lhs.clone(); + let rhs = rhs.clone(); + assert_eq!(lhs.merge(rhs), expected); + } - #[test] - fn add_assign_both_some_rhs_greater() { - let mut s = stats(Some(100)); - s += stats(Some(300)); - assert_eq!(s.memory_peak, Some(300)); + { + let mut lhs = lhs.clone(); + lhs.merge_mut(rhs); + assert_eq!(lhs, expected); + } } #[test] - fn add_assign_accumulate_over_multiple() { + fn merge_mut_accumulate_over_multiple() { let mut s = stats(None); - s += stats(Some(50)); - s += stats(Some(200)); - s += stats(None); - s += stats(Some(150)); + s.merge_mut(stats(Some(50))); + s.merge_mut(stats(Some(200))); + s.merge_mut(stats(None)); + s.merge_mut(stats(Some(150))); assert_eq!(s.memory_peak, Some(200)); } }