From 089bd510cf5ac835f1c3755f44fa2d15d211f09a Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sat, 4 Jan 2025 17:21:53 +1100 Subject: [PATCH] Consolidate coverage test suite steps into a single step --- src/bootstrap/src/core/build_steps/test.rs | 186 ++++++++------------- src/bootstrap/src/core/builder/mod.rs | 2 - src/bootstrap/src/core/builder/tests.rs | 31 ++++ 3 files changed, 99 insertions(+), 120 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index 84d09bbc2e092..bf84290575ac4 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/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(crate) 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, + }); } } diff --git a/src/bootstrap/src/core/builder/mod.rs b/src/bootstrap/src/core/builder/mod.rs index 04d51fab5d521..adf6d9ae72ab3 100644 --- a/src/bootstrap/src/core/builder/mod.rs +++ b/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, diff --git a/src/bootstrap/src/core/builder/tests.rs b/src/bootstrap/src/core/builder/tests.rs index 5769198afac64..7f2d30149eb48 100644 --- a/src/bootstrap/src/core/builder/tests.rs +++ b/src/bootstrap/src/core/builder/tests.rs @@ -828,3 +828,34 @@ 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 { + println!("- {cmd:?}"); + let cmd = cmd.iter().copied().map(str::to_owned).collect::>(); + 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::().iter().map(|(step, ())| step.mode).collect::>(); + assert_eq!(modes, expected); + } +}