Rollup merge of #135097 - Zalathar:coverage-test-step, r=Kobzol

bootstrap: Consolidate coverage test suite steps into a single step

Now that I have more understanding of bootstrap steps, and a renewed distaste for unnecessary macros, I have managed to express the subtleties of the `tests/coverage` test suite in a single step defined in ordinary code, with no need for helper macros.

Deciding which modes to run is still a bit clunky due to limitations in existing ShouldRun/PathSet APIs, but I think it's a net improvement over having to declare several different steps to handle the suite path and aliases.

The interaction with `--skip` isn't as nice as I'd like, but all of the known limitations are limitations that already existed in the previous implementation.

One minor change is that by default compiletest is now invoked in `coverage-run` mode even when cross-compiling. However, in that situation compiletest still knows that it should skip all of the individual coverage-run tests.

r? jieyouxu (or reassign)

Author: matthiaskrgr

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

@@ -27,7 +27,7 @@ use crate::utils::helpers::{
     linker_args, linker_flags, t, target_supports_cranelift_backend, up_to_date,
 };
 use crate::utils::render_tests::{add_flags_and_try_run_tests, try_run_tests};
-use crate::{CLang, DocTests, GitRepo, Mode, envify};
+use crate::{CLang, DocTests, GitRepo, Mode, PathSet, envify};
 
 const ADB_TEST_DIR: &str = "/data/local/tmp/work";
 
@@ -1185,53 +1185,6 @@ macro_rules! test {
     };
 }
 
