compiletest: Encapsulate all of the code that touches libtest

src/tools/compiletest/src/common.rs

@@ -9,9 +9,9 @@ use std::{fmt, iter};
 use build_helper::git::GitConfig;
 use semver::Version;
 use serde::de::{Deserialize, Deserializer, Error as _};
-use test::{ColorConfig, OutputFormat};
 
 pub use self::Mode::*;
+use crate::executor::{ColorConfig, OutputFormat};
 use crate::util::{PathBufExt, add_dylib_path};
 
 macro_rules! string_enum {

src/tools/compiletest/src/executor.rs

@@ -0,0 +1,156 @@
+//! This module encapsulates all of the code that interacts directly with
+//! libtest, to execute the collected tests.
+//!
+//! This will hopefully make it easier to migrate away from libtest someday.
+
+use std::borrow::Cow;
+use std::io;
+use std::sync::Arc;
+
+use crate::common::{Config, TestPaths};
+
+/// Delegates to libtest to run the list of collected tests.
+///
+/// Returns `Ok(true)` if all tests passed, or `Ok(false)` if one or more tests failed.
+pub(crate) fn execute_tests(config: &Config, tests: Vec<CollectedTest>) -> io::Result<bool> {
+    let opts = test_opts(config);
+    let tests = tests.into_iter().map(|t| t.into_libtest()).collect::<Vec<_>>();
+
+    test::run_tests_console(&opts, tests)
+}
+
+/// Information needed to create a `test::TestDescAndFn`.
+pub(crate) struct CollectedTest {
+    pub(crate) desc: CollectedTestDesc,
+    pub(crate) config: Arc<Config>,
+    pub(crate) testpaths: TestPaths,
+    pub(crate) revision: Option<String>,
+}
+
+/// Information needed to create a `test::TestDesc`.
+pub(crate) struct CollectedTestDesc {
+    pub(crate) name: String,
+    pub(crate) ignore: bool,
+    pub(crate) ignore_message: Option<Cow<'static, str>>,
+    pub(crate) should_panic: ShouldPanic,
+}
+
+impl CollectedTest {
+    fn into_libtest(self) -> test::TestDescAndFn {
+        let Self { desc, config, testpaths, revision } = self;
+        let CollectedTestDesc { name, ignore, ignore_message, should_panic } = desc;
+
+        // Libtest requires the ignore message to be a &'static str, so we might
+        // have to leak memory to create it. This is fine, as we only do so once
+        // per test, so the leak won't grow indefinitely.
+        let ignore_message = ignore_message.map(|msg| match msg {
+            Cow::Borrowed(s) => s,
+            Cow::Owned(s) => &*String::leak(s),
+        });
+
+        let desc = test::TestDesc {
+            name: test::DynTestName(name),
+            ignore,
+            ignore_message,
+            source_file: "",
+            start_line: 0,
+            start_col: 0,
+            end_line: 0,
+            end_col: 0,
+            should_panic: should_panic.to_libtest(),
+            compile_fail: false,
+            no_run: false,
+            test_type: test::TestType::Unknown,
+        };
+
+        // This closure is invoked when libtest returns control to compiletest
+        // to execute the test.
+        let testfn = test::DynTestFn(Box::new(move || {
+            crate::runtest::run(config, &testpaths, revision.as_deref());
+            Ok(())
+        }));
+
+        test::TestDescAndFn { desc, testfn }
+    }
+}
+
+/// Whether console output should be colored or not.
+#[derive(Copy, Clone, Default, Debug)]
+pub enum ColorConfig {
+    #[default]
+    AutoColor,
+    AlwaysColor,
+    NeverColor,
+}
+
+impl ColorConfig {
+    fn to_libtest(self) -> test::ColorConfig {
+        match self {
+            Self::AutoColor => test::ColorConfig::AutoColor,
+            Self::AlwaysColor => test::ColorConfig::AlwaysColor,
+            Self::NeverColor => test::ColorConfig::NeverColor,
+        }
+    }
+}
+
+/// Format of the test results output.
+#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
+pub enum OutputFormat {
+    /// Verbose output
+    Pretty,
+    /// Quiet output
+    #[default]
+    Terse,
+    /// JSON output
+    Json,
+}
+
+impl OutputFormat {
+    fn to_libtest(self) -> test::OutputFormat {
+        match self {
+            Self::Pretty => test::OutputFormat::Pretty,
+            Self::Terse => test::OutputFormat::Terse,
+            Self::Json => test::OutputFormat::Json,
+        }
+    }
+}
+
+/// Whether test is expected to panic or not.
+#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
+pub(crate) enum ShouldPanic {
+    No,
+    Yes,
+}
+
+impl ShouldPanic {
+    fn to_libtest(self) -> test::ShouldPanic {
+        match self {
+            Self::No => test::ShouldPanic::No,
+            Self::Yes => test::ShouldPanic::Yes,
+        }
+    }
+}
+
+fn test_opts(config: &Config) -> test::TestOpts {
+    test::TestOpts {
+        exclude_should_panic: false,
+        filters: config.filters.clone(),
+        filter_exact: config.filter_exact,
+        run_ignored: if config.run_ignored { test::RunIgnored::Yes } else { test::RunIgnored::No },
+        format: config.format.to_libtest(),
+        logfile: config.logfile.clone(),
+        run_tests: true,
+        bench_benchmarks: true,
+        nocapture: config.nocapture,
+        color: config.color.to_libtest(),
+        shuffle: false,
+        shuffle_seed: None,
+        test_threads: None,
+        skip: config.skip.clone(),
+        list: false,
+        options: test::Options::new(),
+        time_options: None,
+        force_run_in_process: false,
+        fail_fast: config.fail_fast,
+    }
+}

src/tools/compiletest/src/header.rs

@@ -11,6 +11,7 @@ use tracing::*;
 
 use crate::common::{Config, Debugger, FailMode, Mode, PassMode};
 use crate::debuggers::{extract_cdb_version, extract_gdb_version};
+use crate::executor::{CollectedTestDesc, ShouldPanic};
 use crate::header::auxiliary::{AuxProps, parse_and_update_aux};
 use crate::header::needs::CachedNeedsConditions;
 use crate::util::static_regex;
@@ -1355,15 +1356,15 @@ where
     Some((min, max))
 }
 
