Merge pull request #297 from oli-obk/push-uuyxytltvwqn

Make `Errored` debug rendering readable

Author: oli-obk

examples/rustc_basic.rs

@@ -1,14 +1,12 @@
 #[cfg(feature = "rustc")]
-use std::sync::atomic::Ordering;
-#[cfg(feature = "rustc")]
 use ui_test::{run_tests, Config};
 
 #[cfg(feature = "rustc")]
 #[cfg_attr(test, test)]
 fn main() -> ui_test::color_eyre::Result<()> {
     let config = Config::rustc("examples_tests/rustc_basic");
     let abort_check = config.abort_check.clone();
-    ctrlc::set_handler(move || abort_check.store(true, Ordering::Relaxed))?;
+    ctrlc::set_handler(move || abort_check.abort())?;
 
     // Compile all `.rs` files in the given directory (relative to your
     // Cargo.toml) and compare their output against the corresponding

examples/rustc_twice_with_different_flags.rs

@@ -1,6 +1,4 @@
 #[cfg(feature = "rustc")]
-use std::sync::atomic::Ordering;
-#[cfg(feature = "rustc")]
 use ui_test::{
     default_file_filter, default_per_file_config, run_tests_generic, status_emitter, Config,
 };
@@ -10,7 +8,7 @@ use ui_test::{
 fn main() -> ui_test::color_eyre::Result<()> {
     let config = Config::rustc("examples_tests/rustc_basic");
     let abort_check = config.abort_check.clone();
-    ctrlc::set_handler(move || abort_check.store(true, Ordering::Relaxed))?;
+    ctrlc::set_handler(move || abort_check.abort())?;
 
     // Compile all `.rs` files in the given directory (relative to your
     // Cargo.toml) and compare their output against the corresponding

src/aux_builds.rs

@@ -122,7 +122,7 @@ impl Build for AuxBuilder {
 
         aux_cmd.arg("--emit=link");
         let filename = self.aux_file.file_stem().unwrap().to_str().unwrap();
-        let output = aux_cmd.output().unwrap();
+        let output = config.config.run_command(&mut aux_cmd)?;
         if !output.status.success() {
             let error = Error::Command {
                 kind: "compilation of aux build failed".to_string(),
@@ -138,11 +138,7 @@ impl Build for AuxBuilder {
 
         // Now run the command again to fetch the output filenames
         aux_cmd.arg("--print").arg("file-names");
-        let output = aux_cmd.output().unwrap();
-
-        if build_manager.aborted() {
-            return Err(Errored::aborted());
-        }
+        let output = config.config.run_command(&mut aux_cmd)?;
 
         assert!(output.status.success());
 

src/build_manager.rs

@@ -1,8 +1,9 @@
 //! Auxiliary and dependency builder. Extendable to custom builds.
 
 use crate::{
+    per_test_config::TestConfig,
     status_emitter::{RevisionStyle, TestStatus},
-    test_result::TestRun,
+    test_result::{TestResult, TestRun},
     Config, Errored,
 };
 use color_eyre::eyre::Result;
@@ -27,7 +28,7 @@ pub trait Build {
 pub struct BuildManager {
     #[allow(clippy::type_complexity)]
     cache: RwLock<HashMap<String, Arc<OnceLock<Result<Vec<OsString>, ()>>>>>,
-    config: Config,
+    pub(crate) config: Config,
     new_job_submitter: Sender<NewJob>,
 }
 
@@ -52,12 +53,24 @@ impl BuildManager {
 
     /// Lazily add more jobs after a test has finished. These are added to the queue
     /// as normally, but nested below the test.
-    pub fn add_new_job(&self, job: impl Send + 'static + FnOnce() -> TestRun) {
+    pub fn add_new_job(
+        &self,
+        mut config: TestConfig,
+        job: impl Send + 'static + FnOnce(&mut TestConfig) -> TestResult,
+    ) {
         if self.aborted() {
             return;
         }
         self.new_job_submitter
-            .send(Box::new(move |sender| Ok(sender.send(job())?)))
+            .send(Box::new(move |sender| {
+                let result = job(&mut config);
+                let result = TestRun {
+                    result,
+                    status: config.status,
+                    abort_check: config.config.abort_check,
+                };
+                Ok(sender.send(result)?)
+            }))
             .unwrap()
     }
 
@@ -143,6 +156,6 @@ impl BuildManager {
 
     /// Whether the build was cancelled
     pub fn aborted(&self) -> bool {
-        self.config.aborted()
+        self.config.abort_check.aborted()
     }
 }

src/config.rs

@@ -8,7 +8,7 @@ use crate::{
     diagnostics::{self, Diagnostics},
     parser::CommandParserFunc,
     per_test_config::{Comments, Condition},
-    CommandBuilder, Error, Errors,
+    CommandBuilder, Error, Errored, Errors,
 };
 use color_eyre::eyre::Result;
 use regex::bytes::Regex;
@@ -17,6 +17,7 @@ use std::{
     collections::BTreeMap,
     num::NonZeroUsize,
     path::{Path, PathBuf},
+    process::{Command, Output},
     sync::{atomic::AtomicBool, Arc},
 };
 
@@ -61,10 +62,26 @@ pub struct Config {
     pub custom_comments: BTreeMap<&'static str, CommandParserFunc>,
     /// Custom diagnostic extractor (invoked on the output of tests)
     pub diagnostic_extractor: fn(&Path, &[u8]) -> Diagnostics,
-    /// An atomic bool that can be set to `true` to abort all tests.
-    /// Will not cancel child processes, but if set from a Ctrl+C handler,
-    /// the pressing of Ctrl+C will already have cancelled child processes.
-    pub abort_check: Arc<AtomicBool>,
+    /// Handle to the global abort check.
+    pub abort_check: AbortCheck,
+}
+
+/// An atomic bool that can be set to `true` to abort all tests.
+/// Will not cancel child processes, but if set from a Ctrl+C handler,
+/// the pressing of Ctrl+C will already have cancelled child processes.
+#[derive(Clone, Debug, Default)]
+pub struct AbortCheck(Arc<AtomicBool>);
+
+impl AbortCheck {
+    /// Whether any test has been aborted.
+    pub fn aborted(&self) -> bool {
+        self.0.load(std::sync::atomic::Ordering::Relaxed)
+    }
+
+    /// Inform everyone that an abort has been requested
+    pub fn abort(&self) {
+        self.0.store(true, std::sync::atomic::Ordering::Relaxed)
+    }
 }
 
 /// Function that performs the actual output conflict handling.
@@ -438,8 +455,27 @@ impl Config {
             ^ self.run_only_ignored
     }
 
-    pub(crate) fn aborted(&self) -> bool {
-        self.abort_check.load(std::sync::atomic::Ordering::Relaxed)
+    pub(crate) fn aborted(&self) -> Result<(), Errored> {
+        if self.abort_check.aborted() {
+            Err(Errored::aborted())
+        } else {
+            Ok(())
+        }
+    }
+
+    pub(crate) fn run_command(&self, cmd: &mut Command) -> Result<Output, Errored> {
+        self.aborted()?;
+
+        let output = cmd.output().map_err(|err| Errored {
+            errors: vec![],
+            stderr: err.to_string().into_bytes(),
+            stdout: format!("could not spawn `{:?}` as a process", cmd.get_program()).into_bytes(),
+            command: format!("{cmd:?}"),
+        })?;
+
+        self.aborted()?;
+
+        Ok(output)
     }
 }
 

src/core.rs

@@ -1,6 +1,5 @@
 //! Basic operations useful for building a testsuite
 
-use crate::test_result::Errored;
 use color_eyre::eyre::Result;
 use crossbeam_channel::unbounded;
 use crossbeam_channel::Receiver;
@@ -10,23 +9,9 @@ use std::num::NonZeroUsize;
 use std::path::Component;
 use std::path::Path;
 use std::path::Prefix;
-use std::process::Command;
-use std::process::Output;
 use std::sync::OnceLock;
 use std::thread;
 
-pub(crate) fn run_command(cmd: &mut Command) -> Result<Output, Errored> {
-    match cmd.output() {
-        Err(err) => Err(Errored {
-            errors: vec![],
-            stderr: err.to_string().into_bytes(),
-            stdout: format!("could not spawn `{:?}` as a process", cmd.get_program()).into_bytes(),
-            command: format!("{cmd:?}"),
-        }),
-        Ok(output) => Ok(output),
-    }
-}
-
 /// Remove the common prefix of this path and the `root_dir`.
 pub(crate) fn strip_path_prefix<'a>(
     path: &'a Path,

src/custom_flags/run.rs

@@ -4,7 +4,6 @@ use super::Flag;
 use crate::{
     build_manager::BuildManager, display, per_test_config::TestConfig,
     status_emitter::RevisionStyle, CommandBuilder, Error, Errored, OutputConflictHandling, TestOk,
-    TestRun,
 };
 use bstr::ByteSlice;
 use spanned::Spanned;
@@ -45,16 +44,10 @@ impl Flag for Run {
         if let Some(och) = self.output_conflict_handling {
             config.config.output_conflict_handling = och;
         }
-        build_manager.add_new_job(move || {
+        build_manager.add_new_job(config, move |config| {
             cmd.arg("--print").arg("file-names");
             let output = cmd.output().unwrap();
-            if config.aborted() {
-                return TestRun {
-                    result: Err(Errored::aborted()),
-                    status: config.status,
-                    abort_check: config.config.abort_check.clone(),
-                };
-            }
+            config.aborted()?;
             assert!(output.status.success(), "{cmd:#?}: {output:#?}");
 
             let mut files = output.stdout.lines();
@@ -81,13 +74,7 @@ impl Flag for Run {
                 )
             });
 
-            if config.aborted() {
-                return TestRun {
-                    result: Err(Errored::aborted()),
-                    status: config.status,
-                    abort_check: config.config.abort_check.clone(),
-                };
-            }
+            config.aborted()?;
 
             let mut errors = vec![];
 
@@ -108,20 +95,15 @@ impl Flag for Run {
                     },
                 })
             }
-
-            TestRun {
-                result: if errors.is_empty() {
-                    Ok(TestOk::Ok)
-                } else {
-                    Err(Errored {
-                        command: format!("{exe:?}"),
-                        errors,
-                        stderr: output.stderr,
-                        stdout: output.stdout,
-                    })
-                },
-                status: config.status,
-                abort_check: config.config.abort_check,
+            if errors.is_empty() {
+                Ok(TestOk::Ok)
+            } else {
+                Err(Errored {
+                    command: format!("{exe:?}"),
+                    errors,
+                    stderr: output.stderr,
+                    stdout: output.stdout,
+                })
             }
         });
         Ok(())

src/custom_flags/rustfix.rs

@@ -6,7 +6,7 @@ use crate::{
     display,
     parser::OptWithLine,
     per_test_config::{Comments, Revisioned, TestConfig},
-    Error, Errored, TestOk, TestRun,
+    Error, Errored, TestOk,
 };
 use rustfix::{CodeFix, Filter, Suggestion};
 use spanned::{Span, Spanned};
@@ -214,11 +214,10 @@ fn compile_fixed(
         let mut cmd = fixed_config.build_command(build_manager)?;
         cmd.arg("--crate-name")
             .arg(format!("__{crate_name}_{}", i + 1));
-        build_manager.add_new_job(move || {
+        build_manager.add_new_job(fixed_config, move |fixed_config| {
             let output = cmd.output().unwrap();
-            let result = if fixed_config.aborted() {
-                Err(Errored::aborted())
-            } else if output.status.success() {
+            fixed_config.aborted()?;
+            if output.status.success() {
                 Ok(TestOk::Ok)
             } else {
                 let diagnostics = fixed_config.process(&output.stderr);
@@ -242,11 +241,6 @@ fn compile_fixed(
                     stderr: diagnostics.rendered,
                     stdout: output.stdout,
                 })
-            };
-            TestRun {
-                result,
-                status: fixed_config.status,
-                abort_check: fixed_config.config.abort_check,
             }
         });
     }

src/dependencies.rs

@@ -37,17 +37,7 @@ fn cfgs(config: &Config) -> Result<Vec<Cfg>, Errored> {
     let mut cmd = config.program.build(&config.out_dir);
     cmd.arg(cfg);
     cmd.arg("--target").arg(config.target.as_ref().unwrap());
-    let output = match cmd.output() {
-        Ok(o) => o,
-        Err(e) => {
-            return Err(Errored {
-                command: format!("{cmd:?}"),
-                stderr: e.to_string().into_bytes(),
-                stdout: vec![],
-                errors: vec![],
-            })
-        }
-    };
+    let output = config.run_command(&mut cmd)?;
 
     if !output.status.success() {
         return Err(Errored {
@@ -106,17 +96,7 @@ fn build_dependencies_inner(
 
     build.arg("--message-format=json");
 
-    let output = match build.output() {
-        Err(e) => {
-            return Err(Errored {
-                command: format!("{build:?}"),
-                stderr: e.to_string().into_bytes(),
-                stdout: vec![],
-                errors: vec![],
-            })
-        }
-        Ok(o) => o,
-    };
+    let output = config.run_command(&mut build)?;
 
     if !output.status.success() {
         let stdout = output
@@ -212,17 +192,7 @@ fn build_dependencies_inner(
         .arg(&info.crate_manifest_path);
     info.program.apply_env(&mut metadata);
     set_locking(&mut metadata);
-    let output = match metadata.output() {
-        Err(e) => {
-            return Err(Errored {
-                command: format!("{metadata:?}"),
-                errors: vec![],
-                stderr: e.to_string().into_bytes(),
-                stdout: vec![],
-            })
-        }
-        Ok(output) => output,
-    };
+    let output = config.run_command(&mut metadata)?;
 
     if !output.status.success() {
         return Err(Errored {