From 32bbc19c6bce3e03b39ba8e2caf707ac7803d20c Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Fri, 9 Oct 2020 22:37:43 +1000
Subject: [PATCH] Fix a timeout bug in zebra_test::command

And add tests for the command functionality.

Also document some remaining bugs (see #1140).
---
 Cargo.lock                 |  1 +
 zebra-test/Cargo.toml      |  1 +
 zebra-test/src/command.rs  | 16 +++++++++++++++-
 zebrad/tests/acceptance.rs | 11 ++++++-----
 4 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index b0992155..46e8c6e6 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -3292,6 +3292,7 @@ dependencies = [
  "proptest",
  "regex",
  "spandoc",
+ "tempdir",
  "thiserror",
  "tokio",
  "tower",
diff --git a/zebra-test/Cargo.toml b/zebra-test/Cargo.toml
index df3b7058..b115880f 100644
--- a/zebra-test/Cargo.toml
+++ b/zebra-test/Cargo.toml
@@ -22,6 +22,7 @@ thiserror = "1.0.21"
 pretty_assertions = "0.6.1"
 owo-colors = "1.1.3"
 proptest = "0.10.1"
+tempdir = "0.3.7"
 
 [dev-dependencies]
 tokio = { version = "0.2", features = ["full"] }
diff --git a/zebra-test/src/command.rs b/zebra-test/src/command.rs
index ea91de06..ced8f81f 100644
--- a/zebra-test/src/command.rs
+++ b/zebra-test/src/command.rs
@@ -123,6 +123,7 @@ pub struct TestChild<T> {
 }
 
 impl<T> TestChild<T> {
+    /// Kill the child process.
     #[spandoc::spandoc]
     pub fn kill(&mut self) -> Result<()> {
         /// SPANDOC: Killing child process
@@ -131,6 +132,9 @@ impl<T> TestChild<T> {
         Ok(())
     }
 
+    /// Waits for the child process to exit, then returns its output.
+    ///
+    /// Ignores any configured timeouts.
     #[spandoc::spandoc]
     pub fn wait_with_output(self) -> Result<TestOutput<T>> {
         /// SPANDOC: waiting for command to exit
@@ -146,11 +150,18 @@ impl<T> TestChild<T> {
         })
     }
 
+    /// Set a timeout for `expect_stdout`.
+    ///
+    /// Does not apply to `wait_with_output`.
     pub fn with_timeout(mut self, timeout: Duration) -> Self {
         self.deadline = Some(Instant::now() + timeout);
         self
     }
 
+    /// Checks each line of the child's stdout against `regex`, and returns matching lines.
+    ///
+    /// Kills the child after the configured timeout has elapsed.
+    /// Note: the timeout is only checked after each line.
     #[instrument(skip(self))]
     pub fn expect_stdout(&mut self, regex: &str) -> Result<&mut Self> {
         if self.stdout.is_none() {
@@ -186,7 +197,10 @@ impl<T> TestChild<T> {
             }
         }
 
-        if self.past_deadline() && !self.is_running() {
+        if self.past_deadline() && self.is_running() {
+            // If the process exits between is_running and kill, we will see
+            // spurious errors here. If that happens, ignore "no such process"
+            // errors from kill.
             self.kill()?;
         }
 
diff --git a/zebrad/tests/acceptance.rs b/zebrad/tests/acceptance.rs
index 0d94c57f..770c6293 100644
--- a/zebrad/tests/acceptance.rs
+++ b/zebrad/tests/acceptance.rs
@@ -44,13 +44,14 @@ fn testdir() -> Result<TempDir> {
 }
 
 /// Extension trait for methods on `tempdir::TempDir` for using it as a test
-/// directory.
-trait TestDirExt
+/// directory for `zebrad`.
+trait ZebradTestDirExt
 where
     Self: Borrow<TempDir> + Sized,
 {
-    /// Spawn the zebrad as a child process in this test directory, potentially
-    /// taking ownership of the tempdir for the duration of the child process.
+    /// Spawn `zebrad` with `args` as a child process in this test directory,
+    /// potentially taking ownership of the tempdir for the duration of the
+    /// child process.
     fn spawn_child(self, args: &[&str]) -> Result<TestChild<Self>>;
 
     /// Add the given config to the test directory and use it for all
@@ -58,7 +59,7 @@ where
     fn with_config(self, config: ZebradConfig) -> Result<Self>;
 }
 
-impl<T> TestDirExt for T
+impl<T> ZebradTestDirExt for T
 where
     Self: Borrow<TempDir> + Sized,
 {