Rollup merge of rust-lang#139317 - Zalathar:hide-libtest, r=jieyouxu

compiletest: Encapsulate all of the code that touches libtest

Compiletest currently relies on unstable libtest APIs in order to actually execute tests. That's unfortunate, but removing the dependency isn't trivial.

However, we can make a small step towards removing the libtest dependency by encapsulating the libtest interactions into a single dedicated module. That makes it easier to see what parts of libtest are actually used.

---

As a side-effect of moving the `test_opts` function into that dedicated module, this PR also ends up allowing `--fail-fast` to be passed on the command line, instead of requiring an environment variable.

---

There is still (at least) one other aspect of the libtest dependency that this PR does not address, namely the fact that we rely on libtest's output capture (via unstable std APIs) to capture the output that we print during individual tests. I hope to do something about that at some point.

r? jieyouxu

Author: Zalathar

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 {
@@ -178,6 +178,10 @@ pub struct Config {
     /// `true` to overwrite stderr/stdout files instead of complaining about changes in output.
     pub bless: bool,
 
+    /// Stop as soon as possible after any test fails.
+    /// May run a few more tests before stopping, due to threading.
+    pub fail_fast: bool,
+
     /// The library paths required for running the compiler.
     pub compile_lib_path: PathBuf,
 

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
-                            // 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)));
+                            ignore_message = Some(reason.into());
                         }
                         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,
-        _ if should_fail => test::ShouldPanic::Yes,
-        _ => test::ShouldPanic::No,
+        crate::common::Pretty => ShouldPanic::No,
+        _ if should_fail => ShouldPanic::Yes,
+        _ => ShouldPanic::No,
     };
 
-    test::TestDesc {
-        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,
-    }
+    CollectedTestDesc { name, ignore, ignore_message, should_panic }
 }
 
 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]