Consolidate coverage test suite steps into a single step

Author: Zalathar

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

@@ -943,8 +943,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);
+    }
+}