-/// Declares an alias for running the [`Coverage`] tests in only one mode.
-/// Adapted from [`test`].
-macro_rules! coverage_test_alias {
-    (
-        $( #[$attr:meta] )* // allow docstrings and attributes
-        $name:ident {
-            alias_and_mode: $alias_and_mode:expr, // &'static str
-            default: $default:expr, // bool
-            only_hosts: $only_hosts:expr // bool
-            $( , )? // optional trailing comma
-        }
-    ) => {
-        $( #[$attr] )*
-        #[derive(Debug, Clone, PartialEq, Eq, Hash)]
-        pub struct $name {
-            pub compiler: Compiler,
-            pub target: TargetSelection,
-        }
-
-        impl $name {
-            const MODE: &'static str = $alias_and_mode;
-        }
-
-        impl Step for $name {
-            type Output = ();
-            const DEFAULT: bool = $default;
-            const ONLY_HOSTS: bool = $only_hosts;
-
-            fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
-                // Register the mode name as a command-line alias.
-                // This allows `x test coverage-map` and `x test coverage-run`.
-                run.alias($alias_and_mode)
-            }
-
-            fn make_run(run: RunConfig<'_>) {
-                let compiler = run.builder.compiler(run.builder.top_stage, run.build_triple());
-
-                run.builder.ensure($name { compiler, target: run.target });
-            }
-
-            fn run(self, builder: &Builder<'_>) {
-                Coverage::run_coverage_tests(builder, self.compiler, self.target, Self::MODE);
-            }
-        }
-    };
-}
-
 #[derive(Debug, Copy, Clone, Hash, PartialEq, Eq, Ord, PartialOrd)]
 pub struct RunMakeSupport {
     pub compiler: Compiler,
@@ -1473,99 +1426,96 @@ impl Step for RunMake {
 
 test!(Assembly { path: "tests/assembly", mode: "assembly", suite: "assembly", default: true });
 
-/// Coverage tests are a bit more complicated than other test suites, because
-/// we want to run the same set of test files in multiple different modes,
-/// in a way that's convenient and flexible when invoked manually.
-///
-/// This combined step runs the specified tests (or all of `tests/coverage`)
-/// in both "coverage-map" and "coverage-run" modes.
-///
-/// Used by:
-/// - `x test coverage`
-/// - `x test tests/coverage`
-/// - `x test tests/coverage/trivial.rs` (etc)
-///
-/// (Each individual mode also has its own step that will run the tests in
-/// just that mode.)
-#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+/// Runs the coverage test suite at `tests/coverage` in some or all of the
+/// coverage test modes.
+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct Coverage {
     pub compiler: Compiler,
     pub target: TargetSelection,
+    pub mode: &'static str,
 }
 
 impl Coverage {
     const PATH: &'static str = "tests/coverage";
     const SUITE: &'static str = "coverage";
-
-    /// Runs the coverage test suite (or a user-specified subset) in one mode.
-    ///
-    /// This same function is used by the multi-mode step ([`Coverage`]) and by
-    /// the single-mode steps ([`CoverageMap`] and [`CoverageRun`]), to help
-    /// ensure that they all behave consistently with each other, regardless of
-    /// how the coverage tests have been invoked.
-    fn run_coverage_tests(
-        builder: &Builder<'_>,
-        compiler: Compiler,
-        target: TargetSelection,
-        mode: &'static str,
-    ) {
-        // Like many other test steps, we delegate to a `Compiletest` step to
-        // actually run the tests. (See `test_definitions!`.)
-        builder.ensure(Compiletest {
-            compiler,
-            target,
-            mode,
-            suite: Self::SUITE,
-            path: Self::PATH,
-            compare_mode: None,
-        });
-    }
+    const ALL_MODES: &[&str] = &["coverage-map", "coverage-run"];
 }
 
 impl Step for Coverage {
     type Output = ();
-    /// We rely on the individual CoverageMap/CoverageRun steps to run themselves.
-    const DEFAULT: bool = false;
-    /// When manually invoked, try to run as much as possible.
+    const DEFAULT: bool = true;
     /// Compiletest will automatically skip the "coverage-run" tests if necessary.
     const ONLY_HOSTS: bool = false;
 
-    fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
-        // Take responsibility for command-line paths within `tests/coverage`.
-        run.suite_path(Self::PATH)
+    fn should_run(mut run: ShouldRun<'_>) -> ShouldRun<'_> {
+        // Support various invocation styles, including:
+        // - `./x test coverage`
+        // - `./x test tests/coverage/trivial.rs`
+        // - `./x test coverage-map`
+        // - `./x test coverage-run -- tests/coverage/trivial.rs`
+        run = run.suite_path(Self::PATH);
+        for mode in Self::ALL_MODES {
+            run = run.alias(mode);
+        }
+        run
     }
 
     fn make_run(run: RunConfig<'_>) {
         let compiler = run.builder.compiler(run.builder.top_stage, run.build_triple());
+        let target = run.target;
+
+        // List of (coverage) test modes that the coverage test suite will be
+        // run in. It's OK for this to contain duplicates, because the call to
+        // `Builder::ensure` below will take care of deduplication.
+        let mut modes = vec![];
+
+        // From the pathsets that were selected on the command-line (or by default),
+        // determine which modes to run in.
+        for path in &run.paths {
+            match path {
+                PathSet::Set(_) => {
+                    for mode in Self::ALL_MODES {
+                        if path.assert_single_path().path == Path::new(mode) {
+                            modes.push(mode);
+                            break;
+                        }
+                    }
+                }
+                PathSet::Suite(_) => {
+                    modes.extend(Self::ALL_MODES);
+                    break;
+                }
+            }
+        }
+
+        // Skip any modes that were explicitly skipped/excluded on the command-line.
+        // FIXME(Zalathar): Integrate this into central skip handling somehow?
+        modes.retain(|mode| !run.builder.config.skip.iter().any(|skip| skip == Path::new(mode)));
+
+        // FIXME(Zalathar): Make these commands skip all coverage tests, as expected:
+        // - `./x test --skip=tests`
+        // - `./x test --skip=tests/coverage`
+        // - `./x test --skip=coverage`
+        // Skip handling currently doesn't have a way to know that skipping the coverage
+        // suite should also skip the `coverage-map` and `coverage-run` aliases.
 
-        run.builder.ensure(Coverage { compiler, target: run.target });
+        for mode in modes {
+            run.builder.ensure(Coverage { compiler, target, mode });
+        }
     }
 
     fn run(self, builder: &Builder<'_>) {
-        // Run the specified coverage tests (possibly all of them) in both modes.
-        Self::run_coverage_tests(builder, self.compiler, self.target, CoverageMap::MODE);
-        Self::run_coverage_tests(builder, self.compiler, self.target, CoverageRun::MODE);
-    }
-}
-
-coverage_test_alias! {
-    /// Runs the `tests/coverage` test suite in "coverage-map" mode only.
-    /// Used by `x test` and `x test coverage-map`.
-    CoverageMap {
-        alias_and_mode: "coverage-map",
-        default: true,
-        only_hosts: false,
-    }
-}
-coverage_test_alias! {
-    /// Runs the `tests/coverage` test suite in "coverage-run" mode only.
-    /// Used by `x test` and `x test coverage-run`.
-    CoverageRun {
-        alias_and_mode: "coverage-run",
-        default: true,
-        // Compiletest knows how to automatically skip these tests when cross-compiling,
-        // but skipping the whole step here makes it clearer that they haven't run at all.
-        only_hosts: true,
+        let Self { compiler, target, mode } = self;
+        // Like other compiletest suite test steps, delegate to an internal
+        // compiletest task to actually run the tests.
+        builder.ensure(Compiletest {
+            compiler,
+            target,
+            mode,
+            suite: Self::SUITE,
+            path: Self::PATH,
+            compare_mode: None,
+        });
     }
 }
 

src/bootstrap/src/core/builder/mod.rs

@@ -944,8 +944,6 @@ impl<'a> Builder<'a> {
                 test::Ui,
                 test::Crashes,
                 test::Coverage,
-                test::CoverageMap,
-                test::CoverageRun,
                 test::MirOpt,
                 test::Codegen,
                 test::CodegenUnits,

src/bootstrap/src/core/builder/tests.rs

@@ -828,3 +828,36 @@ fn test_test_compiler() {
 
     assert_eq!((compiler, cranelift, gcc), (true, false, false));
 }
+
+#[test]
+fn test_test_coverage() {
+    struct Case {
+        cmd: &'static [&'static str],
+        expected: &'static [&'static str],
+    }
+    let cases = &[
+        Case { cmd: &["test"], expected: &["coverage-map", "coverage-run"] },
+        Case { cmd: &["test", "coverage"], expected: &["coverage-map", "coverage-run"] },
+        Case { cmd: &["test", "coverage-map"], expected: &["coverage-map"] },
+        Case { cmd: &["test", "coverage-run"], expected: &["coverage-run"] },
+        Case { cmd: &["test", "coverage", "--skip=coverage"], expected: &[] },
+        Case { cmd: &["test", "coverage", "--skip=tests/coverage"], expected: &[] },
+        Case { cmd: &["test", "coverage", "--skip=coverage-map"], expected: &["coverage-run"] },
+        Case { cmd: &["test", "coverage", "--skip=coverage-run"], expected: &["coverage-map"] },
+        Case { cmd: &["test", "--skip=coverage-map", "--skip=coverage-run"], expected: &[] },
+        Case { cmd: &["test", "coverage", "--skip=tests"], expected: &[] },
+    ];
+
+    for &Case { cmd, expected } in cases {
+        // Print each test case so that if one fails, the most recently printed
+        // case is the one that failed.
+        println!("Testing case: {cmd:?}");
+        let cmd = cmd.iter().copied().map(str::to_owned).collect::<Vec<_>>();
+        let config = configure_with_args(&cmd, &[TEST_TRIPLE_1], &[TEST_TRIPLE_1]);
+        let mut cache = run_build(&config.paths.clone(), config);
+
+        let modes =
+            cache.all::<test::Coverage>().iter().map(|(step, ())| step.mode).collect::<Vec<_>>();
+        assert_eq!(modes, expected);
+    }
+}