Refactor distant binary to yield software exit code when oneoff operation fails

Cargo.lock

@@ -244,6 +244,7 @@ dependencies = [
  "fork",
  "lazy_static",
  "log",
+ "predicates",
  "rand",
  "rstest",
  "serde_json",
@@ -343,6 +344,15 @@ dependencies = [
  "yansi",
 ]
 
+[[package]]
+name = "float-cmp"
+version = "0.9.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "98de4bbd547a563b716d8dfa9aad1cb19bfab00f4fa09a6a4ed21dbcf44ce9c4"
+dependencies = [
+ "num-traits",
+]
+
 [[package]]
 name = "fnv"
 version = "1.0.7"
@@ -675,6 +685,12 @@ dependencies = [
  "winapi",
 ]
 
+[[package]]
+name = "normalize-line-endings"
+version = "0.3.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "61807f77802ff30975e01f4f071c8ba10c022052f98b3294119f3e615d13e5be"
+
 [[package]]
 name = "ntapi"
 version = "0.3.6"
@@ -797,8 +813,11 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "c143348f141cc87aab5b950021bac6145d0e5ae754b0591de23244cee42c9308"
 dependencies = [
  "difflib",
+ "float-cmp",
  "itertools",
+ "normalize-line-endings",
  "predicates-core",
+ "regex",
 ]
 
 [[package]]

Cargo.toml

@@ -36,4 +36,5 @@ whoami = "1.1.2"
 [dev-dependencies]
 assert_cmd = "2.0.0"
 assert_fs = "1.0.3"
+predicates = "2.0.2"
 rstest = "0.11.0"

src/exit.rs

@@ -19,7 +19,8 @@ pub enum ExitCode {
     /// EX_UNAVAILABLE (69) - being used when IO error encountered where connection is problem
     Unavailable,
 
-    /// EX_SOFTWARE (70) - being used for internal errors that can occur like joining a task
+    /// EX_SOFTWARE (70) - being used for when an action fails as well as for internal errors that
+    /// can occur like joining a task
     Software,
 
     /// EX_OSERR (71) - being used when fork failed
@@ -38,24 +39,44 @@ pub enum ExitCode {
     Custom(i32),
 }
 
+impl ExitCode {
+    /// Convert into numeric exit code
+    pub fn to_i32(&self) -> i32 {
+        match *self {
+            Self::Usage => 64,
+            Self::DataErr => 65,
+            Self::NoInput => 66,
+            Self::NoHost => 68,
+            Self::Unavailable => 69,
+            Self::Software => 70,
+            Self::OsErr => 71,
+            Self::IoError => 74,
+            Self::TempFail => 75,
+            Self::Protocol => 76,
+            Self::Custom(x) => x,
+        }
+    }
+}
+
+impl From<ExitCode> for i32 {
+    fn from(code: ExitCode) -> Self {
+        code.to_i32()
+    }
+}
+
 /// Represents an error that can be converted into an exit code
 pub trait ExitCodeError: std::error::Error {
     fn to_exit_code(&self) -> ExitCode;
 
+    /// Indicates if the error message associated with this exit code error
+    /// should be printed, or if this is just used to reflect the exit code
+    /// when the process exits
+    fn is_silent(&self) -> bool {
+        false
+    }
+
     fn to_i32(&self) -> i32 {
-        match self.to_exit_code() {
-            ExitCode::Usage => 64,
-            ExitCode::DataErr => 65,
-            ExitCode::NoInput => 66,
-            ExitCode::NoHost => 68,
-            ExitCode::Unavailable => 69,
-            ExitCode::Software => 70,
-            ExitCode::OsErr => 71,
-            ExitCode::IoError => 74,
-            ExitCode::TempFail => 75,
-            ExitCode::Protocol => 76,
-            ExitCode::Custom(x) => x,
-        }
+        self.to_exit_code().to_i32()
     }
 }
 

src/lib.rs

@@ -11,12 +11,16 @@ mod utils;
 
 use log::error;
 
+pub use exit::{ExitCode, ExitCodeError};
+
 /// Main entrypoint into the program
 pub fn run() {
     let opt = opt::Opt::load();
     let logger = init_logging(&opt.common);
     if let Err(x) = opt.subcommand.run(opt.common) {
-        error!("Exiting due to error: {}", x);
+        if !x.is_silent() {
+            error!("Exiting due to error: {}", x);
+        }
         logger.flush();
         logger.shutdown();
 

src/subcommand/action.rs

@@ -20,16 +20,22 @@ pub enum Error {
     IoError(io::Error),
     #[display(fmt = "Non-interactive but no operation supplied")]
     MissingOperation,
+    OperationFailed,
     RemoteProcessError(RemoteProcessError),
     TransportError(TransportError),
 }
 
 impl ExitCodeError for Error {
+    fn is_silent(&self) -> bool {
+        matches!(self, Self::OperationFailed)
+    }
+
     fn to_exit_code(&self) -> ExitCode {
         match self {
             Self::BadProcessExit(x) => ExitCode::Custom(*x),
             Self::IoError(x) => x.to_exit_code(),
             Self::MissingOperation => ExitCode::Usage,
+            Self::OperationFailed => ExitCode::Software,
             Self::RemoteProcessError(x) => x.to_exit_code(),
             Self::TransportError(x) => x.to_exit_code(),
         }
@@ -155,8 +161,18 @@ where
             let res = session
                 .send_timeout(Request::new(utils::new_tenant(), vec![data]), timeout)
                 .await?;
+
+            // If we have an error as our payload, then we want to reflect that in our
+            // exit code
+            let is_err = res.payload.iter().any(|d| d.is_error());
+
             ResponseOut::new(cmd.format, res)?.print();
-            Ok(())
+
+            if is_err {
+                Err(Error::OperationFailed)
+            } else {
+                Ok(())
+            }
         }
 
         // Interactive mode will send an optional first request and then continue

tests/action/file_read_test.rs

@@ -1,5 +1,7 @@
-use crate::fixtures::*;
+use crate::{fixtures::*, utils::FAILURE_LINE};
+use assert_cmd::Command;
 use assert_fs::prelude::*;
+use distant::ExitCode;
 use distant_core::{
     data::{Error, ErrorKind},
     Response, ResponseData,
@@ -7,13 +9,13 @@ use distant_core::{
 use rstest::*;
 
 #[rstest]
-fn should_print_out_file_contents(ctx: &'_ DistantServerCtx) {
+fn should_print_out_file_contents(mut action_cmd: Command) {
     let temp = assert_fs::TempDir::new().unwrap();
     let file = temp.child("test-file");
     file.write_str("some\ntext\ncontent").unwrap();
 
     // distant action file-read {path}
-    ctx.new_cmd("action")
+    action_cmd
         .args(&["file-read", file.to_str().unwrap()])
         .assert()
         .success()
@@ -22,14 +24,13 @@ fn should_print_out_file_contents(ctx: &'_ DistantServerCtx) {
 }
 
 #[rstest]
-fn should_support_json_output(ctx: &'_ DistantServerCtx) {
+fn should_support_json_output(mut action_cmd: Command) {
     let temp = assert_fs::TempDir::new().unwrap();
     let file = temp.child("test-file");
     file.write_str("some\ntext\ncontent").unwrap();
 
     // distant action --format json file-read {path}
-    let cmd = ctx
-        .new_cmd("action")
+    let cmd = action_cmd
         .args(&["--format", "json"])
         .args(&["file-read", file.to_str().unwrap()])
         .assert()
@@ -46,17 +47,30 @@ fn should_support_json_output(ctx: &'_ DistantServerCtx) {
 }
 
 #[rstest]
-fn yield_an_error_when_fails(ctx: &'_ DistantServerCtx) {
+fn yield_an_error_when_fails(mut action_cmd: Command) {
+    let temp = assert_fs::TempDir::new().unwrap();
+    let file = temp.child("missing-file");
+
+    // distant action file-read {path}
+    action_cmd
+        .args(&["file-read", file.to_str().unwrap()])
+        .assert()
+        .code(ExitCode::Software.to_i32())
+        .stdout("")
+        .stderr(FAILURE_LINE.clone());
+}
+
+#[rstest]
+fn should_support_json_output_for_error(mut action_cmd: Command) {
     let temp = assert_fs::TempDir::new().unwrap();
     let file = temp.child("missing-file");
 
     // distant action --format json file-read {path}
-    let cmd = ctx
-        .new_cmd("action")
+    let cmd = action_cmd
         .args(&["--format", "json"])
         .args(&["file-read", file.to_str().unwrap()])
         .assert()
-        .success()
+        .code(ExitCode::Software.to_i32())
         .stderr("");
 
     let res: Response = serde_json::from_slice(&cmd.get_output().stdout).unwrap();

tests/e2e_tests.rs

@@ -1,2 +1,3 @@
 mod action;
 mod fixtures;
+mod utils;

tests/fixtures.rs

@@ -1,12 +1,9 @@
 use assert_cmd::Command;
 use distant_core::*;
 use rstest::*;
-use std::{ffi::OsStr, net::SocketAddr, thread, time::Duration};
+use std::{ffi::OsStr, net::SocketAddr, thread};
 use tokio::{runtime::Runtime, sync::mpsc};
 
-/// Timeout to wait for a command to complete
-const TIMEOUT_SECS: u64 = 10;
-
 /// Context for some listening distant server
 pub struct DistantServerCtx {
     pub addr: SocketAddr,
@@ -60,19 +57,11 @@ impl DistantServerCtx {
     /// configured with an environment that can talk to a remote distant server
     pub fn new_cmd(&self, subcommand: impl AsRef<OsStr>) -> Command {
         let mut cmd = Command::cargo_bin(env!("CARGO_PKG_NAME")).unwrap();
-
-        println!("DISTANT_HOST = {}", self.addr.ip());
-        println!("DISTANT_PORT = {}", self.addr.port());
-        println!("DISTANT_AUTH_KEY = {}", self.auth_key);
-
-        // NOTE: We define a command that has a timeout of 10s because the handshake
-        //       involved in a non-release test can take several seconds
         cmd.arg(subcommand)
             .args(&["--session", "environment"])
             .env("DISTANT_HOST", self.addr.ip().to_string())
             .env("DISTANT_PORT", self.addr.port().to_string())
-            .env("DISTANT_AUTH_KEY", self.auth_key.as_str())
-            .timeout(Duration::from_secs(TIMEOUT_SECS));
+            .env("DISTANT_AUTH_KEY", self.auth_key.as_str());
         cmd
     }
 }
@@ -86,9 +75,19 @@ impl Drop for DistantServerCtx {
 
 #[fixture]
 pub fn ctx() -> &'static DistantServerCtx {
-    &DISTANT_SERVER_CTX
+    lazy_static::lazy_static! {
+        static ref CTX: DistantServerCtx = DistantServerCtx::initialize();
+    }
+
+    &CTX
 }
 
-lazy_static::lazy_static! {
-    static ref DISTANT_SERVER_CTX: DistantServerCtx = DistantServerCtx::initialize();
+#[fixture]
+pub fn action_cmd(ctx: &'_ DistantServerCtx) -> Command {
+    ctx.new_cmd("action")
+}
+
+#[fixture]
+pub fn lsp_cmd(ctx: &'_ DistantServerCtx) -> Command {
+    ctx.new_cmd("lsp")
 }

tests/utils.rs

@@ -0,0 +1,7 @@
+use predicates::prelude::*;
+
+lazy_static::lazy_static! {
+    /// Predicate that checks for a single line that is a failure
+    pub static ref FAILURE_LINE: predicates::str::RegexPredicate =
+        predicate::str::is_match(r"^Failed \(.*\): '.*'\.\n$").unwrap();
+}