Rollup merge of #154586 - jdonszelmann:record-rerun, r=clubby789,jieyouxu

Record failed tests with `--record`, and rerun them with `--rerun`

This adds two parameters to `x test`:

## `--record`

Writes a file, by default `build/failed-tests`, but this can be overwritten with
```toml
[build]
record_failed_tests_path = "somepath"
```
with a list of all tests that fail that run.

## `--rerun`

Looks for the failed-tests file, parse it, and attempt to rerun only those tests. No cli-arguments are necessary, i.e.
```
x test tests/ui --record
x test --rerun
```

Will run all failed uitests. No need to pass tests/ui to the rerun invocation.

The last commit is a little awkward, but I think it's the best way to make it so that we *first* run all tests that have to be rerun, and *then* rerun tests passed through the cli.

This makes it so:
```
x test tests/ui --rerun
```
will *first* rerun failed tests, some of which may be uitests, if any fail it quits and reports failed tests, but if all pass it will run all normally passed tests. In other words, only if all previously-failed tests pass on the rerun, we then also run uitests.
Without the last commit, this would instead just run all uitests, since the failed tests form a subset of all uitests. I think that's less useful.

Author: JonathanBrouwer

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

@@ -0,0 +1,110 @@
+use std::collections::BTreeSet;
+use std::fs::{self, File};
+use std::io::{BufRead, BufReader, ErrorKind};
+use std::path::{Path, PathBuf};
+
+use crate::core::builder::{Builder, ShouldRun, Step};
+use crate::t;
+
+#[derive(Clone)]
+pub struct RecordFailedTests {
+    failed_tests_path: Option<PathBuf>,
+}
+
+impl RecordFailedTests {
+    pub fn path(&self) -> Option<&Path> {
+        self.failed_tests_path.as_deref()
+    }
+}
+
+/// This step is run as a dependency of most testing steps.
+/// Upon running, a file is created for failed tests to be recorded in if `--record` is passed on
+/// the command line.
+///
+/// This step is the only way to get access to a token type called [`RecordFailedTests`].
+/// Having this token type signifies the fact that a file was created to store failed tests in,
+/// and is required to create a `Renderer`, the type that renders the outputs of tests.
+///
+/// If `--rerun` isn't passed, or we're in dry-run mode, running this step is a no-op,
+/// and the `RecordFailedTest` type doesn't (need to) signify anything.
+#[derive(Clone, Copy, Eq, PartialEq, Hash, Debug)]
+pub struct SetupFailedTestsFile;
+impl Step for SetupFailedTestsFile {
+    type Output = RecordFailedTests;
+
+    fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
+        run.never()
+    }
+
+    fn run(self, builder: &Builder<'_>) -> Self::Output {
+        if !builder.config.cmd.record() || builder.config.dry_run() {
+            return RecordFailedTests { failed_tests_path: None };
+        }
+
+        let failed_tests_path = builder.config.record_failed_tests_path.clone();
+        println!(
+            "setting up tracking of failed tests in {} (`--record` was passed)",
+            failed_tests_path.display()
+        );
+        if failed_tests_path.exists() {
+            println!("deleting previously recorded failed tests");
+            t!(fs::remove_file(&failed_tests_path));
+        }
+        RecordFailedTests { failed_tests_path: Some(failed_tests_path) }
+    }
+}
+
+pub fn collect_previously_failed_tests(failed_tests_file_path: &PathBuf) -> Vec<PathBuf> {
+    let mut paths = BTreeSet::new();
+
+    println!(
+        "`--rerun` passed so looking for failed tests in {}",
+        failed_tests_file_path.display()
+    );
+
+    let lines: Vec<String> = match File::open(failed_tests_file_path) {
+        Ok(f) => t!(BufReader::new(f).lines().collect()),
+        Err(e) if e.kind() == ErrorKind::NotFound => {
+            println!(
+                "WARNING: failed tests file doesn't exist: `--rerun` only makes sense after a previous test run with `--record`"
+            );
+            return Vec::new();
+        }
+        Err(e) => t!(Err(e)),
+    };
+
+    const MAX_RERUN_PRINTS: usize = 10;
+
+    for line in lines {
+        let trimmed = line.as_str().trim();
+        let without_revision =
+            trimmed.rsplit_once("#").map(|(before, _)| before).unwrap_or(trimmed);
+        let without_suite_prefix = without_revision
+            .strip_prefix("[")
+            .and_then(|rest| rest.split_once("]"))
+            .map(|(_, after)| after.trim())
+            .unwrap_or(without_revision);
+
+        let failed_test_path = PathBuf::from(without_suite_prefix.to_string());
+        if paths.insert(failed_test_path.clone()) {
+            if paths.len() == 1 {
+                println!("rerunning previously failed tests:");
+            }
+            if paths.len() <= MAX_RERUN_PRINTS {
+                println!("    {}", failed_test_path.display());
+            }
+        }
+    }
+
+    if paths.len() > MAX_RERUN_PRINTS {
+        println!("    and {} more...", paths.len() - MAX_RERUN_PRINTS)
+    }
+
+    if paths.is_empty() {
+        println!(
+            "WARNING: failed tests file doesn't contain any failed tests: `--rerun` only makes sense after a previous test run with `--record`"
+        );
+    }
+
+    paths.into_iter().collect()
+}

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

