diff --git a/src/test/clitools.rs b/src/test/clitools.rs index b7a81d4a05..9dfc6d7799 100644 --- a/src/test/clitools.rs +++ b/src/test/clitools.rs @@ -7,12 +7,12 @@ use std::{ env::{self, consts::EXE_SUFFIX}, ffi::{OsStr, OsString}, fmt::Debug, - fs, + fs::{self, File}, io::{self, Write}, mem, ops::{Deref, DerefMut}, path::{Path, PathBuf}, - process::{Child, Command, ExitStatus}, + process::{Child, Command, ExitStatus, Stdio}, string::FromUtf8Error, sync::{Arc, LazyLock, RwLock, RwLockWriteGuard}, thread, @@ -1001,7 +1001,7 @@ impl CliTestContext { } /// Run a rustup command until it reaches `checkpoint` and terminate it. - pub fn kill_at>(&self, checkpoint: &str, args: impl AsRef<[S]>) -> ExitStatus { + pub fn kill_at>(&self, checkpoint: &str, args: impl AsRef<[S]>) -> Assert { self.spawn_at(checkpoint, args).kill() } @@ -1034,6 +1034,17 @@ impl CliTestContext { ); cmd.env(CHECKPOINT_ENV, checkpoint); + // Here we are using files instead of pipes, so that a lingering grandchild cannot block us + // from collecting the output once the command is gone. + cmd.stdout(Stdio::from( + File::create(ParkedChild::output_path(&marker, "stdout")) + .expect("failed to create stdout file for checkpoint test"), + )); + cmd.stderr(Stdio::from( + File::create(ParkedChild::output_path(&marker, "stderr")) + .expect("failed to create stderr file for checkpoint test"), + )); + let mut parked = { // The lock covers the spawn itself. It must not be held while the // child stays parked: a writer queued behind it would then block @@ -1150,7 +1161,7 @@ impl Drop for WorkDirGuard<'_> { /// A spawned rustup command parked at a checkpoint. /// /// The spawned command is created by [`CliTestContext::spawn_at`] and killed on drop unless -/// [`ParkedChild::kill`] was called first. +/// [`ParkedChild::kill`] or [`ParkedChild::resume`] was called first. #[must_use] pub struct ParkedChild { child: Option, @@ -1159,7 +1170,7 @@ pub struct ParkedChild { impl ParkedChild { /// Terminate and reap the parked command immediately. - pub fn kill(mut self) -> ExitStatus { + pub fn kill(mut self) -> Assert { let mut child = self.child.take().unwrap(); child .kill() @@ -1167,18 +1178,44 @@ impl ParkedChild { let status = child .wait() .expect("failed to reap command after checkpoint"); - remove_checkpoint_marker(&self.marker); - status + ensure_removed("checkpoint marker", &self.marker); + self.finish(status) } /// Resume the parked command and wait for it to finish. - pub fn resume(mut self) -> ExitStatus { - remove_checkpoint_marker(&self.marker); - self.child + pub fn resume(mut self) -> Assert { + ensure_removed("checkpoint marker", &self.marker); + let status = self + .child .take() .unwrap() .wait() - .expect("failed to reap resumed checkpoint command") + .expect("failed to reap resumed checkpoint command"); + self.finish(status) + } + + /// Reaps the command and collects what it printed so far into an [`Assert`]. + fn finish(self, status: ExitStatus) -> Assert { + let marker = &self.marker; + let output = SanitizedOutput::try_from(Output { + status: status.code(), + stdout: fs::read(Self::output_path(marker, "stdout")) + .expect("failed to read stdout of parked command"), + stderr: fs::read(Self::output_path(marker, "stderr")) + .expect("failed to read stderr of parked command"), + }) + .unwrap(); + + println!("parked command finished"); + println!("status: {:?}", output.status); + println!("stdout:\n====\n{}\n====\n", output.stdout); + println!("stderr:\n====\n{}\n====\n", output.stderr); + + Assert::new(output) + } + + fn output_path(marker: &Path, name: &str) -> PathBuf { + marker.with_extension(name) } } @@ -1189,15 +1226,30 @@ impl Drop for ParkedChild { }; let _ = child.kill(); let _ = child.wait(); - remove_checkpoint_marker(&self.marker); + let marker = &self.marker; + ensure_removed("checkpoint marker", marker); + ensure_removed( + "stdout of parked command", + &Self::output_path(marker, "stdout"), + ); + ensure_removed( + "stderr of parked command", + &Self::output_path(marker, "stderr"), + ); } } -fn remove_checkpoint_marker(marker: &Path) { - if let Err(error) = fs::remove_file(marker) +/// Removes a file if it hasn't been removed yet. +/// +/// # Panics +/// +/// This function panics if any error other than [`io::ErrorKind::NotFound`] occurs while removing +/// the file. +fn ensure_removed(name: &str, path: &Path) { + if let Err(error) = fs::remove_file(path) && error.kind() != io::ErrorKind::NotFound { - panic!("failed to remove checkpoint marker: {error}"); + panic!("failed to remove {name}: {error}"); } } diff --git a/tests/suite/cli_crash.rs b/tests/suite/cli_crash.rs index 70c5c95498..7752afad91 100644 --- a/tests/suite/cli_crash.rs +++ b/tests/suite/cli_crash.rs @@ -6,11 +6,11 @@ use rustup::{ #[tokio::test] async fn interrupted_install_can_be_retried() { let cx = CliTestContext::new(Scenario::SimpleV2).await; - let status = cx.kill_at( + cx.kill_at( CHECKPOINT_UPDATE_BEFORE_METADATA, ["rustup", "toolchain", "install", "nightly"], - ); - assert!(!status.success()); + ) + .is_err(); cx.config .expect(["rustup", "toolchain", "install", "nightly"]) diff --git a/tests/suite/cli_self_upd.rs b/tests/suite/cli_self_upd.rs index 20ab436055..0676602f1c 100644 --- a/tests/suite/cli_self_upd.rs +++ b/tests/suite/cli_self_upd.rs @@ -738,7 +738,7 @@ async fn managed_updater_survives_concurrent_proxy_cleanup() { cx.config.expect(["rustc", "--version"]).await.is_ok(); assert!(updater.exists()); - assert!(parked.resume().success()); + parked.resume().is_ok(); wait_for_completed_update(&cx.config.rustupdir.rustupdir); assert_ne!(before_hash, calc_hash(&rustup)); } @@ -759,12 +759,13 @@ async fn self_update_replacement_survives_proxy_cleanup() { cx.config.expect(["rustc", "--version"]).await.is_ok(); - let status = parked.resume(); + let resumed = parked.resume(); assert!( rustup.exists(), - "concurrent proxy removed the installed rustup during self-update ({status})" + "concurrent proxy removed the installed rustup during self-update (status {:?})", + resumed.output.status, ); - assert!(status.success(), "self-update failed: {status}"); + resumed.is_ok(); assert_ne!(before_hash, calc_hash(&rustup)); }