refactor(test): return Assert from ParkedChild::{kill,resume}()

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<S: AsRef<OsStr>>(&self, checkpoint: &str, args: impl AsRef<[S]>) -> ExitStatus {
+    pub fn kill_at<S: AsRef<OsStr>>(&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<Child>,
@@ -1159,26 +1170,52 @@ 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()
             .expect("failed to terminate command at checkpoint");
         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}");
     }
 }
 

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"])

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));
 }