Add ScenarioOutput concept

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<ScenarioOutcome> {
-    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 {

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<LogFile> {
-        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<LogFile> {
+        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.

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<String, usize>,
 }
 
 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> {
-        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<ScenarioOutput> {
+        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<Utf8PathBuf> {
-        // 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<File> {
         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<Vec<String
     Ok(r)
 }
 
+/// Where to write output about a particular Scenario.
+// TODO: Maybe merge with LogFile?
+pub struct ScenarioOutput {
+    pub log_file: LogFile,
+    /// Dif
+    pub diff_path: Utf8PathBuf,
+}
+
+impl ScenarioOutput {
+    pub fn log_file_path(&self) -> &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")

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(),
-        }
-    }
 }

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));
     }
 }