From fb57584546a58ee56a9d45176a331de2d5e85c7a Mon Sep 17 00:00:00 2001 From: dailz Date: Fri, 5 Jun 2026 15:20:24 +0800 Subject: [PATCH] fix(bench): validate --suites names, reject unknown suites at CLI boundary Introduce Suite enum (runner.rs) replacing stringly-typed suite matching. BenchConfig.suites is now Option>, making invalid states unrepresentable. Unknown suite names produce a clear error listing all valid values. Fixes: #37 --- crates/bench/src/main.rs | 17 ++++- crates/bench/src/runner.rs | 127 ++++++++++++++++++++++++++++++++++--- 2 files changed, 133 insertions(+), 11 deletions(-) diff --git a/crates/bench/src/main.rs b/crates/bench/src/main.rs index 1155d04..3ed5b38 100644 --- a/crates/bench/src/main.rs +++ b/crates/bench/src/main.rs @@ -25,6 +25,21 @@ struct Args { fn main() { let args = Args::parse(); + let suites = match args.suites { + Some(names) => { + let parsed: Result, _> = + names.iter().map(|s| s.parse::()).collect(); + match parsed { + Ok(s) => Some(s), + Err(e) => { + eprintln!("error: {e}"); + std::process::exit(1); + } + } + } + None => None, + }; + println!("=== Benchmark: mmap vs pread ==="); println!("Test file: {}", args.test_file.display()); println!("Quick mode: {}", args.quick); @@ -33,7 +48,7 @@ fn main() { let config = log_viewer_bench::runner::BenchConfig { test_file: args.test_file.clone(), quick_mode: args.quick, - suites: args.suites, + suites, }; if !config.test_file.exists() { diff --git a/crates/bench/src/runner.rs b/crates/bench/src/runner.rs index 726561f..380d48f 100644 --- a/crates/bench/src/runner.rs +++ b/crates/bench/src/runner.rs @@ -2,10 +2,67 @@ use crate::metrics::MetricsCollector; use crate::types::BenchmarkResult; use std::path::PathBuf; +/// All recognized benchmark suite identifiers. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Suite { + Startup, + Render, + Jump, + Memory, + Growth, + Rotation, + Concurrent, +} + +impl Suite { + /// All valid suite identifiers, in execution order. + pub const ALL: &[Suite] = &[ + Suite::Startup, + Suite::Render, + Suite::Jump, + Suite::Memory, + Suite::Growth, + Suite::Rotation, + Suite::Concurrent, + ]; + + #[must_use] + pub const fn as_str(self) -> &'static str { + match self { + Suite::Startup => "startup", + Suite::Render => "render", + Suite::Jump => "jump", + Suite::Memory => "memory", + Suite::Growth => "growth", + Suite::Rotation => "rotation", + Suite::Concurrent => "concurrent", + } + } +} + +impl std::str::FromStr for Suite { + type Err = String; + + fn from_str(s: &str) -> Result { + Suite::ALL + .iter() + .find(|suite| suite.as_str() == s) + .copied() + .ok_or_else(|| { + let valid = Suite::ALL + .iter() + .map(|s| s.as_str()) + .collect::>() + .join(", "); + format!("invalid value '{s}' for '--suites': valid values are {valid}") + }) + } +} + pub struct BenchConfig { pub test_file: PathBuf, pub quick_mode: bool, - pub suites: Option>, + pub suites: Option>, } fn warn_reset_hwm() { @@ -19,41 +76,91 @@ fn warn_reset_hwm() { pub fn run_all(config: &BenchConfig) -> Vec { let mut results = Vec::new(); - let should_run = |name: &str| -> bool { + let should_run = |suite: Suite| -> bool { match &config.suites { - Some(suites) => suites.iter().any(|s| s == name), + Some(suites) => suites.contains(&suite), None => true, } }; - if should_run("startup") { + if should_run(Suite::Startup) { warn_reset_hwm(); results.extend(crate::suites::startup::run(config)); } - if should_run("render") { + if should_run(Suite::Render) { warn_reset_hwm(); results.extend(crate::suites::render::run(config)); } - if should_run("jump") { + if should_run(Suite::Jump) { warn_reset_hwm(); results.extend(crate::suites::jump::run(config)); } - if should_run("memory") { + if should_run(Suite::Memory) { warn_reset_hwm(); results.extend(crate::suites::memory::run(config)); } - if should_run("growth") { + if should_run(Suite::Growth) { warn_reset_hwm(); results.extend(crate::suites::growth::run(config)); } - if should_run("rotation") { + if should_run(Suite::Rotation) { warn_reset_hwm(); results.extend(crate::suites::rotation::run(config)); } - if should_run("concurrent") { + if should_run(Suite::Concurrent) { warn_reset_hwm(); results.extend(crate::suites::concurrent::run(config)); } results } + +#[cfg(test)] +mod tests { + use super::Suite; + use std::str::FromStr; + + #[test] + fn parse_all_valid_suites() { + let expected = [ + ("startup", Suite::Startup), + ("render", Suite::Render), + ("jump", Suite::Jump), + ("memory", Suite::Memory), + ("growth", Suite::Growth), + ("rotation", Suite::Rotation), + ("concurrent", Suite::Concurrent), + ]; + for (s, expected_suite) in expected { + assert_eq!(Suite::from_str(s).unwrap(), expected_suite, "failed to parse '{s}'"); + } + } + + #[test] + fn misspelled_suite_returns_error() { + let err = Suite::from_str("startp").unwrap_err(); + assert!( + err.contains("invalid value 'startp'"), + "error should mention the bad value: {err}" + ); + } + + #[test] + fn error_message_lists_all_valid_values() { + let err = Suite::from_str("bogus").unwrap_err(); + for name in Suite::ALL.iter().map(|s| s.as_str()) { + assert!( + err.contains(name), + "error should list valid suite '{name}': {err}" + ); + } + } + + #[test] + fn mixed_valid_invalid_stops_at_first_error() { + // "startup" is valid, "zzz" is not — collect hits the first Err + let names = ["startup".to_string(), "zzz".to_string()]; + let result: Result, _> = names.iter().map(|s| s.parse()).collect(); + assert!(result.is_err()); + } +}