Less sleeps (#9848)

* Less sleeps

* No need to use tokio-test crate

* Less sleep

* Avoid leaving zombie substrates around

(when panicing in tests)

* Remove unused imports

* Incorporating feedback

* rename method

* Use rpc_api

* Update bin/node/cli/tests/temp_base_path_works.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* Update bin/node/cli/tests/temp_base_path_works.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

substrate/bin/node/cli/tests/check_block_works.rs

@@ -24,11 +24,11 @@ use tempfile::tempdir;
 
 pub mod common;
 
-#[test]
-fn check_block_works() {
+#[tokio::test]
+async fn check_block_works() {
 	let base_path = tempdir().expect("could not create a temp dir");
 
-	common::run_node_for_a_while(base_path.path(), &["--dev"]);
+	common::run_node_for_a_while(base_path.path(), &["--dev"]).await;
 
 	let status = Command::new(cargo_bin("substrate"))
 		.args(&["check-block", "--dev", "--pruning", "archive", "-d"])

substrate/bin/node/cli/tests/common.rs

@@ -23,58 +23,115 @@ use nix::{
 	sys::signal::{kill, Signal::SIGINT},
 	unistd::Pid,
 };
+use node_primitives::Block;
+use remote_externalities::rpc_api;
 use std::{
 	convert::TryInto,
+	ops::{Deref, DerefMut},
 	path::Path,
 	process::{Child, Command, ExitStatus},
-	thread,
 	time::Duration,
 };
+use tokio::time::timeout;
+
+static LOCALHOST_WS: &str = "ws://127.0.0.1:9944/";
 
 /// Wait for the given `child` the given number of `secs`.
 ///
 /// Returns the `Some(exit status)` or `None` if the process did not finish in the given time.
-pub fn wait_for(child: &mut Child, secs: usize) -> Option<ExitStatus> {
-	for i in 0..secs {
-		match child.try_wait().unwrap() {
-			Some(status) => {
-				if i > 5 {
-					eprintln!("Child process took {} seconds to exit gracefully", i);
-				}
-				return Some(status)
-			},
-			None => thread::sleep(Duration::from_secs(1)),
+pub fn wait_for(child: &mut Child, secs: u64) -> Result<ExitStatus, ()> {
+	let result = wait_timeout::ChildExt::wait_timeout(child, Duration::from_secs(5.min(secs)))
+		.map_err(|_| ())?;
+	if let Some(exit_status) = result {
+		Ok(exit_status)
+	} else {
+		if secs > 5 {
+			eprintln!("Child process taking over 5 seconds to exit gracefully");
+			let result = wait_timeout::ChildExt::wait_timeout(child, Duration::from_secs(secs - 5))
+				.map_err(|_| ())?;
+			if let Some(exit_status) = result {
+				return Ok(exit_status)
+			}
 		}
+		eprintln!("Took too long to exit (> {} seconds). Killing...", secs);
+		let _ = child.kill();
+		child.wait().unwrap();
+		Err(())
 	}
-	eprintln!("Took too long to exit (> {} seconds). Killing...", secs);
-	let _ = child.kill();
-	child.wait().unwrap();
-
-	None
 }
 
-/// Run the node for a while (30 seconds)
-pub fn run_node_for_a_while(base_path: &Path, args: &[&str]) {
+/// Wait for at least n blocks to be finalized within a specified time.
+pub async fn wait_n_finalized_blocks(
+	n: usize,
+	timeout_secs: u64,
+) -> Result<(), tokio::time::error::Elapsed> {
+	timeout(Duration::from_secs(timeout_secs), wait_n_finalized_blocks_from(n, LOCALHOST_WS)).await
+}
+
+/// Wait for at least n blocks to be finalized from a specified node
+pub async fn wait_n_finalized_blocks_from(n: usize, url: &str) {
+	let mut built_blocks = std::collections::HashSet::new();
+	let mut interval = tokio::time::interval(Duration::from_secs(2));
+
+	loop {
+		if let Ok(block) = rpc_api::get_finalized_head::<Block, _>(url.to_string()).await {
+			built_blocks.insert(block);
+			if built_blocks.len() > n {
+				break
+			}
+		};
+		interval.tick().await;
+	}
+}
+
+/// Run the node for a while (3 blocks)
+pub async fn run_node_for_a_while(base_path: &Path, args: &[&str]) {
 	let mut cmd = Command::new(cargo_bin("substrate"));
 
-	let mut cmd = cmd.args(args).arg("-d").arg(base_path).spawn().unwrap();
+	let mut child = KillChildOnDrop(cmd.args(args).arg("-d").arg(base_path).spawn().unwrap());
 
 	// Let it produce some blocks.
-	thread::sleep(Duration::from_secs(30));
-	assert!(cmd.try_wait().unwrap().is_none(), "the process should still be running");
+	let _ = wait_n_finalized_blocks(3, 30).await;
+
+	assert!(child.try_wait().unwrap().is_none(), "the process should still be running");
 
 	// Stop the process
-	kill(Pid::from_raw(cmd.id().try_into().unwrap()), SIGINT).unwrap();
-	assert!(wait_for(&mut cmd, 40).map(|x| x.success()).unwrap_or_default());
+	kill(Pid::from_raw(child.id().try_into().unwrap()), SIGINT).unwrap();
+	assert!(wait_for(&mut child, 40).map(|x| x.success()).unwrap());
 }
 
 /// Run the node asserting that it fails with an error
 pub fn run_node_assert_fail(base_path: &Path, args: &[&str]) {
 	let mut cmd = Command::new(cargo_bin("substrate"));
 
-	let mut cmd = cmd.args(args).arg("-d").arg(base_path).spawn().unwrap();
+	let mut child = KillChildOnDrop(cmd.args(args).arg("-d").arg(base_path).spawn().unwrap());
 
-	// Let it produce some blocks.
-	thread::sleep(Duration::from_secs(10));
-	assert!(cmd.try_wait().unwrap().is_some(), "the process should not be running anymore");
+	// Let it produce some blocks, but it should die within 10 seconds.
+	assert_ne!(
+		wait_timeout::ChildExt::wait_timeout(&mut *child, Duration::from_secs(10)).unwrap(),
+		None,
+		"the process should not be running anymore"
+	);
+}
+
+pub struct KillChildOnDrop(pub Child);
+
+impl Drop for KillChildOnDrop {
+	fn drop(&mut self) {
+		let _ = self.0.kill();
+	}
+}
+
+impl Deref for KillChildOnDrop {
+	type Target = Child;
+
+	fn deref(&self) -> &Self::Target {
+		&self.0
+	}
+}
+
+impl DerefMut for KillChildOnDrop {
+	fn deref_mut(&mut self) -> &mut Self::Target {
+		&mut self.0
+	}
 }

substrate/bin/node/cli/tests/database_role_subdir_migration.rs

@@ -25,9 +25,9 @@ use tempfile::tempdir;
 
 pub mod common;
 
-#[test]
+#[tokio::test]
 #[cfg(unix)]
-fn database_role_subdir_migration() {
+async fn database_role_subdir_migration() {
 	type Block = RawBlock<ExtrinsicWrapper<u64>>;
 
 	let base_path = tempdir().expect("could not create a temp dir");
@@ -62,7 +62,8 @@ fn database_role_subdir_migration() {
 			"44445",
 			"--no-prometheus",
 		],
-	);
+	)
+	.await;
 
 	// check if the database dir had been migrated
 	assert!(!path.join("db_version").exists());

substrate/bin/node/cli/tests/export_import_flow.rs

@@ -182,13 +182,13 @@ impl<'a> ExportImportRevertExecutor<'a> {
 	}
 }
 
-#[test]
-fn export_import_revert() {
+#[tokio::test]
+async fn export_import_revert() {
 	let base_path = tempdir().expect("could not create a temp dir");
 	let exported_blocks_file = base_path.path().join("exported_blocks");
 	let db_path = base_path.path().join("db");
 
-	common::run_node_for_a_while(base_path.path(), &["--dev"]);
+	common::run_node_for_a_while(base_path.path(), &["--dev"]).await;
 
 	let mut executor = ExportImportRevertExecutor::new(&base_path, &exported_blocks_file, &db_path);
 

substrate/bin/node/cli/tests/inspect_works.rs

@@ -24,11 +24,11 @@ use tempfile::tempdir;
 
 pub mod common;
 
-#[test]
-fn inspect_works() {
+#[tokio::test]
+async fn inspect_works() {
 	let base_path = tempdir().expect("could not create a temp dir");
 
-	common::run_node_for_a_while(base_path.path(), &["--dev"]);
+	common::run_node_for_a_while(base_path.path(), &["--dev"]).await;
 
 	let status = Command::new(cargo_bin("substrate"))
 		.args(&["inspect", "--dev", "--pruning", "archive", "-d"])

substrate/bin/node/cli/tests/purge_chain_works.rs

@@ -22,12 +22,12 @@ use tempfile::tempdir;
 
 pub mod common;
 
-#[test]
+#[tokio::test]
 #[cfg(unix)]
-fn purge_chain_works() {
+async fn purge_chain_works() {
 	let base_path = tempdir().expect("could not create a temp dir");
 
-	common::run_node_for_a_while(base_path.path(), &["--dev"]);
+	common::run_node_for_a_while(base_path.path(), &["--dev"]).await;
 
 	let status = Command::new(cargo_bin("substrate"))
 		.args(&["purge-chain", "--dev", "-d"])

substrate/bin/node/cli/tests/running_the_node_and_interrupt.rs

@@ -17,7 +17,6 @@
 // along with this program. If not, see <https://www.gnu.org/licenses/>.
 
 #![cfg(unix)]
-
 use assert_cmd::cargo::cargo_bin;
 use nix::{
 	sys::signal::{
@@ -26,67 +25,43 @@ use nix::{
 	},
 	unistd::Pid,
 };
-use sc_service::Deref;
 use std::{
 	convert::TryInto,
-	ops::DerefMut,
 	process::{Child, Command},
-	thread,
-	time::Duration,
 };
 use tempfile::tempdir;
 
 pub mod common;
 
-#[test]
-fn running_the_node_works_and_can_be_interrupted() {
-	fn run_command_and_kill(signal: Signal) {
+#[tokio::test]
+async fn running_the_node_works_and_can_be_interrupted() {
+	async fn run_command_and_kill(signal: Signal) {
 		let base_path = tempdir().expect("could not create a temp dir");
-		let mut cmd = Command::new(cargo_bin("substrate"))
-			.args(&["--dev", "-d"])
-			.arg(base_path.path())
-			.spawn()
-			.unwrap();
+		let mut cmd = common::KillChildOnDrop(
+			Command::new(cargo_bin("substrate"))
+				.args(&["--dev", "-d"])
+				.arg(base_path.path())
+				.spawn()
+				.unwrap(),
+		);
 
-		thread::sleep(Duration::from_secs(20));
+		common::wait_n_finalized_blocks(3, 30).await.unwrap();
 		assert!(cmd.try_wait().unwrap().is_none(), "the process should still be running");
 		kill(Pid::from_raw(cmd.id().try_into().unwrap()), signal).unwrap();
 		assert_eq!(
 			common::wait_for(&mut cmd, 30).map(|x| x.success()),
-			Some(true),
+			Ok(true),
 			"the process must exit gracefully after signal {}",
 			signal,
 		);
 	}
 
-	run_command_and_kill(SIGINT);
-	run_command_and_kill(SIGTERM);
+	run_command_and_kill(SIGINT).await;
+	run_command_and_kill(SIGTERM).await;
 }
 
-struct KillChildOnDrop(Child);
-
-impl Drop for KillChildOnDrop {
-	fn drop(&mut self) {
-		let _ = self.0.kill();
-	}
-}
-
-impl Deref for KillChildOnDrop {
-	type Target = Child;
-
-	fn deref(&self) -> &Self::Target {
-		&self.0
-	}
-}
-
-impl DerefMut for KillChildOnDrop {
-	fn deref_mut(&mut self) -> &mut Self::Target {
-		&mut self.0
-	}
-}
-
-#[test]
-fn running_two_nodes_with_the_same_ws_port_should_work() {
+#[tokio::test]
+async fn running_two_nodes_with_the_same_ws_port_should_work() {
 	fn start_node() -> Child {
 		Command::new(cargo_bin("substrate"))
 			.args(&["--dev", "--tmp", "--ws-port=45789"])
@@ -94,10 +69,10 @@ fn running_two_nodes_with_the_same_ws_port_should_work() {
 			.unwrap()
 	}
 
-	let mut first_node = KillChildOnDrop(start_node());
-	let mut second_node = KillChildOnDrop(start_node());
+	let mut first_node = common::KillChildOnDrop(start_node());
+	let mut second_node = common::KillChildOnDrop(start_node());
 
-	thread::sleep(Duration::from_secs(30));
+	let _ = common::wait_n_finalized_blocks(3, 30).await;
 
 	assert!(first_node.try_wait().unwrap().is_none(), "The first node should still be running");
 	assert!(second_node.try_wait().unwrap().is_none(), "The second node should still be running");
@@ -107,12 +82,12 @@ fn running_two_nodes_with_the_same_ws_port_should_work() {
 
 	assert_eq!(
 		common::wait_for(&mut first_node, 30).map(|x| x.success()),
-		Some(true),
+		Ok(true),
 		"The first node must exit gracefully",
 	);
 	assert_eq!(
 		common::wait_for(&mut second_node, 30).map(|x| x.success()),
-		Some(true),
+		Ok(true),
 		"The second node must exit gracefully",
 	);
 }

substrate/bin/node/cli/tests/temp_base_path_works.rs

@@ -29,37 +29,34 @@ use std::{
 	io::Read,
 	path::PathBuf,
 	process::{Command, Stdio},
-	thread,
-	time::Duration,
 };
 
 pub mod common;
 
-#[test]
-fn temp_base_path_works() {
+#[tokio::test]
+async fn temp_base_path_works() {
 	let mut cmd = Command::new(cargo_bin("substrate"));
-
-	let mut cmd = cmd
-		.args(&["--dev", "--tmp"])
-		.stdout(Stdio::piped())
-		.stderr(Stdio::piped())
-		.spawn()
-		.unwrap();
+	let mut child = common::KillChildOnDrop(
+		cmd.args(&["--dev", "--tmp"])
+			.stdout(Stdio::piped())
+			.stderr(Stdio::piped())
+			.spawn()
+			.unwrap(),
+	);
 
 	// Let it produce some blocks.
-	thread::sleep(Duration::from_secs(30));
-	assert!(cmd.try_wait().unwrap().is_none(), "the process should still be running");
+	common::wait_n_finalized_blocks(3, 30).await.unwrap();
+	assert!(child.try_wait().unwrap().is_none(), "the process should still be running");
 
 	// Stop the process
-	kill(Pid::from_raw(cmd.id().try_into().unwrap()), SIGINT).unwrap();
-	assert!(common::wait_for(&mut cmd, 40).map(|x| x.success()).unwrap_or_default());
+	kill(Pid::from_raw(child.id().try_into().unwrap()), SIGINT).unwrap();
+	assert!(common::wait_for(&mut child, 40).map(|x| x.success()).unwrap_or_default());
 
 	// Ensure the database has been deleted
 	let mut stderr = String::new();
-	cmd.stderr.unwrap().read_to_string(&mut stderr).unwrap();
+	child.stderr.as_mut().unwrap().read_to_string(&mut stderr).unwrap();
 	let re = Regex::new(r"Database: .+ at (\S+)").unwrap();
-	let db_path =
-		PathBuf::from(re.captures(stderr.as_str()).unwrap().get(1).unwrap().as_str().to_string());
+	let db_path = PathBuf::from(re.captures(stderr.as_str()).unwrap().get(1).unwrap().as_str());
 
 	assert!(!db_path.exists());
 }