-pub fn make_test_description<R: Read>(
+pub(crate) fn make_test_description<R: Read>(
     config: &Config,
     cache: &HeadersCache,
-    name: test::TestName,
+    name: String,
     path: &Path,
     src: R,
     test_revision: Option<&str>,
     poisoned: &mut bool,
-) -> test::TestDesc {
+) -> CollectedTestDesc {
     let mut ignore = false;
     let mut ignore_message = None;
     let mut should_fail = false;
@@ -1387,10 +1388,7 @@ pub fn make_test_description<R: Read>(
                     match $e {
                         IgnoreDecision::Ignore { reason } => {
                             ignore = true;
-                            // The ignore reason must be a &'static str, so we have to leak memory to
+                            ignore_message = Some(reason.into());
-                            // create it. This is fine, as the header is parsed only at the start of
-                            // compiletest so it won't grow indefinitely.
-                            ignore_message = Some(&*Box::leak(Box::<str>::from(reason)));
                         }
                         IgnoreDecision::Error { message } => {
                             eprintln!("error: {}:{line_number}: {message}", path.display());
@@ -1431,25 +1429,12 @@ pub fn make_test_description<R: Read>(
     // since we run the pretty printer across all tests by default.
     // If desired, we could add a `should-fail-pretty` annotation.
     let should_panic = match config.mode {
-        crate::common::Pretty => test::ShouldPanic::No,
+        crate::common::Pretty => ShouldPanic::No,
-        _ if should_fail => test::ShouldPanic::Yes,
+        _ if should_fail => ShouldPanic::Yes,
-        _ => test::ShouldPanic::No,
+        _ => ShouldPanic::No,
     };
 
-    test::TestDesc {
+    CollectedTestDesc { name, ignore, ignore_message, should_panic }
-        name,
-        ignore,
-        ignore_message,
-        source_file: "",
-        start_line: 0,
-        start_col: 0,
-        end_line: 0,
-        end_col: 0,
-        should_panic,
-        compile_fail: false,
-        no_run: false,
-        test_type: test::TestType::Unknown,
-    }
 }
 
 fn ignore_cdb(config: &Config, line: &str) -> IgnoreDecision {

src/tools/compiletest/src/header/tests.rs

@@ -8,14 +8,15 @@ use super::{
     parse_normalize_rule,
 };
 use crate::common::{Config, Debugger, Mode};
+use crate::executor::{CollectedTestDesc, ShouldPanic};
 
 fn make_test_description<R: Read>(
     config: &Config,
-    name: test::TestName,
+    name: String,
     path: &Path,
     src: R,
     revision: Option<&str>,
-) -> test::TestDesc {
+) -> CollectedTestDesc {
     let cache = HeadersCache::load(config);
     let mut poisoned = false;
     let test = crate::header::make_test_description(
@@ -233,7 +234,7 @@ fn parse_rs(config: &Config, contents: &str) -> EarlyProps {
 }
 
 fn check_ignore(config: &Config, contents: &str) -> bool {
-    let tn = test::DynTestName(String::new());
+    let tn = String::new();
     let p = Path::new("a.rs");
     let d = make_test_description(&config, tn, p, std::io::Cursor::new(contents), None);
     d.ignore
@@ -242,13 +243,13 @@ fn check_ignore(config: &Config, contents: &str) -> bool {
 #[test]
 fn should_fail() {
     let config: Config = cfg().build();
-    let tn = test::DynTestName(String::new());
+    let tn = String::new();
     let p = Path::new("a.rs");
 
     let d = make_test_description(&config, tn.clone(), p, std::io::Cursor::new(""), None);
-    assert_eq!(d.should_panic, test::ShouldPanic::No);
+    assert_eq!(d.should_panic, ShouldPanic::No);
     let d = make_test_description(&config, tn, p, std::io::Cursor::new("//@ should-fail"), None);
-    assert_eq!(d.should_panic, test::ShouldPanic::Yes);
+    assert_eq!(d.should_panic, ShouldPanic::Yes);
 }
 
 #[test]

src/tools/compiletest/src/lib.rs

@@ -12,6 +12,7 @@ pub mod common;
 pub mod compute_diff;
 mod debuggers;
 pub mod errors;
+mod executor;
 pub mod header;
 mod json;
 mod raise_fd_limit;
@@ -32,7 +33,6 @@ use std::{env, fs, vec};
 
 use build_helper::git::{get_git_modified_files, get_git_untracked_files};
 use getopts::Options;
-use test::ColorConfig;
 use tracing::*;
 use walkdir::WalkDir;
 
@@ -41,6 +41,7 @@ use crate::common::{
     CompareMode, Config, Debugger, Mode, PassMode, TestPaths, UI_EXTENSIONS, expected_output_path,
     output_base_dir, output_relative_path,
 };
+use crate::executor::{CollectedTest, ColorConfig, OutputFormat};
 use crate::header::HeadersCache;
 use crate::util::logv;
 
@@ -402,9 +403,9 @@ pub fn parse_config(args: Vec<String>) -> Config {
         verbose: matches.opt_present("verbose"),
         format: match (matches.opt_present("quiet"), matches.opt_present("json")) {
             (true, true) => panic!("--quiet and --json are incompatible"),
-            (true, false) => test::OutputFormat::Terse,
+            (true, false) => OutputFormat::Terse,
-            (false, true) => test::OutputFormat::Json,
+            (false, true) => OutputFormat::Json,
-            (false, false) => test::OutputFormat::Pretty,
+            (false, false) => OutputFormat::Pretty,
         },
         only_modified: matches.opt_present("only-modified"),
         color,
@@ -535,8 +536,6 @@ pub fn run_tests(config: Arc<Config>) {
     // Let tests know which target they're running as
     env::set_var("TARGET", &config.target);
 
-    let opts = test_opts(&config);
-
     let mut configs = Vec::new();
     if let Mode::DebugInfo = config.mode {
         // Debugging emscripten code doesn't make sense today
@@ -563,12 +562,12 @@ pub fn run_tests(config: Arc<Config>) {
         tests.extend(collect_and_make_tests(c));
     }
 
-    tests.sort_by(|a, b| a.desc.name.as_slice().cmp(&b.desc.name.as_slice()));
+    tests.sort_by(|a, b| Ord::cmp(&a.desc.name, &b.desc.name));
 
     // Delegate to libtest to filter and run the big list of structures created
-    // during test discovery. When libtest decides to run a test, it will invoke
+    // during test discovery. When libtest decides to run a test, it will
-    // the corresponding closure created by `make_test_closure`.
+    // return control to compiletest by invoking a closure.
-    let res = test::run_tests_console(&opts, tests);
+    let res = crate::executor::execute_tests(&config, tests);
 
     // Check the outcome reported by libtest.
     match res {
@@ -612,30 +611,6 @@ pub fn run_tests(config: Arc<Config>) {
     }
 }
 
-pub fn test_opts(config: &Config) -> test::TestOpts {
-    test::TestOpts {
-        exclude_should_panic: false,
-        filters: config.filters.clone(),
-        filter_exact: config.filter_exact,
-        run_ignored: if config.run_ignored { test::RunIgnored::Yes } else { test::RunIgnored::No },
-        format: config.format,
-        logfile: config.logfile.clone(),
-        run_tests: true,
-        bench_benchmarks: true,
-        nocapture: config.nocapture,
-        color: config.color,
-        shuffle: false,
-        shuffle_seed: None,
-        test_threads: None,
-        skip: config.skip.clone(),
-        list: false,
-        options: test::Options::new(),
-        time_options: None,
-        force_run_in_process: false,
-        fail_fast: config.fail_fast,
-    }
-}
-
 /// Read-only context data used during test collection.
 struct TestCollectorCx {
     config: Arc<Config>,
@@ -646,17 +621,17 @@ struct TestCollectorCx {
 
 /// Mutable state used during test collection.
 struct TestCollector {
-    tests: Vec<test::TestDescAndFn>,
+    tests: Vec<CollectedTest>,
     found_path_stems: HashSet<PathBuf>,
     poisoned: bool,
 }
 
-/// Creates libtest structures for every test/revision in the test suite directory.
+/// Creates test structures for every test/revision in the test suite directory.
 ///
 /// This always inspects _all_ test files in the suite (e.g. all 17k+ ui tests),
 /// regardless of whether any filters/tests were specified on the command-line,
 /// because filtering is handled later by libtest.
-pub fn collect_and_make_tests(config: Arc<Config>) -> Vec<test::TestDescAndFn> {
+pub(crate) fn collect_and_make_tests(config: Arc<Config>) -> Vec<CollectedTest> {
     debug!("making tests from {}", config.src_test_suite_root.display());
     let common_inputs_stamp = common_inputs_stamp(&config);
     let modified_tests =
@@ -885,7 +860,7 @@ fn make_test(cx: &TestCollectorCx, collector: &mut TestCollector, testpaths: &Te
     };
 
     // For each revision (or the sole dummy revision), create and append a
-    // `test::TestDescAndFn` that can be handed over to libtest.
+    // `CollectedTest` that can be handed over to the test executor.
     collector.tests.extend(revisions.into_iter().map(|revision| {
         // Create a test name and description to hand over to libtest.
         let src_file = fs::File::open(&test_path).expect("open test file to parse ignores");
@@ -908,13 +883,14 @@ fn make_test(cx: &TestCollectorCx, collector: &mut TestCollector, testpaths: &Te
         if !cx.config.force_rerun && is_up_to_date(cx, testpaths, &early_props, revision) {
             desc.ignore = true;
             // Keep this in sync with the "up-to-date" message detected by bootstrap.
-            desc.ignore_message = Some("up-to-date");
+            desc.ignore_message = Some("up-to-date".into());
         }
 
-        // Create the callback that will run this test/revision when libtest calls it.
+        let config = Arc::clone(&cx.config);
-        let testfn = make_test_closure(Arc::clone(&cx.config), testpaths, revision);
+        let testpaths = testpaths.clone();
+        let revision = revision.map(str::to_owned);
 
-        test::TestDescAndFn { desc, testfn }
+        CollectedTest { desc, config, testpaths, revision }
     }));
 }
 
@@ -1046,11 +1022,7 @@ impl Stamp {
 }
 
 /// Creates a name for this test/revision that can be handed over to libtest.
-fn make_test_name(
+fn make_test_name(config: &Config, testpaths: &TestPaths, revision: Option<&str>) -> String {
-    config: &Config,
-    testpaths: &TestPaths,
-    revision: Option<&str>,
-) -> test::TestName {
     // Print the name of the file, relative to the sources root.
     let path = testpaths.file.strip_prefix(&config.src_root).unwrap();
     let debugger = match config.debugger {
@@ -1062,32 +1034,14 @@ fn make_test_name(
         None => String::new(),
     };
 
-    test::DynTestName(format!(
+    format!(
         "[{}{}{}] {}{}",
         config.mode,
         debugger,
         mode_suffix,
         path.display(),
         revision.map_or("".to_string(), |rev| format!("#{}", rev))
-    ))
+    )
-}
-
-/// Creates a callback for this test/revision that libtest will call when it
-/// decides to actually run the underlying test.
-fn make_test_closure(
-    config: Arc<Config>,
-    testpaths: &TestPaths,
-    revision: Option<&str>,
-) -> test::TestFn {
-    let testpaths = testpaths.clone();
-    let revision = revision.map(str::to_owned);
-
-    // This callback is the link between compiletest's test discovery code,
-    // and the parts of compiletest that know how to run an individual test.
-    test::DynTestFn(Box::new(move || {
-        runtest::run(config, &testpaths, revision.as_deref());
-        Ok(())
-    }))
 }
 
 /// Checks that test discovery didn't find any tests whose name stem is a prefix