From fbdbc35f1573c73d044484886058f99ad986a8d3 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 24 Aug 2024 13:12:28 -0700 Subject: [PATCH] Add ScenarioOutput concept --- src/lab.rs | 11 ++--- src/log_file.rs | 35 +++++----------- src/output.rs | 105 +++++++++++++++++++++++++++++++----------------- src/scenario.rs | 7 ---- tests/main.rs | 9 ++++- 5 files changed, 92 insertions(+), 75 deletions(-) diff --git a/src/lab.rs b/src/lab.rs index 172c5549..95444c92 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -11,6 +11,7 @@ use std::thread; use std::time::Instant; use itertools::Itertools; +use output::ScenarioOutput; use tracing::{debug, debug_span, error, trace, warn}; use crate::cargo::run_cargo; @@ -209,13 +210,13 @@ fn test_scenario( options: &Options, console: &Console, ) -> Result { - let mut log_file = output_mutex + let scenario_output = output_mutex .lock() - .expect("lock output_dir to create log") - .create_log(scenario)?; + .expect("lock output_dir to start scenario") + .start_scenario(scenario)?; + let ScenarioOutput { mut log_file, .. } = scenario_output; log_file.message(&scenario.to_string()); console.scenario_started(build_dir.path().as_ref(), scenario, log_file.path())?; - let diff_filename = output_mutex.lock().unwrap().write_diff_file(scenario)?; let phases: &[Phase] = if options.check_only { &[Phase::Check] @@ -234,7 +235,7 @@ fn test_scenario( let dir: &Path = build_dir.path().as_ref(); console.scenario_started(dir, scenario, log_file.path())?; - let mut outcome = ScenarioOutcome::new(&log_file, &diff_filename, scenario.clone()); + let mut outcome = ScenarioOutcome::new(&log_file, &scenario_output.diff_path, scenario.clone()); for &phase in phases { console.scenario_phase_started(dir, phase); let timeout = match phase { diff --git a/src/log_file.rs b/src/log_file.rs index e7db7a99..f078d36d 100644 --- a/src/log_file.rs +++ b/src/log_file.rs @@ -4,7 +4,7 @@ //! and test cases, mixed with commentary from cargo-mutants. use std::fs::{File, OpenOptions}; -use std::io::{self, Write}; +use std::io::Write; use anyhow::Context; use camino::{Utf8Path, Utf8PathBuf}; @@ -22,30 +22,15 @@ pub struct LogFile { } impl LogFile { - pub fn create_in(log_dir: &Utf8Path, scenario_name: &str) -> Result { - let basename = clean_filename(scenario_name); - for i in 0..1000 { - let t = if i == 0 { - format!("{basename}.log") - } else { - format!("{basename}_{i:03}.log") - }; - let path = log_dir.join(t); - match OpenOptions::new() - .read(true) - .append(true) - .create_new(true) - .open(&path) - { - Ok(write_to) => return Ok(LogFile { path, write_to }), - Err(e) if e.kind() == io::ErrorKind::AlreadyExists => continue, - Err(e) => return Err(anyhow::Error::from(e).context("create test log file")), - } - } - unreachable!( - "couldn't create any test log in {:?} for {:?}", - log_dir, scenario_name, - ); + pub fn create_in(log_dir: &Utf8Path, basename: &str) -> Result { + let path = log_dir.join(format!("{basename}.log")); + let write_to = OpenOptions::new() + .create_new(true) + .read(true) + .append(true) + .open(&path) + .with_context(|| format!("create test log file {path:?}"))?; + Ok(LogFile { path, write_to }) } /// Open the log file to append more content. diff --git a/src/output.rs b/src/output.rs index 962fc88e..e047f1aa 100644 --- a/src/output.rs +++ b/src/output.rs @@ -2,6 +2,8 @@ //! A `mutants.out` directory holding logs and other output. +use std::collections::hash_map::Entry; +use std::collections::HashMap; use std::fs::{create_dir, remove_dir_all, rename, write, File, OpenOptions}; use std::io::{BufWriter, Write}; use std::path::Path; @@ -87,7 +89,6 @@ impl LockFile { pub struct OutputDir { path: Utf8PathBuf, log_dir: Utf8PathBuf, - diff_dir: Utf8PathBuf, #[allow(unused)] // Lifetime controls the file lock lock_file: File, /// A file holding a list of missed mutants as text, one per line. @@ -99,6 +100,12 @@ pub struct OutputDir { unviable_list: File, /// The accumulated overall lab outcome. pub lab_outcome: LabOutcome, + /// Incrementing sequence numbers for each scenario, so that they can each have a unique + /// filename. + pub scenario_index: usize, + /// Log filenames which have already been used, and the number of times that each + /// basename has been used. + used_log_names: HashMap, } impl OutputDir { @@ -120,7 +127,7 @@ impl OutputDir { if output_dir.exists() { LockFile::acquire_lock(output_dir.as_ref())?; // Now release the lock for a bit while we move the directory. This might be - // slightly racy. + // slightly racy. Maybe we should move the lock outside the directory. let rotated = in_dir.join(ROTATED_NAME); if rotated.exists() { @@ -157,24 +164,54 @@ impl OutputDir { path: output_dir, lab_outcome: LabOutcome::new(), log_dir, - diff_dir, lock_file, missed_list, caught_list, timeout_list, unviable_list, + scenario_index: 0, + used_log_names: HashMap::new(), }) } - /// Create a new log for a given scenario. - /// - /// Returns the [File] to which subprocess output should be sent, and a LogFile to read it - /// later. - pub fn create_log(&self, scenario: &Scenario) -> Result { - LogFile::create_in(&self.log_dir, &scenario.log_file_name_base()) + /// Allocate a sequence number and the output files for a scenario. + pub fn start_scenario(&mut self, scenario: &Scenario) -> Result { + let scenario_name = match scenario { + Scenario::Baseline => "baseline".into(), + Scenario::Mutant(mutant) => mutant.log_file_name_base(), + }; + let basename = match self.used_log_names.entry(scenario_name.clone()) { + Entry::Occupied(mut e) => { + let index = e.get_mut(); + *index += 1; + format!("{scenario_name}_{index:03}") + } + Entry::Vacant(e) => { + e.insert(0); + scenario_name + } + }; + // TODO: Maybe store pathse relative to the output directory; it would be more useful + // if the whole directory is later archived and moved. + let log_file = LogFile::create_in(&self.log_dir, &basename)?; + // TODO: Don't write a diff for the baseline? + let diff_path = Utf8PathBuf::from(format!("diff/{basename}.diff")); + let diff = if let Scenario::Mutant(mutant) = scenario { + // TODO: This calculates the mutated text again, and perhaps we could do it + // only once in the caller. + mutant.diff() + } else { + String::new() + }; + let full_diff_path = self.path().join(&diff_path); + write(&full_diff_path, diff.as_bytes()) + .with_context(|| format!("write diff file {full_diff_path:?}"))?; + Ok(ScenarioOutput { + log_file, + diff_path, + }) } - #[allow(dead_code)] /// Return the path of the `mutants.out` directory. pub fn path(&self) -> &Utf8Path { &self.path @@ -209,23 +246,6 @@ impl OutputDir { Ok(()) } - pub fn write_diff_file(&self, scenario: &Scenario) -> Result { - // TODO: Unify with code to calculate log file names; maybe OutputDir should assign unique - // names? - let diff_filename = self - .diff_dir - .join(format!("{}.diff", scenario.log_file_name_base())); - let diff = if let Scenario::Mutant(mutant) = scenario { - // TODO: Calculate the mutant text only once? - mutant.diff() - } else { - String::new() - }; - write(&diff_filename, diff.as_bytes()) - .with_context(|| format!("write diff file {diff_filename:?}"))?; - Ok(diff_filename) - } - pub fn open_debug_log(&self) -> Result { let debug_log_path = self.path.join("debug.log"); OpenOptions::new() @@ -286,6 +306,20 @@ pub fn load_previously_caught(output_parent_dir: &Utf8Path) -> Result &Utf8Path { + self.log_file.path() + } +} + #[cfg(test)] mod test { use std::fs::write; @@ -366,17 +400,14 @@ mod test { let temp_dir_path = Utf8Path::from_path(temp_dir.path()).unwrap(); // Create an initial output dir with one log. - let output_dir = OutputDir::new(temp_dir_path).unwrap(); - output_dir.create_log(&Scenario::Baseline).unwrap(); - assert!(temp_dir - .path() - .join("mutants.out/log/baseline.log") - .is_file()); + let mut output_dir = OutputDir::new(temp_dir_path).unwrap(); + let _scenario_output = output_dir.start_scenario(&Scenario::Baseline).unwrap(); + assert!(temp_dir_path.join("mutants.out/log/baseline.log").is_file()); drop(output_dir); // release the lock. // The second time we create it in the same directory, the old one is moved away. - let output_dir = OutputDir::new(temp_dir_path).unwrap(); - output_dir.create_log(&Scenario::Baseline).unwrap(); + let mut output_dir = OutputDir::new(temp_dir_path).unwrap(); + output_dir.start_scenario(&Scenario::Baseline).unwrap(); assert!(temp_dir .path() .join("mutants.out.old/log/baseline.log") @@ -388,8 +419,8 @@ mod test { drop(output_dir); // The third time (and later), the .old directory is removed. - let output_dir = OutputDir::new(temp_dir_path).unwrap(); - output_dir.create_log(&Scenario::Baseline).unwrap(); + let mut output_dir = OutputDir::new(temp_dir_path).unwrap(); + output_dir.start_scenario(&Scenario::Baseline).unwrap(); assert!(temp_dir .path() .join("mutants.out/log/baseline.log") diff --git a/src/scenario.rs b/src/scenario.rs index 413fdc8c..ea616d91 100644 --- a/src/scenario.rs +++ b/src/scenario.rs @@ -35,11 +35,4 @@ impl Scenario { Scenario::Mutant(mutant) => Some(mutant), } } - - pub fn log_file_name_base(&self) -> String { - match self { - Scenario::Baseline => "baseline".into(), - Scenario::Mutant(mutant) => mutant.log_file_name_base(), - } - } } diff --git a/tests/main.rs b/tests/main.rs index 25e95255..4f8b67e4 100644 --- a/tests/main.rs +++ b/tests/main.rs @@ -94,7 +94,14 @@ fn tree_with_child_directories_is_well_tested() { let mut all_diffs = HashSet::new(); for outcome_json in json["outcomes"].as_array().unwrap() { let diff_path = outcome_json["diff_path"].as_str().unwrap(); - assert!(Utf8Path::new(diff_path).is_file()); + assert!( + tmp_src_dir + .path() + .join("mutants.out") + .join(diff_path) + .is_file(), + "{diff_path:?} is not a file" + ); assert!(all_diffs.insert(diff_path)); } }