Rollup merge of #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: matthiaskrgr

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]

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;
 
@@ -50,6 +51,12 @@ use crate::util::logv;
 /// some code here that inspects environment variables or even runs executables
 /// (e.g. when discovering debugger versions).
 pub fn parse_config(args: Vec<String>) -> Config {
+    if env::var("RUST_TEST_NOCAPTURE").is_ok() {
+        eprintln!(
+            "WARNING: RUST_TEST_NOCAPTURE is not supported. Use the `--no-capture` flag instead."
+        );
+    }
+
     let mut opts = Options::new();
     opts.reqopt("", "compile-lib-path", "path to host shared libraries", "PATH")
         .reqopt("", "run-lib-path", "path to target shared libraries", "PATH")
@@ -128,6 +135,7 @@ pub fn parse_config(args: Vec<String>) -> Config {
             "bless",
             "overwrite stderr/stdout files instead of complaining about a mismatch",
         )
+        .optflag("", "fail-fast", "stop as soon as possible after any test fails")
         .optflag("", "quiet", "print one character per test instead of one line")
         .optopt("", "color", "coloring: auto, always, never", "WHEN")
         .optflag("", "json", "emit json output instead of plaintext output")
@@ -319,6 +327,9 @@ pub fn parse_config(args: Vec<String>) -> Config {
 
     Config {
         bless: matches.opt_present("bless"),
+        fail_fast: matches.opt_present("fail-fast")
+            || env::var_os("RUSTC_TEST_FAIL_FAST").is_some(),
+
         compile_lib_path: make_absolute(opt_path(matches, "compile-lib-path")),
         run_lib_path: make_absolute(opt_path(matches, "run-lib-path")),
         rustc_path: opt_path(matches, "rustc-path"),
@@ -392,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,
-            (false, true) => test::OutputFormat::Json,
-            (false, false) => test::OutputFormat::Pretty,
+            (true, false) => OutputFormat::Terse,
+            (false, true) => OutputFormat::Json,
+            (false, false) => OutputFormat::Pretty,
         },
         only_modified: matches.opt_present("only-modified"),
         color,
@@ -525,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
@@ -553,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
-    // the corresponding closure created by `make_test_closure`.
-    let res = test::run_tests_console(&opts, tests);
+    // during test discovery. When libtest decides to run a test, it will
+    // return control to compiletest by invoking a closure.
+    let res = crate::executor::execute_tests(&config, tests);
 
     // Check the outcome reported by libtest.
     match res {
@@ -602,37 +611,6 @@ pub fn run_tests(config: Arc<Config>) {
     }
 }
 
-pub fn test_opts(config: &Config) -> test::TestOpts {
-    if env::var("RUST_TEST_NOCAPTURE").is_ok() {
-        eprintln!(
-            "WARNING: RUST_TEST_NOCAPTURE is no longer used. \
-                   Use the `--nocapture` flag instead."
-        );
-    }
-
-    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: std::env::var_os("RUSTC_TEST_FAIL_FAST").is_some(),
-    }
-}
-
 /// Read-only context data used during test collection.
 struct TestCollectorCx {
     config: Arc<Config>,
@@ -643,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 =
@@ -882,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");
@@ -905,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 testfn = make_test_closure(Arc::clone(&cx.config), testpaths, revision);
+        let config = Arc::clone(&cx.config);
+        let testpaths = testpaths.clone();
+        let revision = revision.map(str::to_owned);
 
-        test::TestDescAndFn { desc, testfn }
+        CollectedTest { desc, config, testpaths, revision }
     }));
 }
 
@@ -1043,11 +1022,7 @@ impl Stamp {
 }
 
 /// Creates a name for this test/revision that can be handed over to libtest.
-fn make_test_name(
-    config: &Config,
-    testpaths: &TestPaths,
-    revision: Option<&str>,
-) -> test::TestName {
+fn make_test_name(config: &Config, testpaths: &TestPaths, revision: Option<&str>) -> String {
     // 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 {
@@ -1059,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