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

liamaharon · kianenigma · koute · breathx · commit 91eb29d454b1 · 2023-04-22T16:57:57.000+03: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
@@ -19,8 +19,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
@@ -18,17 +18,15 @@
 
 #![cfg(unix)]
 use assert_cmd::cargo::cargo_bin;
-use nix::{
-	sys::signal::{
-		kill,
-		Signal::{self, SIGINT, SIGTERM},
-	},
-	unistd::Pid,
+use nix::sys::signal::Signal::{self, SIGINT, SIGTERM};
+use std::{
+	process::{self, Command},
+	time::Duration,
 };
 use std::process::{self, Child, Command};
 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() {
@@ -75,17 +73,9 @@ async fn running_the_node_works_and_can_be_interrupted() {
 
 #[tokio::test]
 async fn running_two_nodes_with_the_same_ws_port_should_work() {
-	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());
+	common::run_with_timeout(Duration::from_secs(60 * 10), async move {
+		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::find_ws_url_from_output(stderr);
diff --git a/bin/node/cli/tests/telemetry.rs b/bin/node/cli/tests/telemetry.rs
@@ -23,7 +23,7 @@ use nix::{
 };
 use std::process;
 
-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
@@ -30,7 +30,7 @@ use std::{
 	process::{Command, Stdio},
 };
 
-pub mod common;
+use substrate_cli_test_utils as common;
 
 #[tokio::test]
 async fn temp_base_path_works() {
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)?); )* );
@@ -359,10 +359,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
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
