Remove sleep from tests (#4639)

* Remove sleep and use polkadot test service

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* updates

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Fix other tests

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Run metrics tests separately

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* copy some substrate utilities

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* update runtime metric test

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Remove sleep from cli tests

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* cargo

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Polkadot companion for Substrate#10463 (#4519)

* Grandpa and Beefy protocol names include chain id

Signed-off-by: acatangiu <adrian@parity.io>

* chain_spec: include fork id

* use simplified protocol name

* fix after merge

* avoid using hash default, even for protocol names

* update lockfile for substrate

Co-authored-by: parity-processbot <>

* configuration: Update upgrade validation delay doc (#4662)

* typo

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* review feedback

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* cargo lock

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* use testnet profile

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Don't run with runtime-benchmark feature

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* conditional compile up one level

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

Co-authored-by: Sergei Shulepov <sergei@parity.io>

polkadot/Cargo.lock

@@ -4433,6 +4433,19 @@ dependencies = [
  "memoffset",
 ]
 
+[[package]]
+name = "node-primitives"
+version = "2.0.0"
+source = "git+https://github.com/paritytech/substrate?branch=master#c0f688d2c957d7a816b9abfad70d7d89a094996c"
+dependencies = [
+ "frame-system",
+ "parity-scale-codec",
+ "scale-info",
+ "sp-application-crypto",
+ "sp-core",
+ "sp-runtime",
+]
+
 [[package]]
 name = "nodrop"
 version = "0.1.14"
@@ -5898,9 +5911,12 @@ dependencies = [
  "assert_cmd",
  "color-eyre",
  "nix",
+ "node-primitives",
  "parity-util-mem",
  "polkadot-cli",
+ "remote-externalities",
  "tempfile",
+ "tokio",
 ]
 
 [[package]]
@@ -6542,10 +6558,15 @@ dependencies = [
  "nix",
  "parity-scale-codec",
  "polkadot-primitives",
+ "polkadot-test-service",
+ "prometheus-parse",
  "sc-cli",
+ "sc-client-api",
  "sc-service",
  "sc-tracing",
+ "sp-keyring",
  "substrate-prometheus-endpoint",
+ "substrate-test-utils",
  "tempfile",
  "tokio",
  "tracing",
@@ -7576,6 +7597,17 @@ dependencies = [
  "thiserror",
 ]
 
+[[package]]
+name = "prometheus-parse"
+version = "0.2.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "c996f3caea1c51aa034c0d2dfd8447a12c555f4567b02677ef8a865ac4cce712"
+dependencies = [
+ "chrono",
+ "lazy_static",
+ "regex",
+]
+
 [[package]]
 name = "prost"
 version = "0.9.0"

polkadot/Cargo.toml

@@ -21,6 +21,9 @@ parity-util-mem = { version = "*", default-features = false, features = ["jemall
 assert_cmd = "2.0.2"
 nix = "0.23.1"
 tempfile = "3.2.0"
+tokio = "1.15.0"
+remote-externalities = { git = "https://github.com/paritytech/substrate", branch = "master" }
+node-primitives = { git = "https://github.com/paritytech/substrate", branch = "master" }
 
 [workspace]
 members = [

polkadot/cli/Cargo.toml

@@ -52,7 +52,7 @@ cli = [
 	"polkadot-node-core-pvf",
 	"polkadot-performance-test",
 ]
-runtime-benchmarks = [ "service/runtime-benchmarks" ]
+runtime-benchmarks = [ "service/runtime-benchmarks", "polkadot-node-metrics/runtime-benchmarks" ]
 trie-memory-tracker = [ "sp-trie/memory-tracker" ]
 full-node = [ "service/full-node" ]
 try-runtime = [ "service/try-runtime" ]

polkadot/node/metrics/Cargo.toml

@@ -29,7 +29,14 @@ nix = "0.23.1"
 tempfile = "3.2.0"
 hyper = { version = "0.14.16", default-features = false, features = ["http1", "tcp"] }
 tokio = "1.13"
+polkadot-test-service = { path = "../test/service", features=["runtime-metrics"]}
+substrate-test-utils = { git = "https://github.com/paritytech/substrate", branch = "master" }
+sc-service = { git = "https://github.com/paritytech/substrate", branch = "master" }
+sp-keyring = { git = "https://github.com/paritytech/substrate", branch = "master" }
+sc-client-api = { git = "https://github.com/paritytech/substrate", branch = "master" }
+prometheus-parse = {version = "0.2.2"}
 
 [features]
 default = []
 runtime-metrics = []
+runtime-benchmarks = []

polkadot/node/metrics/src/lib.rs

@@ -81,5 +81,5 @@ pub mod metrics {
 	}
 }
 
-#[cfg(test)]
+#[cfg(all(feature = "runtime-metrics", not(feature = "runtime-benchmarks"), test))]
 mod tests;

polkadot/node/metrics/src/tests.rs

@@ -13,58 +13,95 @@
 
 // You should have received a copy of the GNU General Public License
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
-#![cfg(feature = "runtime-metrics")]
-use assert_cmd::cargo::cargo_bin;
-use std::{convert::TryInto, process::Command, thread, time::Duration};
-use tempfile::tempdir;
 
-#[test]
-#[cfg(unix)]
-fn runtime_can_publish_metrics() {
-	use hyper::{Client, Uri};
-	use nix::{
-		sys::signal::{kill, Signal::SIGINT},
-		unistd::Pid,
+//! Polkadot runtime metrics integration test.
+
+use hyper::{Client, Uri};
+use polkadot_test_service::{node_config, run_validator_node, test_prometheus_config};
+use primitives::v1::metric_definitions::PARACHAIN_INHERENT_DATA_BITFIELDS_PROCESSED;
+use sc_client_api::{execution_extensions::ExecutionStrategies, ExecutionStrategy};
+use sp_keyring::AccountKeyring::*;
+use std::{collections::HashMap, convert::TryFrom};
+
+const DEFAULT_PROMETHEUS_PORT: u16 = 9616;
+
+#[substrate_test_utils::test]
+async fn runtime_can_publish_metrics() {
+	let mut alice_config =
+		node_config(|| {}, tokio::runtime::Handle::current(), Alice, Vec::new(), true);
+
+	// Enable Prometheus metrics for Alice.
+	alice_config.prometheus_config = Some(test_prometheus_config(DEFAULT_PROMETHEUS_PORT));
+
+	let mut builder = sc_cli::LoggerBuilder::new("");
+
+	// Enable profiling with `wasm_tracing` target.
+	builder.with_profiling(Default::default(), String::from("wasm_tracing=trace"));
+
+	// Setup the runtime metrics provider.
+	crate::logger_hook()(&mut builder, &alice_config);
+
+	// Override default native strategy, runtime metrics are available only in the wasm runtime.
+	alice_config.execution_strategies = ExecutionStrategies {
+		syncing: ExecutionStrategy::AlwaysWasm,
+		importing: ExecutionStrategy::AlwaysWasm,
+		block_construction: ExecutionStrategy::AlwaysWasm,
+		offchain_worker: ExecutionStrategy::AlwaysWasm,
+		other: ExecutionStrategy::AlwaysWasm,
 	};
-	use std::convert::TryFrom;
 
-	const RUNTIME_METRIC_NAME: &str = "polkadot_parachain_inherent_data_bitfields_processed";
-	const DEFAULT_PROMETHEUS_PORT: u16 = 9615;
+	builder.init().expect("Failed to set up the logger");
+
+	// Start validator Alice.
+	let alice = run_validator_node(alice_config, None);
+
+	let bob_config =
+		node_config(|| {}, tokio::runtime::Handle::current(), Bob, vec![alice.addr.clone()], true);
+
+	// Start validator Bob.
+	let _bob = run_validator_node(bob_config, None);
+
+	// Wait for Alice to author two blocks.
+	alice.wait_for_blocks(2).await;
+
 	let metrics_uri = format!("http://localhost:{}/metrics", DEFAULT_PROMETHEUS_PORT);
+	let metrics = scrape_prometheus_metrics(&metrics_uri).await;
 
-	// Start the node with tracing enabled and forced wasm runtime execution.
-	let cmd = Command::new(cargo_bin("polkadot"))
-		// Runtime metrics require this trace target.
-		.args(&["--tracing-targets", "wasm_tracing=trace"])
-		.args(&["--execution", "wasm"])
-		.args(&["--dev", "-d"])
-		.arg(tempdir().expect("failed to create temp dir.").path())
-		.spawn()
-		.expect("failed to start the node process");
-
-	// Enough time to author one block.
-	thread::sleep(Duration::from_secs(30));
-
-	let runtime = tokio::runtime::Runtime::new().expect("failed to create tokio runtime");
-
-	runtime.block_on(async {
-		let client = Client::new();
-
-		let res = client
-			.get(Uri::try_from(&metrics_uri).expect("bad URI"))
-			.await
-			.expect("get request failed");
-
-		let body = String::from_utf8(
-			hyper::body::to_bytes(res).await.expect("can't get body as bytes").to_vec(),
-		)
-		.expect("body is not an UTF8 string");
-
-		// Time to die.
-		kill(Pid::from_raw(cmd.id().try_into().unwrap()), SIGINT)
-			.expect("failed to kill the node process");
-
-		// If the node has authored at least 1 block this should pass.
-		assert!(body.contains(&RUNTIME_METRIC_NAME));
-	});
+	// There should be at least 1 bitfield processed by now.
+	assert!(
+		*metrics
+			.get(&PARACHAIN_INHERENT_DATA_BITFIELDS_PROCESSED.name.to_owned())
+			.unwrap() > 1
+	);
+}
+
+async fn scrape_prometheus_metrics(metrics_uri: &str) -> HashMap<String, u64> {
+	let res = Client::new()
+		.get(Uri::try_from(metrics_uri).expect("bad URI"))
+		.await
+		.expect("GET request failed");
+
+	// Retrieve the `HTTP` response body.
+	let body = String::from_utf8(
+		hyper::body::to_bytes(res).await.expect("can't get body as bytes").to_vec(),
+	)
+	.expect("body is not an UTF8 string");
+
+	let lines: Vec<_> = body.lines().map(|s| Ok(s.to_owned())).collect();
+	prometheus_parse::Scrape::parse(lines.into_iter())
+		.expect("Scraper failed to parse Prometheus metrics")
+		.samples
+		.into_iter()
+		.map(|sample| {
+			(
+				sample.metric.to_owned(),
+				match sample.value {
+					prometheus_parse::Value::Counter(value) => value as u64,
+					prometheus_parse::Value::Gauge(value) => value as u64,
+					prometheus_parse::Value::Untyped(value) => value as u64,
+					_ => unreachable!("unexpected metric type"),
+				},
+			)
+		})
+		.collect()
 }

polkadot/node/test/service/Cargo.toml

@@ -62,3 +62,6 @@ pallet-balances = { git = "https://github.com/paritytech/substrate", branch = "m
 serde_json = "1.0.74"
 substrate-test-utils = { git = "https://github.com/paritytech/substrate", branch = "master" }
 tokio = { version = "1.15", features = ["macros"] }
+
+[features]
+runtime-metrics=["polkadot-test-runtime/runtime-metrics"]

polkadot/node/test/service/src/lib.rs

@@ -28,7 +28,9 @@ use polkadot_overseer::Handle;
 use polkadot_primitives::v1::{Balance, CollatorPair, HeadData, Id as ParaId, ValidationCode};
 use polkadot_runtime_common::BlockHashCount;
 use polkadot_runtime_parachains::paras::ParaGenesisArgs;
-use polkadot_service::{ClientHandle, Error, ExecuteWithClient, FullClient, IsCollator, NewFull};
+use polkadot_service::{
+	ClientHandle, Error, ExecuteWithClient, FullClient, IsCollator, NewFull, PrometheusConfig,
+};
 use polkadot_test_runtime::{
 	ParasSudoWrapperCall, Runtime, SignedExtra, SignedPayload, SudoCall, UncheckedExtrinsic,
 	VERSION,
@@ -48,7 +50,11 @@ use sp_blockchain::HeaderBackend;
 use sp_keyring::Sr25519Keyring;
 use sp_runtime::{codec::Encode, generic, traits::IdentifyAccount, MultiSigner};
 use sp_state_machine::BasicExternalities;
-use std::{path::PathBuf, sync::Arc};
+use std::{
+	net::{Ipv4Addr, SocketAddr},
+	path::PathBuf,
+	sync::Arc,
+};
 use substrate_test_client::{
 	BlockchainEventsExt, RpcHandlersExt, RpcTransactionError, RpcTransactionOutput,
 };
@@ -102,6 +108,14 @@ impl ClientHandle for TestClient {
 	}
 }
 
+/// Returns a prometheus config usable for testing.
+pub fn test_prometheus_config(port: u16) -> PrometheusConfig {
+	PrometheusConfig::new_with_default_registry(
+		SocketAddr::new(Ipv4Addr::LOCALHOST.into(), port),
+		"test-chain".to_string(),
+	)
+}
+
 /// Create a Polkadot `Configuration`.
 ///
 /// By default an in-memory socket will be used, therefore you need to provide boot
@@ -160,7 +174,7 @@ pub fn node_config(
 		keep_blocks: KeepBlocks::All,
 		transaction_storage: TransactionStorageMode::BlockBody,
 		chain_spec: Box::new(spec),
-		wasm_method: WasmExecutionMethod::Interpreted,
+		wasm_method: WasmExecutionMethod::Compiled,
 		wasm_runtime_overrides: Default::default(),
 		// NOTE: we enforce the use of the native runtime to make the errors more debuggable
 		execution_strategies: ExecutionStrategies {
@@ -195,21 +209,11 @@ pub fn node_config(
 	}
 }
 
-/// Run a test validator node that uses the test runtime.
-///
-/// The node will be using an in-memory socket, therefore you need to provide boot nodes if you
-/// want it to be connected to other nodes.
-///
-/// The `storage_update_func` function will be executed in an externalities provided environment
-/// and can be used to make adjustments to the runtime genesis storage.
+/// Run a test validator node that uses the test runtime and specified `config`.
 pub fn run_validator_node(
-	tokio_handle: tokio::runtime::Handle,
-	key: Sr25519Keyring,
-	storage_update_func: impl Fn(),
-	boot_nodes: Vec<MultiaddrWithPeerId>,
+	config: Configuration,
 	worker_program_path: Option<PathBuf>,
 ) -> PolkadotTestNode {
-	let config = node_config(storage_update_func, tokio_handle, key, boot_nodes, true);
 	let multiaddr = config.network.listen_addresses[0].clone();
 	let NewFull { task_manager, client, network, rpc_handlers, overseer_handle, .. } =
 		new_full(config, IsCollator::No, worker_program_path)

polkadot/node/test/service/tests/build-blocks.rs

@@ -23,21 +23,23 @@ async fn ensure_test_service_build_blocks() {
 	let mut builder = sc_cli::LoggerBuilder::new("");
 	builder.with_colors(false);
 	builder.init().expect("Sets up logger");
-
-	let mut alice = run_validator_node(
+	let alice_config = node_config(
+		|| {},
 		tokio::runtime::Handle::current(),
 		Sr25519Keyring::Alice,
-		|| {},
 		Vec::new(),
-		None,
+		true,
 	);
-	let mut bob = run_validator_node(
+	let mut alice = run_validator_node(alice_config, None);
+
+	let bob_config = node_config(
+		|| {},
 		tokio::runtime::Handle::current(),
 		Sr25519Keyring::Bob,
-		|| {},
 		vec![alice.addr.clone()],
-		None,
+		true,
 	);
+	let mut bob = run_validator_node(bob_config, None);
 
 	{
 		let t1 = future::join(alice.wait_for_blocks(3), bob.wait_for_blocks(3)).fuse();

polkadot/node/test/service/tests/call-function.rs

@@ -19,8 +19,10 @@ use sp_keyring::Sr25519Keyring::{Alice, Bob, Charlie};
 
 #[substrate_test_utils::test]
 async fn call_function_actually_work() {
-	let alice =
-		run_validator_node(tokio::runtime::Handle::current(), Alice, || {}, Vec::new(), None);
+	let alice_config =
+		node_config(|| {}, tokio::runtime::Handle::current(), Alice, Vec::new(), true);
+
+	let alice = run_validator_node(alice_config, None);
 
 	let function = polkadot_test_runtime::Call::Balances(pallet_balances::Call::transfer {
 		dest: Charlie.to_account_id().into(),

polkadot/parachain/test-parachains/adder/collator/tests/integration.rs

@@ -31,23 +31,27 @@ async fn collating_using_adder_collator() {
 
 	let para_id = ParaId::from(100);
 
-	// start alice
-	let alice = polkadot_test_service::run_validator_node(
+	let alice_config = polkadot_test_service::node_config(
+		|| {},
 		tokio::runtime::Handle::current(),
 		Alice,
+		Vec::new(),
+		true,
+	);
+
+	// start alice
+	let alice = polkadot_test_service::run_validator_node(alice_config, Some(PUPPET_EXE.into()));
+
+	let bob_config = polkadot_test_service::node_config(
 		|| {},
-		vec![],
-		Some(PUPPET_EXE.into()),
+		tokio::runtime::Handle::current(),
+		Bob,
+		vec![alice.addr.clone()],
+		true,
 	);
 
 	// start bob
-	let bob = polkadot_test_service::run_validator_node(
-		tokio::runtime::Handle::current(),
-		Bob,
-		|| {},
-		vec![alice.addr.clone()],
-		Some(PUPPET_EXE.into()),
-	);
+	let bob = polkadot_test_service::run_validator_node(bob_config, Some(PUPPET_EXE.into()));
 
 	let collator = test_parachain_adder_collator::Collator::new();
 

polkadot/runtime/test-runtime/Cargo.toml

@@ -78,6 +78,8 @@ substrate-wasm-builder = { git = "https://github.com/paritytech/substrate", bran
 default = ["std"]
 no_std = []
 only-staking = []
+runtime-metrics = ["polkadot-runtime-parachains/runtime-metrics", "sp-io/with-tracing"]
+
 std = [
 	"authority-discovery-primitives/std",
 	"pallet-authority-discovery/std",

polkadot/scripts/gitlab/test_linux_stable.sh

@@ -6,3 +6,8 @@ source "$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )/../com
 
 # Builds with the runtime benchmarks/metrics features are only to be used for testing.
 time cargo test --workspace --profile testnet --verbose --locked --features=runtime-benchmarks,runtime-metrics
+
+# We need to separately run the `polkadot-node-metrics` tests. More specifically, because the 
+# `runtime_can_publish_metrics` test uses the `test-runtime` which doesn't support
+# the `runtime-benchmarks` feature.
+time cargo test  --profile testnet --verbose --locked --features=runtime-metrics -p polkadot-node-metrics

polkadot/tests/common.rs

@@ -14,11 +14,16 @@
 // You should have received a copy of the GNU General Public License
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
+use node_primitives::Block;
+use remote_externalities::rpc_api::get_finalized_head;
 use std::{
 	process::{Child, 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 ammount of `secs`.
 ///
@@ -36,3 +41,27 @@ pub fn wait_for(child: &mut Child, secs: usize) -> Option<ExitStatus> {
 
 	None
 }
+
+/// Wait for at least `n` blocks to be finalized within the specified time.
+pub async fn wait_n_finalized_blocks(
+	n: usize,
+	timeout_duration: Duration,
+) -> Result<(), tokio::time::error::Elapsed> {
+	timeout(timeout_duration, wait_n_finalized_blocks_from(n, LOCALHOST_WS)).await
+}
+
+/// Wait for at least `n` blocks to be finalized from a specified node.
+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(6));
+
+	loop {
+		if let Ok(block) = get_finalized_head::<Block, _>(url.to_string()).await {
+			built_blocks.insert(block);
+			if built_blocks.len() > n {
+				break
+			}
+		};
+		interval.tick().await;
+	}
+}

polkadot/tests/purge_chain_works.rs

@@ -15,14 +15,14 @@
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
 use assert_cmd::cargo::cargo_bin;
-use std::{convert::TryInto, process::Command, thread, time::Duration};
+use std::{convert::TryInto, process::Command, time::Duration};
 use tempfile::tempdir;
 
-mod common;
+pub mod common;
 
-#[test]
+#[tokio::test]
 #[cfg(unix)]
-fn purge_chain_works() {
+async fn purge_chain_works() {
 	use nix::{
 		sys::signal::{kill, Signal::SIGINT},
 		unistd::Pid,
@@ -36,15 +36,12 @@ fn purge_chain_works() {
 		.spawn()
 		.unwrap();
 
-	// Let it produce some blocks.
-	// poll once per second for faster failure
-	for _ in 0..30 {
-		thread::sleep(Duration::from_secs(1));
-		assert!(cmd.try_wait().unwrap().is_none(), "the process should still be running");
-	}
+	// Let it produce 1 block.
+	common::wait_n_finalized_blocks(1, Duration::from_secs(60)).await.unwrap();
 
-	// Stop the process
+	// Send SIGINT to node.
 	kill(Pid::from_raw(cmd.id().try_into().unwrap()), SIGINT).unwrap();
+	// Wait for the node to handle it and exit.
 	assert!(common::wait_for(&mut cmd, 30).map(|x| x.success()).unwrap_or_default());
 
 	// Purge chain

polkadot/tests/running_the_node_and_interrupt.rs

@@ -15,14 +15,14 @@
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
 use assert_cmd::cargo::cargo_bin;
-use std::{convert::TryInto, process::Command, thread, time::Duration};
+use std::{convert::TryInto, process::Command, time::Duration};
 use tempfile::tempdir;
 
-mod common;
+pub mod common;
 
-#[test]
+#[tokio::test]
 #[cfg(unix)]
-fn running_the_node_works_and_can_be_interrupted() {
+async fn running_the_node_works_and_can_be_interrupted() {
 	use nix::{
 		sys::signal::{
 			kill,
@@ -31,7 +31,7 @@ fn running_the_node_works_and_can_be_interrupted() {
 		unistd::Pid,
 	};
 
-	fn run_command_and_kill(signal: Signal) {
+	async fn run_command_and_kill(signal: Signal) {
 		let tmpdir = tempdir().expect("coult not create temp dir");
 
 		let mut cmd = Command::new(cargo_bin("polkadot"))
@@ -40,7 +40,9 @@ fn running_the_node_works_and_can_be_interrupted() {
 			.spawn()
 			.unwrap();
 
-		thread::sleep(Duration::from_secs(30));
+		// Let it produce three blocks.
+		common::wait_n_finalized_blocks(3, Duration::from_secs(60)).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!(
@@ -51,6 +53,6 @@ fn running_the_node_works_and_can_be_interrupted() {
 		);
 	}
 
-	run_command_and_kill(SIGINT);
-	run_command_and_kill(SIGTERM);
+	run_command_and_kill(SIGINT).await;
+	run_command_and_kill(SIGTERM).await;
 }