Fix try-runtime follow-chain, try-runtime upgrade tuple tests, cli test utils (#13794)

liamaharon · kianenigma · koute · web-flow · commit a1d3d5004379 · 2023-04-06T19:49:53.000+10:00
* new test for try-runtime tuple stuff

* fix

* remove development comment

* formatting

* remove todo comment

* follow-chain working test

* refactor common cli testing utils

* fix comment

* revert Cargo.lock changes

* update Cargo.lock

* improve doc comment

* fix error typo

* update Cargo.lock

* feature gate try-runtime test

* build_substrate cli test util

* feature gate follow_chain tests

* move fn start_node to test-utils

* improve test pkg name

* use tokio Child and Command

* remove redundant import

* fix ci

* fix ci

* don't leave hanging processes

* improved child process cleanup

* use existing KillChildOnDrop

* remove redundant comment

* Update test-utils/cli/src/lib.rs

Co-authored-by: Koute &lt;koute@users.noreply.github.com&gt;

---------

Co-authored-by: kianenigma &lt;kian@parity.io&gt;
Co-authored-by: Koute &lt;koute@users.noreply.github.com&gt;
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/bin/node/cli/Cargo.toml b/bin/node/cli/Cargo.toml
@@ -127,6 +127,7 @@ tokio-util = { version = "0.7.4", features = ["compat"] }
 wait-timeout = "0.2"
 substrate-rpc-client = { path = "../../../utils/frame/rpc/client" }
 pallet-timestamp = { version = "4.0.0-dev", path = "../../../frame/timestamp" }
+substrate-cli-test-utils = { path = "../../../test-utils/cli" }
 
 [build-dependencies]
 clap = { version = "4.0.9", optional = true }
diff --git a/bin/node/cli/tests/benchmark_block_works.rs b/bin/node/cli/tests/benchmark_block_works.rs
@@ -23,7 +23,7 @@ use assert_cmd::cargo::cargo_bin;
 use std::process::Command;
 use tempfile::tempdir;
 
-pub mod common;
+use substrate_cli_test_utils as common;
 
 /// `benchmark block` works for the dev runtime using the wasm executor.
 #[tokio::test]
diff --git a/bin/node/cli/tests/benchmark_pallet_works.rs b/bin/node/cli/tests/benchmark_pallet_works.rs
@@ -21,8 +21,6 @@
 use assert_cmd::cargo::cargo_bin;
 use std::process::Command;
 
-pub mod common;
-
 /// `benchmark pallet` works for the different combinations of `steps` and `repeat`.
 #[test]
 fn benchmark_pallet_works() {
diff --git a/bin/node/cli/tests/check_block_works.rs b/bin/node/cli/tests/check_block_works.rs
@@ -22,7 +22,7 @@ use assert_cmd::cargo::cargo_bin;
 use std::process::Command;
 use tempfile::tempdir;
 
-pub mod common;
+use substrate_cli_test_utils as common;
 
 #[tokio::test]
 async fn check_block_works() {
diff --git a/bin/node/cli/tests/export_import_flow.rs b/bin/node/cli/tests/export_import_flow.rs
@@ -23,7 +23,7 @@ use regex::Regex;
 use std::{fs, path::PathBuf, process::Command};
 use tempfile::{tempdir, TempDir};
 
-pub mod common;
+use substrate_cli_test_utils as common;
 
 fn contains_error(logged_output: &str) -> bool {
 	logged_output.contains("Error")
diff --git a/bin/node/cli/tests/inspect_works.rs b/bin/node/cli/tests/inspect_works.rs
@@ -22,7 +22,7 @@ use assert_cmd::cargo::cargo_bin;
 use std::process::Command;
 use tempfile::tempdir;
 
-pub mod common;
+use substrate_cli_test_utils as common;
 
 #[tokio::test]
 async fn inspect_works() {
diff --git a/bin/node/cli/tests/purge_chain_works.rs b/bin/node/cli/tests/purge_chain_works.rs
@@ -20,7 +20,7 @@ use assert_cmd::cargo::cargo_bin;
 use std::process::Command;
 use tempfile::tempdir;
 
-pub mod common;
+use substrate_cli_test_utils as common;
 
 #[tokio::test]
 #[cfg(unix)]
diff --git a/bin/node/cli/tests/remember_state_pruning_works.rs b/bin/node/cli/tests/remember_state_pruning_works.rs
@@ -18,7 +18,7 @@
 
 use tempfile::tempdir;
 
-pub mod common;
+use substrate_cli_test_utils as common;
 
 #[tokio::test]
 #[cfg(unix)]
diff --git a/bin/node/cli/tests/running_the_node_and_interrupt.rs b/bin/node/cli/tests/running_the_node_and_interrupt.rs
@@ -20,12 +20,12 @@
 use assert_cmd::cargo::cargo_bin;
 use nix::sys::signal::Signal::{self, SIGINT, SIGTERM};
 use std::{
-	process::{self, Child, Command},
+	process::{self, Command},
 	time::Duration,
 };
 use tempfile::tempdir;
 
-pub mod common;
+use substrate_cli_test_utils as common;
 
 #[tokio::test]
 async fn running_the_node_works_and_can_be_interrupted() {
@@ -71,17 +71,8 @@ async fn running_the_node_works_and_can_be_interrupted() {
 #[tokio::test]
 async fn running_two_nodes_with_the_same_ws_port_should_work() {
 	common::run_with_timeout(Duration::from_secs(60 * 10), async move {
-		fn start_node() -> Child {
-			Command::new(cargo_bin("substrate"))
-				.stdout(process::Stdio::piped())
-				.stderr(process::Stdio::piped())
-				.args(&["--dev", "--tmp", "--ws-port=45789", "--no-hardware-benchmarks"])
-				.spawn()
-				.unwrap()
-		}
-
-		let mut first_node = common::KillChildOnDrop(start_node());
-		let mut second_node = common::KillChildOnDrop(start_node());
+		let mut first_node = common::KillChildOnDrop(common::start_node());
+		let mut second_node = common::KillChildOnDrop(common::start_node());
 
 		let stderr = first_node.stderr.take().unwrap();
 		let ws_url = common::extract_info_from_output(stderr).0.ws_url;
diff --git a/bin/node/cli/tests/telemetry.rs b/bin/node/cli/tests/telemetry.rs
@@ -21,7 +21,7 @@ use std::{process, time::Duration};
 
 use crate::common::KillChildOnDrop;
 
-pub mod common;
+use substrate_cli_test_utils as common;
 pub mod websocket_server;
 
 #[tokio::test]
diff --git a/bin/node/cli/tests/temp_base_path_works.rs b/bin/node/cli/tests/temp_base_path_works.rs
@@ -24,7 +24,7 @@ use std::{
 	time::Duration,
 };
 
-pub mod common;
+use substrate_cli_test_utils as common;
 
 #[allow(dead_code)]
 // Apparently `#[ignore]` doesn't actually work to disable this one.
diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs
@@ -371,16 +371,14 @@ macro_rules! parameter_types {
 
 			/// Set the value of this parameter type in the storage.
 			///
-			/// This needs to be executed in an externalities provided
-			/// environment.
+			/// This needs to be executed in an externalities provided environment.
 			pub fn set(value: &$type) {
 				$crate::storage::unhashed::put(&Self::key(), value);
 			}
 
 			/// Returns the value of this parameter type.
 			///
-			/// This needs to be executed in an externalities provided
-			/// environment.
+			/// This needs to be executed in an externalities provided environment.
 			#[allow(unused)]
 			pub fn get() -> $type {
 				$crate::storage::unhashed::get(&Self::key()).unwrap_or_else(|| $value)
diff --git a/frame/support/src/traits/hooks.rs b/frame/support/src/traits/hooks.rs
@@ -197,10 +197,10 @@ impl OnRuntimeUpgrade for Tuple {
 		weight
 	}
 
-	#[cfg(feature = "try-runtime")]
 	/// We are executing pre- and post-checks sequentially in order to be able to test several
 	/// consecutive migrations for the same pallet without errors. Therefore pre and post upgrade
 	/// hooks for tuples are a noop.
+	#[cfg(feature = "try-runtime")]
 	fn try_on_runtime_upgrade(checks: bool) -> Result<Weight, &'static str> {
 		let mut weight = Weight::zero();
 		for_tuples!( #( weight = weight.saturating_add(Tuple::try_on_runtime_upgrade(checks)?); )* );
@@ -364,10 +364,64 @@ pub trait OnTimestampSet<Moment> {
 #[cfg(test)]
 mod tests {
 	use super::*;
+	use sp_io::TestExternalities;
+
+	#[cfg(feature = "try-runtime")]
+	#[test]
+	fn on_runtime_upgrade_pre_post_executed_tuple() {
+		crate::parameter_types! {
+			pub static Pre: Vec<&'static str> = Default::default();
+			pub static Post: Vec<&'static str> = Default::default();
+		}
+
+		macro_rules! impl_test_type {
+			($name:ident) => {
+				struct $name;
+				impl OnRuntimeUpgrade for $name {
+					fn on_runtime_upgrade() -> Weight {
+						Default::default()
+					}
+
+					#[cfg(feature = "try-runtime")]
+					fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
+						Pre::mutate(|s| s.push(stringify!($name)));
+						Ok(Vec::new())
+					}
+
+					#[cfg(feature = "try-runtime")]
+					fn post_upgrade(_: Vec<u8>) -> Result<(), &'static str> {
+						Post::mutate(|s| s.push(stringify!($name)));
+						Ok(())
+					}
+				}
+			};
+		}
+
+		impl_test_type!(Foo);
+		impl_test_type!(Bar);
+		impl_test_type!(Baz);
+
+		TestExternalities::default().execute_with(|| {
+			Foo::try_on_runtime_upgrade(true).unwrap();
+			assert_eq!(Pre::take(), vec!["Foo"]);
+			assert_eq!(Post::take(), vec!["Foo"]);
+
+			<(Foo, Bar, Baz)>::try_on_runtime_upgrade(true).unwrap();
+			assert_eq!(Pre::take(), vec!["Foo", "Bar", "Baz"]);
+			assert_eq!(Post::take(), vec!["Foo", "Bar", "Baz"]);
+
+			<((Foo, Bar), Baz)>::try_on_runtime_upgrade(true).unwrap();
+			assert_eq!(Pre::take(), vec!["Foo", "Bar", "Baz"]);
+			assert_eq!(Post::take(), vec!["Foo", "Bar", "Baz"]);
+
+			<(Foo, (Bar, Baz))>::try_on_runtime_upgrade(true).unwrap();
+			assert_eq!(Pre::take(), vec!["Foo", "Bar", "Baz"]);
+			assert_eq!(Post::take(), vec!["Foo", "Bar", "Baz"]);
+		});
+	}
 
 	#[test]
 	fn on_initialize_and_on_runtime_upgrade_weight_merge_works() {
-		use sp_io::TestExternalities;
 		struct Test;
 
 		impl OnInitialize<u8> for Test {
diff --git a/test-utils/cli/Cargo.toml b/test-utils/cli/Cargo.toml
@@ -0,0 +1,23 @@
+[package]
+name = "substrate-cli-test-utils"
+description = "CLI testing utilities"
+version = "0.1.0"
+authors = ["Parity Technologies <admin@parity.io>"]
+edition = "2021"
+license = "Apache-2.0"
+homepage = "https://substrate.io"
+repository = "https://github.com/paritytech/substrate/"
+publish = false
+
+[package.metadata.docs.rs]
+targets = ["x86_64-unknown-linux-gnu"]
+
+[dependencies]
+substrate-rpc-client = { path = "../../utils/frame/rpc/client" }
+assert_cmd = "2.0.10"
+nix = "0.26.2"
+regex = "1.7.3"
+tempfile = "3.5.0"
+tokio = { version = "1.22.0", features = ["full"] }
+node-primitives = { path = "../../bin/node/primitives" }
+futures = "0.3.28"
diff --git a/test-utils/cli/src/lib.rs b/test-utils/cli/src/lib.rs
@@ -26,12 +26,146 @@ use nix::{
 use node_primitives::{Hash, Header};
 use regex::Regex;
 use std::{
+	env,
 	io::{BufRead, BufReader, Read},
 	ops::{Deref, DerefMut},
 	path::{Path, PathBuf},
 	process::{self, Child, Command},
 	time::Duration,
 };
+use tokio::io::{AsyncBufReadExt, AsyncRead};
+
+/// Starts a new Substrate node in development mode with a temporary chain.
+///
+/// This function creates a new Substrate node using the `substrate` binary.
+/// It configures the node to run in development mode (`--dev`) with a temporary chain (`--tmp`),
+/// sets the WebSocket port to 45789 (`--ws-port=45789`).
+///
+/// # Returns
+///
+/// A [`Child`] process representing the spawned Substrate node.
+///
+/// # Panics
+///
+/// This function will panic if the `substrate` binary is not found or if the node fails to start.
+///
+/// # Examples
+///
+/// ```ignore
+/// use my_crate::start_node;
+///
+/// let child = start_node();
+/// // Interact with the Substrate node using the WebSocket port 45789.
+/// // When done, the node will be killed when the `child` is dropped.
+/// ```
+///
+/// [`Child`]: std::process::Child
+pub fn start_node() -> Child {
+	Command::new(cargo_bin("substrate"))
+		.stdout(process::Stdio::piped())
+		.stderr(process::Stdio::piped())
+		.args(&["--dev", "--tmp", "--ws-port=45789", "--no-hardware-benchmarks"])
+		.spawn()
+		.unwrap()
+}
+
+/// Builds the Substrate project using the provided arguments.
+///
+/// This function reads the CARGO_MANIFEST_DIR environment variable to find the root workspace
+/// directory. It then runs the `cargo b` command in the root directory with the specified
+/// arguments.
+///
+/// This can be useful for building the Substrate binary with a desired set of features prior
+/// to using the binary in a CLI test.
+///
+/// # Arguments
+///
+/// * `args: &[&str]` - A slice of string references representing the arguments to pass to the
+///   `cargo b` command.
+///
+/// # Panics
+///
+/// This function will panic if:
+///
+/// * The CARGO_MANIFEST_DIR environment variable is not set.
+/// * The root workspace directory cannot be determined.
+/// * The 'cargo b' command fails to execute.
+/// * The 'cargo b' command returns a non-successful status.
+///
+/// # Examples
+///
+/// ```ignore
+/// build_substrate(&["--features=try-runtime"]);
+/// ```
+pub fn build_substrate(args: &[&str]) {
+	// Get the root workspace directory from the CARGO_MANIFEST_DIR environment variable
+	let manifest_dir = env::var("CARGO_MANIFEST_DIR").expect("CARGO_MANIFEST_DIR not set");
+	let root_dir = std::path::Path::new(&manifest_dir)
+		.parent()
+		.expect("Failed to find root workspace directory");
+	let output = Command::new("cargo")
+		.arg("build")
+		.args(args)
+		.current_dir(root_dir)
+		.output()
+		.expect(format!("Failed to execute 'cargo b' with args {:?}'", args).as_str());
+
+	if !output.status.success() {
+		panic!(
+			"Failed to execute 'cargo b' with args {:?}': \n{}",
+			args,
+			String::from_utf8_lossy(&output.stderr)
+		);
+	}
+}
+
+/// Takes a readable tokio stream (e.g. from a child process `ChildStderr` or `ChildStdout`) and
+/// a `Regex` pattern, and checks each line against the given pattern as it is produced.
+/// The function returns OK(()) as soon as a line matching the pattern is found, or an Err if
+/// the stream ends without any lines matching the pattern.
+///
+/// # Arguments
+///
+/// * `child_stream` - An async tokio stream, e.g. from a child process `ChildStderr` or
+///   `ChildStdout`.
+/// * `re` - A `Regex` pattern to search for in the stream.
+///
+/// # Returns
+///
+/// * `Ok(())` if a line matching the pattern is found.
+/// * `Err(String)` if the stream ends without any lines matching the pattern.
+///
+/// # Examples
+///
+/// ```ignore
+/// use regex::Regex;
+/// use tokio::process::Command;
+/// use tokio::io::AsyncRead;
+///
+/// # async fn run() {
+/// let child = Command::new("some-command").stderr(std::process::Stdio::piped()).spawn().unwrap();
+/// let stderr = child.stderr.unwrap();
+/// let re = Regex::new("error:").unwrap();
+///
+/// match wait_for_pattern_match_in_stream(stderr, re).await {
+///     Ok(()) => println!("Error found in stderr"),
+///     Err(e) => println!("Error: {}", e),
+/// }
+/// # }
+/// ```
+pub async fn wait_for_stream_pattern_match<R>(stream: R, re: Regex) -> Result<(), String>
+where
+	R: AsyncRead + Unpin,
+{
+	let mut stdio_reader = tokio::io::BufReader::new(stream).lines();
+	while let Ok(Some(line)) = stdio_reader.next_line().await {
+		match re.find(line.as_str()) {
+			Some(_) => return Ok(()),
+			None => (),
+		}
+	}
+	Err(String::from("Stream closed without any lines matching the regex."))
+}
 
 /// Run the given `future` and panic if the `timeout` is hit.
 pub async fn run_with_timeout(timeout: Duration, future: impl futures::Future<Output = ()>) {
diff --git a/utils/frame/try-runtime/cli/Cargo.toml b/utils/frame/try-runtime/cli/Cargo.toml
diff --git a/utils/frame/try-runtime/cli/src/commands/follow_chain.rs b/utils/frame/try-runtime/cli/src/commands/follow_chain.rs
diff --git a/utils/frame/try-runtime/cli/tests/follow_chain.rs b/utils/frame/try-runtime/cli/tests/follow_chain.rs
