Rollup merge of #145926 - Zalathar:no-libtest, r=jieyouxu

compiletest: Remove several remnants of the old libtest-based executor

I noticed a few bits of low-hanging cleanup that are possible now that the non-libtest executor is well and truly established.

Author: Zalathar

src/bootstrap/src/core/build_steps/test.rs

@@ -2025,8 +2025,6 @@ HELP: You can add it into `bootstrap.toml` in `rust.codegen-backends = [{name:?}
             cmd.arg("--verbose");
         }
 
-        cmd.arg("--json");
-
         if builder.config.rustc_debug_assertions {
             cmd.arg("--with-rustc-debug-assertions");
         }

src/tools/compiletest/src/common.rs

@@ -8,7 +8,7 @@ use camino::{Utf8Path, Utf8PathBuf};
 use semver::Version;
 use serde::de::{Deserialize, Deserializer, Error as _};
 
-use crate::executor::{ColorConfig, OutputFormat};
+use crate::executor::ColorConfig;
 use crate::fatal;
 use crate::util::{Utf8PathBufExt, add_dylib_path, string_enum};
 
@@ -565,13 +565,6 @@ pub struct Config {
     /// FIXME: this is *way* too coarse; the user can't select *which* info to verbosely dump.
     pub verbose: bool,
 
-    /// (Useless) Adjust libtest output format.
-    ///
-    /// FIXME: the hand-rolled executor does not support non-JSON output, because `compiletest` need
-    /// to package test outcome as `libtest`-esque JSON that `bootstrap` can intercept *anyway*.
-    /// However, now that we don't use the `libtest` executor, this is useless.
-    pub format: OutputFormat,
-
     /// Whether to use colors in test output.
     ///
     /// Note: the exact control mechanism is delegated to [`colored`].
@@ -768,7 +761,6 @@ impl Config {
             adb_device_status: Default::default(),
             lldb_python_dir: Default::default(),
             verbose: Default::default(),
-            format: Default::default(),
             color: Default::default(),
             remote_test_client: Default::default(),
             compare_mode: Default::default(),

src/tools/compiletest/src/executor.rs

@@ -1,5 +1,9 @@
 //! This module contains a reimplementation of the subset of libtest
 //! functionality needed by compiletest.
+//!
+//! FIXME(Zalathar): Much of this code was originally designed to mimic libtest
+//! as closely as possible, for ease of migration. Now that libtest is no longer
+//! used, we can potentially redesign things to be a better fit for compiletest.
 
 use std::borrow::Cow;
 use std::collections::HashMap;
@@ -207,7 +211,7 @@ impl TestOutcome {
 ///
 /// Adapted from `filter_tests` in libtest.
 ///
-/// FIXME(#139660): After the libtest dependency is removed, redesign the whole filtering system to
+/// FIXME(#139660): Now that libtest has been removed, redesign the whole filtering system to
 /// do a better job of understanding and filtering _paths_, instead of being tied to libtest's
 /// substring/exact matching behaviour.
 fn filter_tests(opts: &Config, tests: Vec<CollectedTest>) -> Vec<CollectedTest> {
@@ -249,15 +253,15 @@ fn get_concurrency() -> usize {
     }
 }
 
-/// Information needed to create a `test::TestDescAndFn`.
+/// Information that was historically needed to create a libtest `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`.
+/// Information that was historically needed to create a libtest `TestDesc`.
 pub(crate) struct CollectedTestDesc {
     pub(crate) name: String,
     pub(crate) ignore: bool,
@@ -274,18 +278,6 @@ pub enum 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,
-}
-
 /// Whether test is expected to panic or not.
 #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
 pub(crate) enum ShouldPanic {

src/tools/compiletest/src/lib.rs

@@ -43,7 +43,7 @@ use crate::common::{
     expected_output_path, output_base_dir, output_relative_path,
 };
 use crate::directives::DirectivesCache;
-use crate::executor::{CollectedTest, ColorConfig, OutputFormat};
+use crate::executor::{CollectedTest, ColorConfig};
 use crate::util::logv;
 
 /// Creates the `Config` instance for this invocation of compiletest.
@@ -137,9 +137,7 @@ pub fn parse_config(args: Vec<String>) -> Config {
             "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")
         .optopt("", "target", "the target to build for", "TARGET")
         .optopt("", "host", "the host to build for", "HOST")
         .optopt("", "cdb", "path to CDB to use for CDB debuginfo tests", "PATH")
@@ -203,7 +201,6 @@ pub fn parse_config(args: Vec<String>) -> Config {
             "COMMAND",
         )
         .reqopt("", "minicore-path", "path to minicore aux library", "PATH")
-        .optflag("N", "no-new-executor", "disables the new test executor, and uses libtest instead")
         .optopt(
             "",
             "debugger",
@@ -436,12 +433,6 @@ pub fn parse_config(args: Vec<String>) -> Config {
             && !opt_str2(matches.opt_str("adb-test-dir")).is_empty(),
         lldb_python_dir: matches.opt_str("lldb-python-dir"),
         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) => OutputFormat::Terse,
-            (false, true) => OutputFormat::Json,
-            (false, false) => OutputFormat::Pretty,
-        },
         only_modified: matches.opt_present("only-modified"),
         color,
         remote_test_client: matches.opt_str("remote-test-client").map(Utf8PathBuf::from),
@@ -527,7 +518,6 @@ pub fn log_config(config: &Config) {
     logv(c, format!("target-linker: {:?}", config.target_linker));
     logv(c, format!("host-linker: {:?}", config.host_linker));
     logv(c, format!("verbose: {}", config.verbose));
-    logv(c, format!("format: {:?}", config.format));
     logv(c, format!("minicore_path: {}", config.minicore_path));
     logv(c, "\n".to_string());
 }
@@ -601,7 +591,7 @@ pub fn run_tests(config: Arc<Config>) {
         configs.push(config.clone());
     };
 
-    // Discover all of the tests in the test suite directory, and build a libtest
+    // Discover all of the tests in the test suite directory, and build a `CollectedTest`
     // structure for each test (or each revision of a multi-revision test).
     let mut tests = Vec::new();
     for c in configs {
@@ -613,50 +603,35 @@ pub fn run_tests(config: Arc<Config>) {
     // Delegate to the executor to filter and run the big list of test structures
     // created during test discovery. When the executor decides to run a test,
     // it will return control to the rest of compiletest by calling `runtest::run`.
-    // FIXME(Zalathar): Once we're confident that we won't need to revert the
-    // removal of the libtest-based executor, remove this Result and other
-    // remnants of the old executor.
-    let res: io::Result<bool> = Ok(executor::run_tests(&config, tests));
-
-    // Check the outcome reported by libtest.
-    match res {
-        Ok(true) => {}
-        Ok(false) => {
-            // We want to report that the tests failed, but we also want to give
-            // some indication of just what tests we were running. Especially on
-            // CI, where there can be cross-compiled tests for a lot of
-            // architectures, without this critical information it can be quite
-            // easy to miss which tests failed, and as such fail to reproduce
-            // the failure locally.
-
-            let mut msg = String::from("Some tests failed in compiletest");
-            write!(msg, " suite={}", config.suite).unwrap();
-
-            if let Some(compare_mode) = config.compare_mode.as_ref() {
-                write!(msg, " compare_mode={}", compare_mode).unwrap();
-            }
+    let ok = executor::run_tests(&config, tests);
+
+    // Check the outcome reported by the executor.
+    if !ok {
+        // We want to report that the tests failed, but we also want to give
+        // some indication of just what tests we were running. Especially on
+        // CI, where there can be cross-compiled tests for a lot of
+        // architectures, without this critical information it can be quite
+        // easy to miss which tests failed, and as such fail to reproduce
+        // the failure locally.
+
+        let mut msg = String::from("Some tests failed in compiletest");
+        write!(msg, " suite={}", config.suite).unwrap();
+
+        if let Some(compare_mode) = config.compare_mode.as_ref() {
+            write!(msg, " compare_mode={}", compare_mode).unwrap();
+        }
 
-            if let Some(pass_mode) = config.force_pass_mode.as_ref() {
-                write!(msg, " pass_mode={}", pass_mode).unwrap();
-            }
+        if let Some(pass_mode) = config.force_pass_mode.as_ref() {
+            write!(msg, " pass_mode={}", pass_mode).unwrap();
+        }
 
-            write!(msg, " mode={}", config.mode).unwrap();
-            write!(msg, " host={}", config.host).unwrap();
-            write!(msg, " target={}", config.target).unwrap();
+        write!(msg, " mode={}", config.mode).unwrap();
+        write!(msg, " host={}", config.host).unwrap();
+        write!(msg, " target={}", config.target).unwrap();
 
-            println!("{msg}");
+        println!("{msg}");
 
-            std::process::exit(1);
-        }
-        Err(e) => {
-            // We don't know if tests passed or not, but if there was an error
-            // during testing we don't want to just succeed (we may not have
-            // tested something), so fail.
-            //
-            // This should realistically "never" happen, so don't try to make
-            // this a pretty error message.
-            panic!("I/O failure during tests: {:?}", e);
-        }
+        std::process::exit(1);
     }
 }
 
@@ -691,7 +666,11 @@ impl TestCollector {
 ///
 /// 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.
+/// because filtering is handled later by code that was copied from libtest.
+///
+/// FIXME(Zalathar): Now that we no longer rely on libtest, try to overhaul
+/// test discovery to take into account the filters/tests specified on the
+/// command-line, instead of having to enumerate everything.
 pub(crate) fn collect_and_make_tests(config: Arc<Config>) -> Vec<CollectedTest> {
     debug!("making tests from {}", config.src_test_suite_root);
     let common_inputs_stamp = common_inputs_stamp(&config);
@@ -805,7 +784,7 @@ fn modified_tests(config: &Config, dir: &Utf8Path) -> Result<Vec<Utf8PathBuf>, S
 }
 
 /// Recursively scans a directory to find test files and create test structures
-/// that will be handed over to libtest.
+/// that will be handed over to the executor.
 fn collect_tests_from_dir(
     cx: &TestCollectorCx,
     dir: &Utf8Path,
@@ -871,7 +850,7 @@ fn collect_tests_from_dir(
             if is_test(file_name)
                 && (!cx.config.only_modified || cx.modified_tests.contains(&file_path))
             {
-                // We found a test file, so create the corresponding libtest structures.
+                // We found a test file, so create the corresponding test structures.
                 debug!(%file_path, "found test file");
 
                 // Record the stem of the test file, to check for overlaps later.
@@ -915,7 +894,7 @@ pub fn is_test(file_name: &str) -> bool {
 }
 
 /// For a single test file, creates one or more test structures (one per revision) that can be
-/// handed over to libtest to run, possibly in parallel.
+/// handed over to the executor to run, possibly in parallel.
 fn make_test(cx: &TestCollectorCx, collector: &mut TestCollector, testpaths: &TestPaths) {
     // For run-make tests, each "test file" is actually a _directory_ containing an `rmake.rs`. But
     // for the purposes of directive parsing, we want to look at that recipe file, not the directory
@@ -929,7 +908,7 @@ fn make_test(cx: &TestCollectorCx, collector: &mut TestCollector, testpaths: &Te
     // Scan the test file to discover its revisions, if any.
     let early_props = EarlyProps::from_file(&cx.config, &test_path);
 
-    // Normally we create one libtest structure per revision, with two exceptions:
+    // Normally we create one structure per revision, with two exceptions:
     // - If a test doesn't use revisions, create a dummy revision (None) so that
     //   the test can still run.
     // - Incremental tests inherently can't run their revisions in parallel, so
@@ -944,12 +923,12 @@ fn make_test(cx: &TestCollectorCx, collector: &mut TestCollector, testpaths: &Te
     // For each revision (or the sole dummy revision), create and append a
     // `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.