@@ -30,6 +30,7 @@ use tracing::{instrument, span};
 
 use crate::core::build_steps::llvm;
 use crate::core::build_steps::llvm::LLVM_INVALIDATION_PATHS;
+use crate::core::build_steps::test::failed_tests::collect_previously_failed_tests;
 pub use crate::core::config::flags::Subcommand;
 use crate::core::config::flags::{Color, Flags, Warnings};
 use crate::core::config::target_selection::TargetSelectionList;
@@ -125,6 +126,7 @@ pub struct Config {
     pub stage0_metadata: build_helper::stage0_parser::Stage0,
     pub android_ndk: Option<PathBuf>,
     pub optimized_compiler_builtins: CompilerBuiltins,
+    pub record_failed_tests_path: PathBuf,
 
     pub stdout_is_tty: bool,
     pub stderr_is_tty: bool,
@@ -507,6 +509,7 @@ impl Config {
             dist_stage: build_dist_stage,
             bench_stage: build_bench_stage,
             patch_binaries_for_nix: build_patch_binaries_for_nix,
+            record_failed_tests_path: build_record_failed_tests_path,
             // This field is only used by bootstrap.py
             metrics: _,
             android_ndk: build_android_ndk,
@@ -1305,6 +1308,19 @@ impl Config {
         );
         let verbose_tests = rust_verbose_tests.unwrap_or(exec_ctx.is_verbose());
 
+        let record_failed_tests_path =
+            out.join(build_record_failed_tests_path.unwrap_or_else(|| "failed-tests".to_string()));
+
+        let paths = {
+            let mut paths = Vec::new();
+            if flags_cmd.rerun() {
+                paths = collect_previously_failed_tests(&record_failed_tests_path);
+            } else {
+                paths.extend(flags_paths);
+            }
+            paths
+        };
+
         Config {
             // tidy-alphabetical-start
             android_ndk: build_android_ndk,
@@ -1435,13 +1451,14 @@ impl Config {
             out,
             patch_binaries_for_nix: build_patch_binaries_for_nix,
             path_modification_cache,
-            paths: flags_paths,
+            paths,
             prefix: install_prefix.map(PathBuf::from),
             print_step_rusage: build_print_step_rusage.unwrap_or(false),
             print_step_timings: build_print_step_timings.unwrap_or(false),
             profiler: build_profiler.unwrap_or(false),
             python: build_python.map(PathBuf::from),
             quiet: flags_quiet,
+            record_failed_tests_path,
             reproducible_artifacts: flags_reproducible_artifact,
             reuse: build_reuse.map(PathBuf::from),
             rust_analyzer_info,

src/bootstrap/src/core/config/config.rs

@@ -440,6 +440,15 @@ pub enum Subcommand {
         #[arg(long)]
         #[doc(hidden)]
         no_doc: bool,
+
+        /// Record all the failed tests in a file in the build directory.
+        ///
+        /// On subsequent invocations, this set of tests can be rerun by passing `--rerun`
+        #[arg(long)]
+        record: bool,
+        /// Rerun tests that previously failed, and stored with `--record`.
+        #[arg(long)]
+        rerun: bool,
     },
     /// Build and run some test suites *in Miri*
     Miri {
@@ -723,6 +732,20 @@ impl Subcommand {
             _ => false,
         }
     }
+
+    pub fn record(&self) -> bool {
+        match self {
+            Subcommand::Test { record, .. } => *record,
+            _ => false,
+        }
+    }
+
+    pub fn rerun(&self) -> bool {
+        match self {
+            Subcommand::Test { rerun, .. } => *rerun,
+            _ => false,
+        }
+    }
 }
 
 /// Returns the shell completion for a given shell, if the result differs from the current

src/bootstrap/src/core/config/flags.rs

@@ -78,6 +78,7 @@ define_config! {
         tidy_extra_checks: Option<String> = "tidy-extra-checks",
         ccache: Option<StringOrBool> = "ccache",
         exclude: Option<Vec<PathBuf>> = "exclude",
+        record_failed_tests_path: Option<String> = "record_failed_tests_path",
     }
 }
 

src/bootstrap/src/core/config/toml/build.rs

@@ -621,6 +621,11 @@ pub const CONFIG_CHANGE_HISTORY: &[ChangeInfo] = &[
         severity: ChangeSeverity::Info,
         summary: "`x.py` stopped accepting partial argument names. Use full names to avoid errors.",
     },
+    ChangeInfo {
+        change_id: 154586,
+        severity: ChangeSeverity::Info,
+        summary: "New option `build.record_failed_tests_path` to store failed tests when passing `--record`. These can be rerun with `--rerun`.",
+    },
     ChangeInfo {
         change_id: 154587,
         severity: ChangeSeverity::Info,

src/bootstrap/src/utils/change_tracker.rs

@@ -6,12 +6,14 @@
 //! and rustc) libtest doesn't include the rendered human-readable output as a JSON field. We had
 //! to reimplement all the rendering logic in this module because of that.
 
+use std::fs::File;
 use std::io::{BufRead, BufReader, Read, Write};
 use std::process::ChildStdout;
 use std::time::Duration;
 
 use termcolor::{Color, ColorSpec, WriteColor};
 
+use crate::core::build_steps::test::failed_tests::RecordFailedTests;
 use crate::core::builder::Builder;
 use crate::utils::exec::BootstrapCommand;
 
@@ -20,21 +22,23 @@ const TERSE_TESTS_PER_LINE: usize = 88;
 pub(crate) fn add_flags_and_try_run_tests(
     builder: &Builder<'_>,
     cmd: &mut BootstrapCommand,
+    record_failed_tests: RecordFailedTests,
 ) -> bool {
     if !cmd.get_args().any(|arg| arg == "--") {
         cmd.arg("--");
     }
     cmd.args(["-Z", "unstable-options", "--format", "json"]);
 
-    try_run_tests(builder, cmd, false)
+    try_run_tests(builder, cmd, false, record_failed_tests)
 }
 
 pub(crate) fn try_run_tests(
     builder: &Builder<'_>,
     cmd: &mut BootstrapCommand,
     stream: bool,
+    record_failed_tests: RecordFailedTests,
 ) -> bool {
-    if run_tests(builder, cmd, stream) {
+    if run_tests(builder, cmd, stream, record_failed_tests) {
         return true;
     }
 
@@ -47,7 +51,12 @@ pub(crate) fn try_run_tests(
     false
 }
 
-fn run_tests(builder: &Builder<'_>, cmd: &mut BootstrapCommand, stream: bool) -> bool {
+fn run_tests(
+    builder: &Builder<'_>,
+    cmd: &mut BootstrapCommand,
+    stream: bool,
+    record_failed_tests: RecordFailedTests,
+) -> bool {
     builder.do_if_verbose(|| println!("running: {cmd:?}"));
 
     let Some(mut streaming_command) = cmd.stream_capture_stdout(&builder.config.exec_ctx) else {
@@ -56,7 +65,8 @@ fn run_tests(builder: &Builder<'_>, cmd: &mut BootstrapCommand, stream: bool) ->
 
     // This runs until the stdout of the child is closed, which means the child exited. We don't
     // run this on another thread since the builder is not Sync.
-    let renderer = Renderer::new(streaming_command.stdout.take().unwrap(), builder);
+    let renderer =
+        Renderer::new(streaming_command.stdout.take().unwrap(), builder, record_failed_tests);
     if stream {
         renderer.stream_all();
     } else {
@@ -87,10 +97,30 @@ struct Renderer<'a> {
     ignored_tests: usize,
     terse_tests_in_line: usize,
     ci_latest_logged_percentage: f64,
+
+    failed_tests: Option<File>,
 }
 
 impl<'a> Renderer<'a> {
-    fn new(stdout: ChildStdout, builder: &'a Builder<'a>) -> Self {
+    fn new(
+        stdout: ChildStdout,
+        builder: &'a Builder<'a>,
+        record_failed_tests: RecordFailedTests,
+    ) -> Self {
+        let failed_tests = record_failed_tests.path().and_then(|path| {
+            // create the file (overwriting any previous) to get ready to record new failed tests
+            match File::options().create(true).append(true).truncate(false).open(path) {
+                Ok(f) => Some(f),
+                Err(e) => {
+                    println!(
+                        "Couldn't open file {} to write test failures to: {e}. (attempted because `--record` was passed). Test failures will not be recorded.",
+                        path.display()
+                    );
+                    None
+                }
+            }
+        });
+
         Self {
             stdout: BufReader::new(stdout),
             benches: Vec::new(),
@@ -102,6 +132,7 @@ impl<'a> Renderer<'a> {
             ignored_tests: 0,
             terse_tests_in_line: 0,
             ci_latest_logged_percentage: 0.0,
+            failed_tests,
         }
     }
 
@@ -268,6 +299,13 @@ impl<'a> Renderer<'a> {
             for failure in &self.failures {
                 println!("    {}", failure.name);
             }
+
+            if self.failed_tests.is_some() {
+                println!(
+                    "This list of test failures was recorded.\nUse `x test --rerun` to retry just these {} failed tests.",
+                    self.failures.len(),
+                )
+            }
         }
 
         if !self.benches.is_empty() {
@@ -360,6 +398,13 @@ impl<'a> Renderer<'a> {
             }
             Message::Test(TestMessage::Failed(outcome)) => {
                 self.render_test_outcome(Outcome::Failed, &outcome);
+                if let Some(failed_tests) = &mut self.failed_tests
+                    && let Err(e) = writeln!(failed_tests, "{}", outcome.name)
+                {
+                    eprintln!(
+                        "failed to write test failure to file: {e} (attempted because `--record` was passed)"
+                    );
+                }
                 self.failures.push(outcome);
             }
             Message::Test(TestMessage::Timeout { name }) => {

src/bootstrap/src/utils/render_tests.rs

@@ -465,6 +465,8 @@ complete -c x -n "__fish_x_using_subcommand test" -l rustfix-coverage -d 'enable
 complete -c x -n "__fish_x_using_subcommand test" -l no-capture -d 'don\'t capture stdout/stderr of tests'
 complete -c x -n "__fish_x_using_subcommand test" -l bypass-ignore-backends -d 'Ignore `//@ ignore-backends` directives'
 complete -c x -n "__fish_x_using_subcommand test" -l no-doc -d 'Deprecated. Use `--all-targets` or `--tests` instead'
+complete -c x -n "__fish_x_using_subcommand test" -l record -d 'Record all the failed tests in a file in the build directory'
+complete -c x -n "__fish_x_using_subcommand test" -l rerun -d 'Rerun tests that previously failed, and stored with `--record`'
 complete -c x -n "__fish_x_using_subcommand test" -s v -l verbose -d 'use verbose output (-vv for very verbose)'
 complete -c x -n "__fish_x_using_subcommand test" -s q -l quiet -d 'use quiet output'
 complete -c x -n "__fish_x_using_subcommand test" -s i -l incremental -d 'use incremental compilation'
@@ -520,6 +522,8 @@ complete -c x -n "__fish_x_using_subcommand t" -l rustfix-coverage -d 'enable th
 complete -c x -n "__fish_x_using_subcommand t" -l no-capture -d 'don\'t capture stdout/stderr of tests'
 complete -c x -n "__fish_x_using_subcommand t" -l bypass-ignore-backends -d 'Ignore `//@ ignore-backends` directives'
 complete -c x -n "__fish_x_using_subcommand t" -l no-doc -d 'Deprecated. Use `--all-targets` or `--tests` instead'
+complete -c x -n "__fish_x_using_subcommand t" -l record -d 'Record all the failed tests in a file in the build directory'
+complete -c x -n "__fish_x_using_subcommand t" -l rerun -d 'Rerun tests that previously failed, and stored with `--record`'
 complete -c x -n "__fish_x_using_subcommand t" -s v -l verbose -d 'use verbose output (-vv for very verbose)'
 complete -c x -n "__fish_x_using_subcommand t" -s q -l quiet -d 'use quiet output'
 complete -c x -n "__fish_x_using_subcommand t" -s i -l incremental -d 'use incremental compilation'

src/etc/completions/x.fish

@@ -543,6 +543,8 @@ Register-ArgumentCompleter -Native -CommandName 'x' -ScriptBlock {
             [CompletionResult]::new('--no-capture', '--no-capture', [CompletionResultType]::ParameterName, 'don''t capture stdout/stderr of tests')
             [CompletionResult]::new('--bypass-ignore-backends', '--bypass-ignore-backends', [CompletionResultType]::ParameterName, 'Ignore `//@ ignore-backends` directives')
             [CompletionResult]::new('--no-doc', '--no-doc', [CompletionResultType]::ParameterName, 'Deprecated. Use `--all-targets` or `--tests` instead')
+            [CompletionResult]::new('--record', '--record', [CompletionResultType]::ParameterName, 'Record all the failed tests in a file in the build directory')
+            [CompletionResult]::new('--rerun', '--rerun', [CompletionResultType]::ParameterName, 'Rerun tests that previously failed, and stored with `--record`')
             [CompletionResult]::new('-v', '-v', [CompletionResultType]::ParameterName, 'use verbose output (-vv for very verbose)')
             [CompletionResult]::new('--verbose', '--verbose', [CompletionResultType]::ParameterName, 'use verbose output (-vv for very verbose)')
             [CompletionResult]::new('-q', '-q', [CompletionResultType]::ParameterName, 'use quiet output')
@@ -606,6 +608,8 @@ Register-ArgumentCompleter -Native -CommandName 'x' -ScriptBlock {
             [CompletionResult]::new('--no-capture', '--no-capture', [CompletionResultType]::ParameterName, 'don''t capture stdout/stderr of tests')
             [CompletionResult]::new('--bypass-ignore-backends', '--bypass-ignore-backends', [CompletionResultType]::ParameterName, 'Ignore `//@ ignore-backends` directives')
             [CompletionResult]::new('--no-doc', '--no-doc', [CompletionResultType]::ParameterName, 'Deprecated. Use `--all-targets` or `--tests` instead')
+            [CompletionResult]::new('--record', '--record', [CompletionResultType]::ParameterName, 'Record all the failed tests in a file in the build directory')
+            [CompletionResult]::new('--rerun', '--rerun', [CompletionResultType]::ParameterName, 'Rerun tests that previously failed, and stored with `--record`')
             [CompletionResult]::new('-v', '-v', [CompletionResultType]::ParameterName, 'use verbose output (-vv for very verbose)')
             [CompletionResult]::new('--verbose', '--verbose', [CompletionResultType]::ParameterName, 'use verbose output (-vv for very verbose)')
             [CompletionResult]::new('-q', '-q', [CompletionResultType]::ParameterName, 'use quiet output')