From 31ce8e5b41a773707d4fbf3e98071bf8960cdc90 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Mon, 3 Aug 2026 23:26:58 +1000 Subject: [PATCH] Register `coverage-map` and `coverage-run` aliases via a separate step Using a separate step lets us remove the `default_to_suites_only` hack. --- src/bootstrap/src/core/build_steps/test.rs | 112 ++++++++++-------- .../snapshots/x_test_coverage_map.snap | 2 +- .../snapshots/x_test_coverage_run.snap | 2 +- src/bootstrap/src/core/builder/mod.rs | 23 +--- 4 files changed, 67 insertions(+), 72 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index 3015d5a83db8d..ecb41667ce811 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -44,7 +44,7 @@ use crate::utils::helpers::{ up_to_date, }; use crate::utils::render_tests::{add_flags_and_try_run_tests, try_run_tests}; -use crate::{CLang, CodegenBackendKind, GitRepo, Mode, PathSet, TestTarget, envify, exit}; +use crate::{CLang, CodegenBackendKind, GitRepo, Mode, TestTarget, envify, exit}; mod compiletest; pub mod failed_tests; @@ -1998,6 +1998,12 @@ impl Coverage { const SUITE: &'static str = "coverage"; const ALL_MODES: &[CompiletestMode] = &[CompiletestMode::CoverageMap, CompiletestMode::CoverageRun]; + + fn new(run: &RunConfig<'_>, mode: CompiletestMode) -> Self { + let compiler = run.builder.compiler(run.builder.top_stage, run.build_triple()); + let target = run.target; + Coverage { compiler, target, mode } + } } impl CommandLineStep for Coverage { @@ -2005,23 +2011,14 @@ impl CommandLineStep for Coverage { /// Compiletest will automatically skip the "coverage-run" tests if necessary. const IS_HOST: bool = false; - fn should_run(mut run: ShouldRun<'_>) -> ShouldRun<'_> { - // Support various invocation styles, including: + fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { + // Handle these invocation styles: + // - `./x test` (including coverage tests) // - `./x test coverage` + // - `./x test tests/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.as_str()); - } - - // Allow `./x test --skip=tests` to properly skip the coverage tests, - // by not treating the `coverage-map` and `coverage-run` aliases as - // implied command-line arguments. - run = run.default_to_suites_only(); - - run + // - `./x test tests/coverage/trivial.rs --skip=coverage-run` + run.suite_path(Coverage::PATH) } fn is_default_step(_builder: &Builder<'_>) -> bool { @@ -2029,41 +2026,13 @@ impl CommandLineStep for Coverage { } 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.as_str()) { - modes.push(mode); - break; - } - } - } - PathSet::Suite(_) => { - modes.extend_from_slice(Self::ALL_MODES); - break; - } - } - } - - // Skip any modes that were explicitly skipped/excluded on the command-line. + // Run the tests in all coverage-test modes, but skip any modes that + // were explicitly skipped on the command-line (e.g. `--skip=coverage-run`). // FIXME(Zalathar): Integrate this into central skip handling somehow? - modes.retain(|mode| { - !run.builder.config.skip.iter().any(|skip| skip == Path::new(mode.as_str())) - }); - - for mode in modes { - run.builder.ensure(Coverage { compiler, target, mode }); + for &mode in Coverage::ALL_MODES { + if !run.builder.config.skip.iter().any(|skip| skip == Path::new(mode.as_str())) { + run.builder.ensure(Coverage::new(&run, mode)); + } } } @@ -2082,6 +2051,49 @@ impl CommandLineStep for Coverage { } } +/// Registers the `coverage-map` and `coverage-run` aliases, which are then +/// forwarded to the [`Coverage`] step. +/// +/// If the aliases were registered by [`Coverage`] directly, they would also +/// be treated as implied command-line arguments when run by default. +/// That would cause things like `./x test --skip=tests` to still run coverage +/// tests, which is undesirable. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub enum CoverageModeAlias {} + +impl CommandLineStep for CoverageModeAlias { + type Output = (); + + fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { + // Register the aliases "coverage-map" and "coverage-run", to handle + // these invocation styles: + // - `./x test coverage-map` + // - `./x test coverage-run -- tests/coverage/trivial.rs` + Coverage::ALL_MODES.iter().fold(run, |run, mode| run.alias(mode.as_str())) + } + + fn is_default_step(_builder: &Builder<'_>) -> bool { + false + } + + fn make_run(run: RunConfig<'_>) { + for path in &run.paths { + let single_path = &path.assert_single_path().path; + for &mode in Coverage::ALL_MODES { + if single_path == Path::new(mode.as_str()) { + // Instead of creating an intermediate `CoverageModeAlias` + // step instance, delegate straight to `Coverage`. + run.builder.ensure(Coverage::new(&run, mode)); + } + } + } + } + + fn run(self, _builder: &Builder<'_>) { + unreachable!("never instantiated; `make_run` creates a Coverage step instead"); + } +} + test!(CoverageRunRustdoc { path: "tests/coverage-run-rustdoc", mode: CompiletestMode::CoverageRun, diff --git a/src/bootstrap/src/core/builder/cli_paths/snapshots/x_test_coverage_map.snap b/src/bootstrap/src/core/builder/cli_paths/snapshots/x_test_coverage_map.snap index 9f0ef84851d5a..7ae3a95a05bd4 100644 --- a/src/bootstrap/src/core/builder/cli_paths/snapshots/x_test_coverage_map.snap +++ b/src/bootstrap/src/core/builder/cli_paths/snapshots/x_test_coverage_map.snap @@ -2,6 +2,6 @@ source: src/bootstrap/src/core/builder/cli_paths/tests.rs expression: test coverage-map --- -[Test] test::Coverage +[Test] test::CoverageModeAlias targets: [aarch64-unknown-linux-gnu] - Set({coverage-map}) diff --git a/src/bootstrap/src/core/builder/cli_paths/snapshots/x_test_coverage_run.snap b/src/bootstrap/src/core/builder/cli_paths/snapshots/x_test_coverage_run.snap index 41700ce9e210c..8657e72033248 100644 --- a/src/bootstrap/src/core/builder/cli_paths/snapshots/x_test_coverage_run.snap +++ b/src/bootstrap/src/core/builder/cli_paths/snapshots/x_test_coverage_run.snap @@ -2,6 +2,6 @@ source: src/bootstrap/src/core/builder/cli_paths/tests.rs expression: test coverage-run --- -[Test] test::Coverage +[Test] test::CoverageModeAlias targets: [aarch64-unknown-linux-gnu] - Set({coverage-run}) diff --git a/src/bootstrap/src/core/builder/mod.rs b/src/bootstrap/src/core/builder/mod.rs index 051e01a0a6666..603ef65854cf6 100644 --- a/src/bootstrap/src/core/builder/mod.rs +++ b/src/bootstrap/src/core/builder/mod.rs @@ -524,13 +524,11 @@ pub struct ShouldRun<'a> { // use a BTreeSet to maintain sort order paths: BTreeSet, - - default_to_suites_only: bool, } impl<'a> ShouldRun<'a> { fn new(builder: &'a Builder<'_>, kind: Kind) -> ShouldRun<'a> { - ShouldRun { builder, kind, paths: BTreeSet::new(), default_to_suites_only: false } + ShouldRun { builder, kind, paths: BTreeSet::new() } } /// The corresponding step should run if the bootstrap command-line selects @@ -643,26 +641,10 @@ impl<'a> ShouldRun<'a> { sets } - /// When generating pathsets for a step that is being run "by default" - /// (i.e. when running bootstrap without an explicit command-line path), - /// discard any paths that were not registered as test suites. - /// - /// This is basically a hack to make path-based skipping work properly for - /// coverage tests, since otherwise the `coverage-map` and `coverage-run` - /// aliases would prevent `./x test --skip=tests` from skipping them. - pub(crate) fn default_to_suites_only(mut self) -> Self { - self.default_to_suites_only = true; - self - } - /// When the corresponding step is run "by default" (without explicit command-line paths), /// act as though the user had explicitly specified these paths. fn default_pathsets(&self) -> Vec { - let mut default_pathsets = self.paths.iter().cloned().collect::>(); - if self.default_to_suites_only { - default_pathsets.retain(|p| matches!(p, PathSet::Suite(_))); - } - default_pathsets + self.paths.iter().cloned().collect::>() } } @@ -898,6 +880,7 @@ impl<'a> Builder<'a> { test::Ui, test::Crashes, test::Coverage, + test::CoverageModeAlias, test::MirOpt, test::CodegenLlvm, test::CodegenUnits,