+        // Create a test name and description to hand over to the executor.
         let src_file = fs::File::open(&test_path).expect("open test file to parse ignores");
         let test_name = make_test_name(&cx.config, testpaths, revision);
-        // Create a libtest description for the test/revision.
+        // Create a description struct for the test/revision.
         // This is where `ignore-*`/`only-*`/`needs-*` directives are handled,
-        // because they need to set the libtest ignored flag.
+        // because they historically needed to set the libtest ignored flag.
         let mut desc = make_test_description(
             &cx.config,
             &cx.cache,
@@ -961,10 +940,12 @@ fn make_test(cx: &TestCollectorCx, collector: &mut TestCollector, testpaths: &Te
         );
 
         // If a test's inputs haven't changed since the last time it ran,
-        // mark it as ignored so that libtest will skip it.
+        // mark it as ignored so that the executor will skip it.
         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.
+            // FIXME(Zalathar): Now that we are no longer tied to libtest, we could
+            // find a less fragile way to communicate this status to bootstrap.
             desc.ignore_message = Some("up-to-date".into());
         }
 
@@ -1104,7 +1085,7 @@ impl Stamp {
     }
 }
 
-/// Creates a name for this test/revision that can be handed over to libtest.
+/// Creates a name for this test/revision that can be handed over to the executor.
 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();

src/tools/compiletest/src/runtest.rs

@@ -2222,7 +2222,7 @@ impl<'test> TestCx<'test> {
                 .env("PAGER", "")
                 .stdin(File::open(&diff_filename).unwrap())
                 // Capture output and print it explicitly so it will in turn be
-                // captured by libtest.
+                // captured by output-capture.
                 .output()
                 .unwrap();
             assert!(output.status.success());

src/tools/compiletest/src/runtest/run_make.rs

@@ -308,7 +308,7 @@ impl TestCx<'_> {
         let stdout = String::from_utf8_lossy(&stdout).into_owned();
         let stderr = String::from_utf8_lossy(&stderr).into_owned();
         // This conditions on `status.success()` so we don't print output twice on error.
-        // NOTE: this code is called from a libtest thread, so it's hidden by default unless --nocapture is passed.
+        // NOTE: this code is called from an executor thread, so it's hidden by default unless --no-capture is passed.
         self.dump_output(status.success(), &cmd.get_program().to_string_lossy(), &stdout, &stderr);
         if !status.success() {
             let res = ProcRes { status, stdout, stderr, truncated, cmdline: format!("{:?}", cmd) };