From fde0ec5ed0ba9ca553df336132316028485cb154 Mon Sep 17 00:00:00 2001 From: Alejandro Martinez Andres <11448715+al3mart@users.noreply.github.com> Date: Wed, 18 Dec 2024 20:36:21 +0100 Subject: [PATCH 1/9] test: improve common/contracts coverage --- crates/pop-cli/src/common/contracts.rs | 27 ++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/crates/pop-cli/src/common/contracts.rs b/crates/pop-cli/src/common/contracts.rs index c353d196..0833e45a 100644 --- a/crates/pop-cli/src/common/contracts.rs +++ b/crates/pop-cli/src/common/contracts.rs @@ -115,6 +115,7 @@ pub fn has_contract_been_built(path: Option<&Path>) -> bool { #[cfg(test)] mod tests { use super::*; + use crate::cli::MockCli; use duct::cmd; use std::fs::{self, File}; @@ -138,4 +139,30 @@ mod tests { assert!(has_contract_been_built(Some(&path.join(name)))); Ok(()) } + + #[tokio::test] + async fn check_contracts_node_and_prompt_works() -> anyhow::Result<()> { + let cache_path: PathBuf = crate::cache()?; + let cli = MockCli::new() + .expect_warning("⚠️ The substrate-contracts-node binary is not found.") + .expect_confirm("📦 Would you like to source it automatically now?", true) + .expect_warning("⚠️ The substrate-contracts-node binary is not found."); + + let node_path = check_contracts_node_and_prompt(false).await?; + // Binary path is at least equals cache path + "substrate-contracts-node". + assert!(node_path + .to_str() + .unwrap() + .starts_with(&cache_path.join("substrate-contracts-node").to_str().unwrap())); + Ok(()) + } + + #[tokio::test] + async fn node_is_terminated() -> anyhow::Result<()> { + let cli = MockCli::new() + .expect_confirm("Would you like to terminate the local node?", false) + .expect_warning("NOTE: The node is running in the background with process ID 0. Please terminate it manually when done."); + Ok(()) + } + } From 8491d302f625c3021d77fd6a54f0a53a738d1462 Mon Sep 17 00:00:00 2001 From: Alejandro Martinez Andres <11448715+al3mart@users.noreply.github.com> Date: Wed, 18 Dec 2024 20:38:10 +0100 Subject: [PATCH 2/9] style: fmt --- crates/pop-cli/src/common/contracts.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/pop-cli/src/common/contracts.rs b/crates/pop-cli/src/common/contracts.rs index 0833e45a..f157c79a 100644 --- a/crates/pop-cli/src/common/contracts.rs +++ b/crates/pop-cli/src/common/contracts.rs @@ -164,5 +164,4 @@ mod tests { .expect_warning("NOTE: The node is running in the background with process ID 0. Please terminate it manually when done."); Ok(()) } - } From 2075be5239b59469b79d5c4f51c856827acef503 Mon Sep 17 00:00:00 2001 From: Alejandro Martinez Andres <11448715+al3mart@users.noreply.github.com> Date: Wed, 18 Dec 2024 22:31:54 +0100 Subject: [PATCH 3/9] refactor: include Cli in methods --- crates/pop-cli/src/commands/test/contract.rs | 2 +- crates/pop-cli/src/commands/up/contract.rs | 37 ++++++------- crates/pop-cli/src/common/contracts.rs | 57 ++++++++++++++------ 3 files changed, 62 insertions(+), 34 deletions(-) diff --git a/crates/pop-cli/src/commands/test/contract.rs b/crates/pop-cli/src/commands/test/contract.rs index 190d9ac6..73a7fbf6 100644 --- a/crates/pop-cli/src/commands/test/contract.rs +++ b/crates/pop-cli/src/commands/test/contract.rs @@ -48,7 +48,7 @@ impl TestContractCommand { sleep(Duration::from_secs(3)).await; } - self.node = match check_contracts_node_and_prompt(self.skip_confirm).await { + self.node = match check_contracts_node_and_prompt(&mut Cli, self.skip_confirm).await { Ok(binary_path) => Some(binary_path), Err(_) => { warning("🚫 substrate-contracts-node is necessary to run e2e tests. Will try to run tests anyway...")?; diff --git a/crates/pop-cli/src/commands/up/contract.rs b/crates/pop-cli/src/commands/up/contract.rs index fed9c206..6b404bb0 100644 --- a/crates/pop-cli/src/commands/up/contract.rs +++ b/crates/pop-cli/src/commands/up/contract.rs @@ -139,15 +139,16 @@ impl UpContractCommand { let log = NamedTempFile::new()?; // uses the cache location - let binary_path = match check_contracts_node_and_prompt(self.skip_confirm).await { - Ok(binary_path) => binary_path, - Err(_) => { - Cli.outro_cancel( - "🚫 You need to specify an accessible endpoint to deploy the contract.", - )?; - return Ok(()); - }, - }; + let binary_path = + match check_contracts_node_and_prompt(&mut Cli, self.skip_confirm).await { + Ok(binary_path) => binary_path, + Err(_) => { + Cli.outro_cancel( + "🚫 You need to specify an accessible endpoint to deploy the contract.", + )?; + return Ok(()); + }, + }; let spinner = spinner(); spinner.start("Starting local node..."); @@ -181,7 +182,7 @@ impl UpContractCommand { Ok(data) => data, Err(e) => { error(format!("An error occurred getting the call data: {e}"))?; - terminate_node(process)?; + terminate_node(&mut Cli, process)?; Cli.outro_cancel(FAILED)?; return Ok(()); }, @@ -200,7 +201,7 @@ impl UpContractCommand { Err(e) => { spinner .error(format!("An error occurred uploading your contract: {e}")); - terminate_node(process)?; + terminate_node(&mut Cli, process)?; Cli.outro_cancel(FAILED)?; return Ok(()); }, @@ -223,7 +224,7 @@ impl UpContractCommand { spinner.error(format!( "An error occurred uploading your contract: {e}" )); - terminate_node(process)?; + terminate_node(&mut Cli, process)?; Cli.outro_cancel(FAILED)?; return Ok(()); }, @@ -243,11 +244,11 @@ impl UpContractCommand { } } else { Cli.outro_cancel("Signed payload doesn't exist.")?; - terminate_node(process)?; + terminate_node(&mut Cli, process)?; return Ok(()); } - terminate_node(process)?; + terminate_node(&mut Cli, process)?; Cli.outro(COMPLETE)?; return Ok(()); } @@ -255,7 +256,7 @@ impl UpContractCommand { // Check for upload only. if self.upload_only { let result = self.upload_contract().await; - terminate_node(process)?; + terminate_node(&mut Cli, process)?; match result { Ok(_) => { Cli.outro(COMPLETE)?; @@ -272,7 +273,7 @@ impl UpContractCommand { Ok(i) => i, Err(e) => { error(format!("An error occurred instantiating the contract: {e}"))?; - terminate_node(process)?; + terminate_node(&mut Cli, process)?; Cli.outro_cancel(FAILED)?; return Ok(()); }, @@ -290,7 +291,7 @@ impl UpContractCommand { }, Err(e) => { spinner.error(format!("{e}")); - terminate_node(process)?; + terminate_node(&mut Cli, process)?; Cli.outro_cancel(FAILED)?; return Ok(()); }, @@ -308,7 +309,7 @@ impl UpContractCommand { contract_info.code_hash, ); - terminate_node(process)?; + terminate_node(&mut Cli, process)?; Cli.outro(COMPLETE)?; } diff --git a/crates/pop-cli/src/common/contracts.rs b/crates/pop-cli/src/common/contracts.rs index f157c79a..0ef2b041 100644 --- a/crates/pop-cli/src/common/contracts.rs +++ b/crates/pop-cli/src/common/contracts.rs @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-3.0 -use cliclack::{confirm, log::warning, spinner}; +use crate::cli::traits::*; +use cliclack::spinner; use pop_common::{manifest::from_path, sourcing::set_executable_permission}; use pop_contracts::contracts_node_generator; use std::{ @@ -14,13 +15,17 @@ use tempfile::NamedTempFile; /// /// # Arguments /// * `skip_confirm`: A boolean indicating whether to skip confirmation prompts. -pub async fn check_contracts_node_and_prompt(skip_confirm: bool) -> anyhow::Result { +pub async fn check_contracts_node_and_prompt( + cli: &mut impl Cli, + skip_confirm: bool, +) -> anyhow::Result { let cache_path: PathBuf = crate::cache()?; let mut binary = contracts_node_generator(cache_path, None).await?; let mut node_path = binary.path(); if !binary.exists() { - warning("⚠️ The substrate-contracts-node binary is not found.")?; - if confirm("📦 Would you like to source it automatically now?") + cli.warning("⚠️ The substrate-contracts-node binary is not found.")?; + if cli + .confirm("📦 Would you like to source it automatically now?") .initial_value(true) .interact()? { @@ -37,14 +42,14 @@ pub async fn check_contracts_node_and_prompt(skip_confirm: bool) -> anyhow::Resu } } if binary.stale() { - warning(format!( + cli.warning(format!( "ℹ️ There is a newer version of {} available:\n {} -> {}", binary.name(), binary.version().unwrap_or("None"), binary.latest().unwrap_or("None") ))?; let latest = if !skip_confirm { - confirm( + cli.confirm( "📦 Would you like to source it automatically now? It may take some time..." .to_string(), ) @@ -73,12 +78,16 @@ pub async fn check_contracts_node_and_prompt(skip_confirm: bool) -> anyhow::Resu } /// Handles the optional termination of a local running node. -pub fn terminate_node(process: Option<(Child, NamedTempFile)>) -> anyhow::Result<()> { +pub fn terminate_node( + cli: &mut impl Cli, + process: Option<(Child, NamedTempFile)>, +) -> anyhow::Result<()> { // Prompt to close any launched node let Some((process, log)) = process else { return Ok(()); }; - if confirm("Would you like to terminate the local node?") + if cli + .confirm("Would you like to terminate the local node?") .initial_value(true) .interact()? { @@ -89,7 +98,7 @@ pub fn terminate_node(process: Option<(Child, NamedTempFile)>) -> anyhow::Result .wait()?; } else { log.keep()?; - warning(format!("NOTE: The node is running in the background with process ID {}. Please terminate it manually when done.", process.id()))?; + cli.warning(format!("NOTE: The node is running in the background with process ID {}. Please terminate it manually when done.", process.id()))?; } Ok(()) @@ -117,7 +126,14 @@ mod tests { use super::*; use crate::cli::MockCli; use duct::cmd; - use std::fs::{self, File}; + use pop_common::find_free_port; + use pop_contracts::{is_chain_alive, run_contracts_node}; + use std::{ + fs::{self, File}, + thread::sleep, + time::Duration, + }; + use url::Url; #[test] fn has_contract_been_built_works() -> anyhow::Result<()> { @@ -143,25 +159,36 @@ mod tests { #[tokio::test] async fn check_contracts_node_and_prompt_works() -> anyhow::Result<()> { let cache_path: PathBuf = crate::cache()?; - let cli = MockCli::new() + let mut cli = MockCli::new() .expect_warning("⚠️ The substrate-contracts-node binary is not found.") .expect_confirm("📦 Would you like to source it automatically now?", true) .expect_warning("⚠️ The substrate-contracts-node binary is not found."); - let node_path = check_contracts_node_and_prompt(false).await?; + let node_path = check_contracts_node_and_prompt(&mut cli, false).await?; // Binary path is at least equals cache path + "substrate-contracts-node". assert!(node_path .to_str() .unwrap() .starts_with(&cache_path.join("substrate-contracts-node").to_str().unwrap())); + cli.verify()?; Ok(()) } #[tokio::test] async fn node_is_terminated() -> anyhow::Result<()> { - let cli = MockCli::new() - .expect_confirm("Would you like to terminate the local node?", false) - .expect_warning("NOTE: The node is running in the background with process ID 0. Please terminate it manually when done."); + let cache = tempfile::tempdir().expect("Could not create temp dir"); + let binary = contracts_node_generator(PathBuf::from(cache.path()), None).await?; + binary.source(false, &(), true).await?; + set_executable_permission(binary.path())?; + let port = find_free_port(None); + let process = run_contracts_node(binary.path(), None, port).await?; + let log = NamedTempFile::new()?; + // Terminate the process. + let mut cli = + MockCli::new().expect_confirm("Would you like to terminate the local node?", true); + assert!(terminate_node(&mut cli, Some((process, log))).is_ok()); + cli.verify()?; + assert_eq!(is_chain_alive(Url::parse(&format!("ws:localhost:{}", port))?).await?, false); Ok(()) } } From 6ccb5d508599f34a6097e5f5d34b719bd4b57f2c Mon Sep 17 00:00:00 2001 From: Alejandro Martinez Andres <11448715+al3mart@users.noreply.github.com> Date: Wed, 18 Dec 2024 22:36:52 +0100 Subject: [PATCH 4/9] style: typo --- crates/pop-cli/src/common/contracts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/pop-cli/src/common/contracts.rs b/crates/pop-cli/src/common/contracts.rs index 0ef2b041..05f2f113 100644 --- a/crates/pop-cli/src/common/contracts.rs +++ b/crates/pop-cli/src/common/contracts.rs @@ -188,7 +188,7 @@ mod tests { MockCli::new().expect_confirm("Would you like to terminate the local node?", true); assert!(terminate_node(&mut cli, Some((process, log))).is_ok()); cli.verify()?; - assert_eq!(is_chain_alive(Url::parse(&format!("ws:localhost:{}", port))?).await?, false); + assert_eq!(is_chain_alive(Url::parse(&format!("ws://localhost:{}", port))?).await?, false); Ok(()) } } From 6a23175cbd45fa6b6fb92335d426f652b74d6484 Mon Sep 17 00:00:00 2001 From: Alejandro Martinez Andres <11448715+al3mart@users.noreply.github.com> Date: Wed, 18 Dec 2024 22:45:57 +0100 Subject: [PATCH 5/9] fix: improve tests --- crates/pop-cli/src/common/contracts.rs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/crates/pop-cli/src/common/contracts.rs b/crates/pop-cli/src/common/contracts.rs index 05f2f113..31e22edf 100644 --- a/crates/pop-cli/src/common/contracts.rs +++ b/crates/pop-cli/src/common/contracts.rs @@ -128,11 +128,7 @@ mod tests { use duct::cmd; use pop_common::find_free_port; use pop_contracts::{is_chain_alive, run_contracts_node}; - use std::{ - fs::{self, File}, - thread::sleep, - time::Duration, - }; + use std::fs::{self, File}; use url::Url; #[test] @@ -158,7 +154,9 @@ mod tests { #[tokio::test] async fn check_contracts_node_and_prompt_works() -> anyhow::Result<()> { - let cache_path: PathBuf = crate::cache()?; + //let cache_path: PathBuf = crate::cache()?; + + let cache_path = tempfile::tempdir().expect("Could create temp dir"); let mut cli = MockCli::new() .expect_warning("⚠️ The substrate-contracts-node binary is not found.") .expect_confirm("📦 Would you like to source it automatically now?", true) @@ -169,9 +167,8 @@ mod tests { assert!(node_path .to_str() .unwrap() - .starts_with(&cache_path.join("substrate-contracts-node").to_str().unwrap())); - cli.verify()?; - Ok(()) + .starts_with(&cache_path.path().join("substrate-contracts-node").to_str().unwrap())); + cli.verify() } #[tokio::test] @@ -187,8 +184,7 @@ mod tests { let mut cli = MockCli::new().expect_confirm("Would you like to terminate the local node?", true); assert!(terminate_node(&mut cli, Some((process, log))).is_ok()); - cli.verify()?; assert_eq!(is_chain_alive(Url::parse(&format!("ws://localhost:{}", port))?).await?, false); - Ok(()) + cli.verify() } } From 0c000b64b9e47a484a02251fd24e7ae4f6496b8f Mon Sep 17 00:00:00 2001 From: Alejandro Martinez Andres <11448715+al3mart@users.noreply.github.com> Date: Wed, 18 Dec 2024 22:46:53 +0100 Subject: [PATCH 6/9] style: better comment --- crates/pop-cli/src/common/contracts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/pop-cli/src/common/contracts.rs b/crates/pop-cli/src/common/contracts.rs index 31e22edf..014743e7 100644 --- a/crates/pop-cli/src/common/contracts.rs +++ b/crates/pop-cli/src/common/contracts.rs @@ -163,7 +163,7 @@ mod tests { .expect_warning("⚠️ The substrate-contracts-node binary is not found."); let node_path = check_contracts_node_and_prompt(&mut cli, false).await?; - // Binary path is at least equals cache path + "substrate-contracts-node". + // Binary path is at least equals to the cache path + "substrate-contracts-node". assert!(node_path .to_str() .unwrap() From 5a3b977f842a5729d1e70f3415267266a56b4b29 Mon Sep 17 00:00:00 2001 From: Alejandro Martinez Andres <11448715+al3mart@users.noreply.github.com> Date: Wed, 18 Dec 2024 22:56:13 +0100 Subject: [PATCH 7/9] refactor: include cache path as param in check_contracts_node_and_prompt --- crates/pop-cli/src/commands/test/contract.rs | 8 ++++++- crates/pop-cli/src/commands/up/contract.rs | 25 ++++++++++++-------- crates/pop-cli/src/common/contracts.rs | 8 +++---- 3 files changed, 25 insertions(+), 16 deletions(-) diff --git a/crates/pop-cli/src/commands/test/contract.rs b/crates/pop-cli/src/commands/test/contract.rs index 73a7fbf6..05e842ec 100644 --- a/crates/pop-cli/src/commands/test/contract.rs +++ b/crates/pop-cli/src/commands/test/contract.rs @@ -48,7 +48,13 @@ impl TestContractCommand { sleep(Duration::from_secs(3)).await; } - self.node = match check_contracts_node_and_prompt(&mut Cli, self.skip_confirm).await { + self.node = match check_contracts_node_and_prompt( + &mut Cli, + &crate::cache()?, + self.skip_confirm, + ) + .await + { Ok(binary_path) => Some(binary_path), Err(_) => { warning("🚫 substrate-contracts-node is necessary to run e2e tests. Will try to run tests anyway...")?; diff --git a/crates/pop-cli/src/commands/up/contract.rs b/crates/pop-cli/src/commands/up/contract.rs index 6b404bb0..32bafcf3 100644 --- a/crates/pop-cli/src/commands/up/contract.rs +++ b/crates/pop-cli/src/commands/up/contract.rs @@ -139,16 +139,21 @@ impl UpContractCommand { let log = NamedTempFile::new()?; // uses the cache location - let binary_path = - match check_contracts_node_and_prompt(&mut Cli, self.skip_confirm).await { - Ok(binary_path) => binary_path, - Err(_) => { - Cli.outro_cancel( - "🚫 You need to specify an accessible endpoint to deploy the contract.", - )?; - return Ok(()); - }, - }; + let binary_path = match check_contracts_node_and_prompt( + &mut Cli, + &crate::cache()?, + self.skip_confirm, + ) + .await + { + Ok(binary_path) => binary_path, + Err(_) => { + Cli.outro_cancel( + "🚫 You need to specify an accessible endpoint to deploy the contract.", + )?; + return Ok(()); + }, + }; let spinner = spinner(); spinner.start("Starting local node..."); diff --git a/crates/pop-cli/src/common/contracts.rs b/crates/pop-cli/src/common/contracts.rs index 014743e7..2b0b23e9 100644 --- a/crates/pop-cli/src/common/contracts.rs +++ b/crates/pop-cli/src/common/contracts.rs @@ -17,10 +17,10 @@ use tempfile::NamedTempFile; /// * `skip_confirm`: A boolean indicating whether to skip confirmation prompts. pub async fn check_contracts_node_and_prompt( cli: &mut impl Cli, + cache_path: &Path, skip_confirm: bool, ) -> anyhow::Result { - let cache_path: PathBuf = crate::cache()?; - let mut binary = contracts_node_generator(cache_path, None).await?; + let mut binary = contracts_node_generator(PathBuf::from(cache_path), None).await?; let mut node_path = binary.path(); if !binary.exists() { cli.warning("⚠️ The substrate-contracts-node binary is not found.")?; @@ -154,15 +154,13 @@ mod tests { #[tokio::test] async fn check_contracts_node_and_prompt_works() -> anyhow::Result<()> { - //let cache_path: PathBuf = crate::cache()?; - let cache_path = tempfile::tempdir().expect("Could create temp dir"); let mut cli = MockCli::new() .expect_warning("⚠️ The substrate-contracts-node binary is not found.") .expect_confirm("📦 Would you like to source it automatically now?", true) .expect_warning("⚠️ The substrate-contracts-node binary is not found."); - let node_path = check_contracts_node_and_prompt(&mut cli, false).await?; + let node_path = check_contracts_node_and_prompt(&mut cli, cache_path.path(), false).await?; // Binary path is at least equals to the cache path + "substrate-contracts-node". assert!(node_path .to_str() From 19e708402d2788d978fe5779aa66a6fb2294ef49 Mon Sep 17 00:00:00 2001 From: Alejandro Martinez Andres <11448715+al3mart@users.noreply.github.com> Date: Wed, 18 Dec 2024 23:10:35 +0100 Subject: [PATCH 8/9] docs: improve function docs --- crates/pop-cli/src/common/contracts.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/pop-cli/src/common/contracts.rs b/crates/pop-cli/src/common/contracts.rs index 2b0b23e9..4c5bf2a7 100644 --- a/crates/pop-cli/src/common/contracts.rs +++ b/crates/pop-cli/src/common/contracts.rs @@ -14,6 +14,8 @@ use tempfile::NamedTempFile; /// prompts the user to update it if the existing binary is not the latest version. /// /// # Arguments +/// * `cli`: Command line interface. +/// * `cache_path`: The cache directory path. /// * `skip_confirm`: A boolean indicating whether to skip confirmation prompts. pub async fn check_contracts_node_and_prompt( cli: &mut impl Cli, @@ -78,6 +80,9 @@ pub async fn check_contracts_node_and_prompt( } /// Handles the optional termination of a local running node. +/// # Arguments +/// * `cli`: Command line interface. +/// * `process`: Tuple identifying the child process to terminate and its log file. pub fn terminate_node( cli: &mut impl Cli, process: Option<(Child, NamedTempFile)>, From e8f44133a0cbc75677156e40fba4facc3d22963a Mon Sep 17 00:00:00 2001 From: Alejandro Martinez Andres <11448715+al3mart@users.noreply.github.com> Date: Wed, 18 Dec 2024 23:13:16 +0100 Subject: [PATCH 9/9] remove typo in comment --- crates/pop-cli/src/common/contracts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/pop-cli/src/common/contracts.rs b/crates/pop-cli/src/common/contracts.rs index 4c5bf2a7..a3b8f878 100644 --- a/crates/pop-cli/src/common/contracts.rs +++ b/crates/pop-cli/src/common/contracts.rs @@ -166,7 +166,7 @@ mod tests { .expect_warning("⚠️ The substrate-contracts-node binary is not found."); let node_path = check_contracts_node_and_prompt(&mut cli, cache_path.path(), false).await?; - // Binary path is at least equals to the cache path + "substrate-contracts-node". + // Binary path is at least equal to the cache path + "substrate-contracts-node". assert!(node_path .to_str() .unwrap()