From d076888f8822d30ffe17f51ba6e81714d5c92796 Mon Sep 17 00:00:00 2001
From: Ana Hobden <operator@hoverbear.org>
Date: Mon, 10 Jul 2023 11:33:53 -0700
Subject: [PATCH] Tweak self-test reporting to make it more clear it may not be
 a total failure (#559)

---
 src/cli/subcommand/install.rs   |  6 +++---
 src/cli/subcommand/self_test.rs |  6 ++++--
 src/error.rs                    | 21 +++++++++++++--------
 src/plan.rs                     | 26 +++++++++++++-------------
 src/self_test.rs                | 15 ++++++++++++---
 5 files changed, 45 insertions(+), 29 deletions(-)

diff --git a/src/cli/subcommand/install.rs b/src/cli/subcommand/install.rs
index 033e28e..18d04ef 100644
--- a/src/cli/subcommand/install.rs
+++ b/src/cli/subcommand/install.rs
@@ -225,10 +225,10 @@ impl CommandExecute for Install {
 
         match install_plan.install(rx1).await {
             Err(err) => {
-                if !no_confirm {
-                    // Attempt to copy self to the store if possible, but since the install failed, this might not work, that's ok.
-                    copy_self_to_nix_dir().await.ok();
+                // Attempt to copy self to the store if possible, but since the install failed, this might not work, that's ok.
+                copy_self_to_nix_dir().await.ok();
 
+                if !no_confirm {
                     let mut was_expected = false;
                     if let Some(expected) = err.expected() {
                         was_expected = true;
diff --git a/src/cli/subcommand/self_test.rs b/src/cli/subcommand/self_test.rs
index e768cc3..ab17d30 100644
--- a/src/cli/subcommand/self_test.rs
+++ b/src/cli/subcommand/self_test.rs
@@ -2,7 +2,7 @@ use std::process::ExitCode;
 
 use clap::Parser;
 
-use crate::cli::CommandExecute;
+use crate::{cli::CommandExecute, NixInstallerError};
 
 /// Run a self test of Nix to ensure that the install worked.
 #[derive(Debug, Parser)]
@@ -12,7 +12,9 @@ pub struct SelfTest {}
 impl CommandExecute for SelfTest {
     #[tracing::instrument(level = "debug", skip_all, fields())]
     async fn execute(self) -> eyre::Result<ExitCode> {
-        crate::self_test::self_test().await?;
+        crate::self_test::self_test()
+            .await
+            .map_err(NixInstallerError::SelfTest)?;
 
         tracing::info!(
             shells = ?crate::self_test::Shell::discover()
diff --git a/src/error.rs b/src/error.rs
index 3c8e641..2d13349 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -15,12 +15,14 @@ pub enum NixInstallerError {
     #[error("Error executing action")]
     Action(#[source] ActionError),
     /// An error originating from a [`self_test`](crate::self_test)
-    #[error("Self test")]
-    SelfTest(
-        #[source]
-        #[from]
-        SelfTestError,
-    ),
+    #[error("Self test error, install may be only partially functional\n{}", .0.iter().map(|err| {
+        if let Some(source) = err.source() {
+            format!("{err}\n{source}\n")
+        } else {
+            format!("{err}\n") 
+        }
+    }).collect::<Vec<_>>().join("\n"))]
+    SelfTest(Vec<SelfTestError>),
     /// An error originating from an [`Action`](crate::action::Action) while reverting
     #[error("Error reverting\n{}", .0.iter().map(|err| {
         if let Some(source) = err.source() {
@@ -100,7 +102,7 @@ impl HasExpectedErrors for NixInstallerError {
         match self {
             NixInstallerError::Action(action_error) => action_error.kind().expected(),
             NixInstallerError::ActionRevert(_) => None,
-            NixInstallerError::SelfTest(_) => None,
+            this @ NixInstallerError::SelfTest(_) => Some(Box::new(this)),
             NixInstallerError::RecordingReceipt(_, _) => None,
             NixInstallerError::CopyingSelf(_) => None,
             NixInstallerError::SerializingReceipt(_) => None,
@@ -124,7 +126,10 @@ impl crate::diagnostics::ErrorDiagnostic for NixInstallerError {
     fn diagnostic(&self) -> String {
         let static_str: &'static str = (self).into();
         let context = match self {
-            Self::SelfTest(self_test) => vec![self_test.diagnostic().to_string()],
+            Self::SelfTest(self_tests) => self_tests
+                .iter()
+                .map(|self_test| self_test.diagnostic().to_string())
+                .collect::<Vec<_>>(),
             Self::Action(action_error) => vec![action_error.diagnostic().to_string()],
             Self::ActionRevert(action_errors) => action_errors
                 .iter()
diff --git a/src/plan.rs b/src/plan.rs
index 43123dd..2140573 100644
--- a/src/plan.rs
+++ b/src/plan.rs
@@ -213,21 +213,21 @@ impl InstallPlan {
                     .await?;
             }
 
-            return Err(err);
-        }
+            Err(err)
+        } else {
+            #[cfg(feature = "diagnostics")]
+            if let Some(diagnostic_data) = &self.diagnostic_data {
+                diagnostic_data
+                    .clone()
+                    .send(
+                        crate::diagnostics::DiagnosticAction::Install,
+                        crate::diagnostics::DiagnosticStatus::Success,
+                    )
+                    .await?;
+            }
 
-        #[cfg(feature = "diagnostics")]
-        if let Some(diagnostic_data) = &self.diagnostic_data {
-            diagnostic_data
-                .clone()
-                .send(
-                    crate::diagnostics::DiagnosticAction::Install,
-                    crate::diagnostics::DiagnosticStatus::Success,
-                )
-                .await?;
+            Ok(())
         }
-
-        Ok(())
     }
 
     #[tracing::instrument(level = "debug", skip_all)]
diff --git a/src/self_test.rs b/src/self_test.rs
index b0d36d0..61552d7 100644
--- a/src/self_test.rs
+++ b/src/self_test.rs
@@ -149,12 +149,21 @@ impl Shell {
 }
 
 #[tracing::instrument(skip_all)]
-pub async fn self_test() -> Result<(), SelfTestError> {
+pub async fn self_test() -> Result<(), Vec<SelfTestError>> {
     let shells = Shell::discover();
 
+    let mut failures = vec![];
+
     for shell in shells {
-        shell.self_test().await?;
+        match shell.self_test().await {
+            Ok(()) => (),
+            Err(err) => failures.push(err),
+        }
     }
 
-    Ok(())
+    if failures.is_empty() {
+        Ok(())
+    } else {
+        Err(failures)
+    }
 }