refactor(test): split lightwalletd test launch into separate methods (#3628)

* fix(test): only run lightwalletd test when the ZEBRA_TEST_LIGHTWALLETD env var is set

* fix(test): actually skip the test

* doc(zebrad): add some test TODOs

* doc(test): document zebrad-specific process launch methods

* refactor(zebrad): split lightwalletd launch into its own testing methods

* fix(zebrad/test): simplify file writing

Co-authored-by: Janito Vaqueiro Ferreira Filho <janito.vff@gmail.com>

* refactor(zebrad/test): rename argument variables

Co-authored-by: Janito Vaqueiro Ferreira Filho <janito.vff@gmail.com>

* lint(zebrad/tests): remove unused import

* fix(zebrad/test): restore SocketAddr import that was removed on main

* rustfmt

* fix(zebrad/test): update integration test to match adityapk00/lightwalletd behaviour

* rustfmt

Co-authored-by: Janito Vaqueiro Ferreira Filho <janito.vff@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

zebrad/tests/acceptance.rs

@@ -27,7 +27,10 @@ use color_eyre::{
 };
 use tempfile::TempDir;
 
-use std::{collections::HashSet, convert::TryInto, env, path::Path, path::PathBuf, time::Duration};
+use std::{
+    collections::HashSet, convert::TryInto, env, net::SocketAddr, path::Path, path::PathBuf,
+    time::Duration,
+};
 
 use zebra_chain::{
     block::Height,
@@ -123,6 +126,8 @@ trait ZebradTestDirExt
 where
     Self: AsRef<Path> + Sized,
 {
+    // Zebra methods
+
     /// 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.
@@ -130,7 +135,7 @@ where
     /// If there is a config in the test directory, pass it to `zebrad`.
     fn spawn_child(self, args: &[&str]) -> Result<TestChild<Self>>;
 
-    /// Create a config file and use it for all subsequently spawned processes.
+    /// Create a config file and use it for all subsequently spawned `zebrad` processes.
     /// Returns an error if the config already exists.
     ///
     /// If needed:
@@ -139,14 +144,14 @@ where
     fn with_config(self, config: &mut ZebradConfig) -> Result<Self>;
 
     /// Create a config file with the exact contents of `config`, and use it for
-    /// all subsequently spawned processes. Returns an error if the config
+    /// all subsequently spawned `zebrad` processes. Returns an error if the config
     /// already exists.
     ///
     /// If needed:
     ///   - recursively create directories for the config and state
     fn with_exact_config(self, config: &ZebradConfig) -> Result<Self>;
 
-    /// Overwrite any existing config file, and use the newly written config for
+    /// Overwrite any existing `zebrad` config file, and use the newly written config for
     /// all subsequently spawned processes.
     ///
     /// If needed:
@@ -154,19 +159,39 @@ where
     ///   - set `config.cache_dir` based on `self`
     fn replace_config(self, config: &mut ZebradConfig) -> Result<Self>;
 
-    /// `cache_dir` config update helper.
+    /// `cache_dir` config update helper for `zebrad`.
     ///
     /// If needed:
     ///   - set the cache_dir in the config.
     fn cache_config_update_helper(self, config: &mut ZebradConfig) -> Result<Self>;
 
-    /// Config writing helper.
+    /// Config writing helper for `zebrad`.
     ///
     /// If needed:
     ///   - recursively create directories for the config and state,
     ///
     /// Then write out the config.
     fn write_config_helper(self, config: &ZebradConfig) -> Result<Self>;
+
+    // lightwalletd methods
+
+    /// Spawn `lightwalletd` with `args` as a child process in this test directory,
+    /// potentially taking ownership of the tempdir for the duration of the
+    /// child process.
+    ///
+    /// By default, launch a working test instance with logging, and avoid port conflicts.
+    ///
+    /// # Panics
+    ///
+    /// If there is no lightwalletd config in the test directory.
+    fn spawn_lightwalletd_child(self, args: &[&str]) -> Result<TestChild<Self>>;
+
+    /// Create a config file and use it for all subsequently spawned `lightwalletd` processes.
+    /// Returns an error if the config already exists.
+    ///
+    /// If needed:
+    ///   - recursively create directories for the config
+    fn with_lightwalletd_config(self, zebra_rpc_listener: SocketAddr) -> Result<Self>;
 }
 
 impl<T> ZebradTestDirExt for T
@@ -174,8 +199,8 @@ where
     Self: TestDirExt + AsRef<Path> + Sized,
 {
     fn spawn_child(self, args: &[&str]) -> Result<TestChild<Self>> {
-        let path = self.as_ref();
+        let dir = self.as_ref();
-        let default_config_path = path.join("zebrad.toml");
+        let default_config_path = dir.join("zebrad.toml");
 
         if default_config_path.exists() {
             let mut extra_args: Vec<_> = vec![
@@ -247,6 +272,69 @@ where
 
         Ok(self)
     }
+
+    fn spawn_lightwalletd_child(self, extra_args: &[&str]) -> Result<TestChild<Self>> {
+        let dir = self.as_ref().to_owned();
+        let default_config_path = dir.join("lightwalletd-zcash.conf");
+
+        assert!(
+            default_config_path.exists(),
+            "lightwalletd requires a config"
+        );
+
+        // By default, launch a working test instance with logging,
+        // and avoid port conflicts.
+        let mut args: Vec<_> = vec![
+            // the fake zcashd conf we just wrote
+            "--zcash-conf-path",
+            default_config_path
+                .as_path()
+                .to_str()
+                .expect("Path is valid Unicode"),
+            // the lightwalletd cache directory
+            //
+            // TODO: create a sub-directory for lightwalletd
+            "--data-dir",
+            dir.to_str().expect("Path is valid Unicode"),
+            // log to standard output
+            //
+            // TODO: if lightwalletd needs to run on Windows,
+            //       work out how to log to the terminal on all platforms
+            "--log-file",
+            "/dev/stdout",
+            // let the OS choose a random available wallet client port
+            "--grpc-bind-addr",
+            "127.0.0.1:0",
+            "--http-bind-addr",
+            "127.0.0.1:0",
+            // don't require a TLS certificate for the HTTP server
+            "--no-tls-very-insecure",
+        ];
+        args.extend_from_slice(extra_args);
+
+        self.spawn_child_with_command("lightwalletd", &args)
+    }
+
+    fn with_lightwalletd_config(self, zebra_rpc_listener: SocketAddr) -> Result<Self> {
+        use std::fs;
+
+        let lightwalletd_config = format!(
+            "\
+            rpcbind={}\n\
+            rpcport={}\n\
+            ",
+            zebra_rpc_listener.ip(),
+            zebra_rpc_listener.port(),
+        );
+
+        let dir = self.as_ref();
+        fs::create_dir_all(dir)?;
+
+        let config_file = dir.join("lightwalletd-zcash.conf");
+        fs::write(config_file, lightwalletd_config.as_bytes())?;
+
+        Ok(self)
+    }
 }
 
 #[test]
@@ -933,6 +1021,7 @@ fn sync_large_checkpoints_mempool_mainnet() -> Result<()> {
 #[test]
 #[ignore]
 fn full_sync_mainnet() {
+    // TODO: add "ZEBRA" at the start of this env var, to avoid clashes
     full_sync_test(Mainnet, "FULL_SYNC_MAINNET_TIMEOUT_MINUTES").expect("unexpected test failure");
 }
 
@@ -944,6 +1033,7 @@ fn full_sync_mainnet() {
 #[test]
 #[ignore]
 fn full_sync_testnet() {
+    // TODO: add "ZEBRA" at the start of this env var, to avoid clashes
     full_sync_test(Testnet, "FULL_SYNC_TESTNET_TIMEOUT_MINUTES").expect("unexpected test failure");
 }
 
@@ -1499,86 +1589,29 @@ fn lightwalletd_integration() -> Result<()> {
     // [Note on port conflict](#Note on port conflict)
     let listen_port = random_known_port();
     let listen_ip = "127.0.0.1".parse().expect("hard-coded IP is valid");
-    let listen_addr = SocketAddr::new(listen_ip, listen_port);
+    let zebra_rpc_listener = SocketAddr::new(listen_ip, listen_port);
 
     // Write a configuration that has the rpc listen_addr option set
     // TODO: split this config into another function?
     let mut config = default_test_config()?;
-    config.rpc.listen_addr = Some(listen_addr);
+    config.rpc.listen_addr = Some(zebra_rpc_listener);
 
-    let dir = testdir()?.with_config(&mut config)?;
+    let zdir = testdir()?.with_config(&mut config)?;
-    let mut zebrad = dir.spawn_child(&["start"])?.with_timeout(LAUNCH_DELAY);
+    let mut zebrad = zdir.spawn_child(&["start"])?.with_timeout(LAUNCH_DELAY);
 
     // Wait until `zebrad` has opened the RPC endpoint
-    zebrad
+    zebrad.expect_stdout_line_matches(
-        .expect_stdout_line_matches(format!("Opened RPC endpoint at {}", listen_addr).as_str())?;
+        format!("Opened RPC endpoint at {}", zebra_rpc_listener).as_str(),
+    )?;
 
     // Launch lightwalletd
-    // TODO: split this into another function
 
     // Write a fake zcashd configuration that has the rpcbind and rpcport options set
-    // TODO: split this into another function
+    let ldir = testdir()?;
-    let dir = testdir()?;
+    let ldir = ldir.with_lightwalletd_config(zebra_rpc_listener)?;
-    let lightwalletd_config = format!(
-        "\
-        rpcbind={}\n\
-        rpcport={}\n\
-        ",
-        listen_ip, listen_port
-    );
-
-    use std::fs;
-    use std::io::Write;
-
-    let path = dir.path().to_owned();
-
-    // TODO: split this into another function
-    if !config.state.ephemeral {
-        let cache_dir = path.join("state");
-        fs::create_dir_all(&cache_dir)?;
-    } else {
-        fs::create_dir_all(&path)?;
-    }
-
-    let config_file = path.join("lightwalletd-zcash.conf");
-    fs::File::create(config_file)?.write_all(lightwalletd_config.as_bytes())?;
-
-    let result = {
-        let default_config_path = path.join("lightwalletd-zcash.conf");
-
-        assert!(
-            default_config_path.exists(),
-            "lightwalletd requires a config"
-        );
-
-        dir.spawn_child_with_command(
-            "lightwalletd",
-            &[
-                // the fake zcashd conf we just wrote
-                "--zcash-conf-path",
-                default_config_path
-                    .as_path()
-                    .to_str()
-                    .expect("Path is valid Unicode"),
-                // the lightwalletd cache directory
-                //
-                // TODO: create a sub-directory for lightwalletd
-                "--data-dir",
-                path.to_str().expect("Path is valid Unicode"),
-                // log to standard output
-                "--log-file",
-                "/dev/stdout",
-                // randomise wallet client ports
-                "--grpc-bind-addr",
-                "127.0.0.1:0",
-                "--http-bind-addr",
-                "127.0.0.1:0",
-                // don't require a TLS certificate
-                "--no-tls-very-insecure",
-            ],
-        )
-    };
 
+    // Launch the lightwalletd process
+    let result = ldir.spawn_lightwalletd_child(&[]);
     let (lightwalletd, zebrad) = zebrad.kill_on_error(result)?;
     let mut lightwalletd = lightwalletd.with_timeout(LAUNCH_DELAY);
 
@@ -1601,8 +1634,11 @@ fn lightwalletd_integration() -> Result<()> {
     //
     // TODO: update the missing method name when we add a new Zebra RPC
 
-    let result = lightwalletd
+    // Note:
-        .expect_stdout_line_matches("Method not found.*error zcashd getbestblockhash rpc");
+    // zcash/lightwalletd calls getbestblockhash here, but
+    // adityapk00/lightwalletd calls getblock
+    let result =
+        lightwalletd.expect_stdout_line_matches("Method not found.*error zcashd getblock rpc");
     let (_, zebrad) = zebrad.kill_on_error(result)?;
     let result = lightwalletd.expect_stdout_line_matches(
         "Lightwalletd died with a Fatal error. Check logfile for details",