Fix: CI failing for some CLI tests (#5043)

* Initial commit

Forked at: ad90ab7ec9
Parent branch: origin/master

* Increase killing grace period of CLI tests and display more info

* Use --dev everywhere possible

* Put pruning mode to its own params struct

* Add pruning params to export-blocks command

* Added missing file

* Removed not-dev mode in tests

* Add pruning mode to the revert command

* Decrease killing grace period again

* Move back unsafe_pruning to import_params

* Applied proposed changes

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

@@ -26,10 +26,10 @@ mod common;
 fn check_block_works() {
 	let base_path = tempdir().expect("could not create a temp dir");
 
-	common::run_command_for_a_while(base_path.path(), false);
+	common::run_dev_node_for_a_while(base_path.path());
 
 	let status = Command::new(cargo_bin("substrate"))
-		.args(&["check-block", "-d"])
+		.args(&["check-block", "--dev", "--pruning", "archive", "-d"])
 		.arg(base_path.path())
 		.arg("1")
 		.status()

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

@@ -27,13 +27,18 @@ use nix::unistd::Pid;
 ///
 /// 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 _ in 0..secs {
+	for i in 0..secs {
 		match child.try_wait().unwrap() {
-			Some(status) => return Some(status),
+			Some(status) => {
+				if i > 5 {
+					eprintln!("Child process took {} seconds to exit gracefully", i);
+				}
+				return Some(status)
+			},
 			None => thread::sleep(Duration::from_secs(1)),
 		}
 	}
-	eprintln!("Took to long to exit. Killing...");
+	eprintln!("Took too long to exit (> {} seconds). Killing...", secs);
 	let _ = child.kill();
 	child.wait().unwrap();
 
@@ -41,14 +46,11 @@ pub fn wait_for(child: &mut Child, secs: usize) -> Option<ExitStatus> {
 }
 
 /// Run the node for a while (30 seconds)
-pub fn run_command_for_a_while(base_path: &Path, dev: bool) {
+pub fn run_dev_node_for_a_while(base_path: &Path) {
 	let mut cmd = Command::new(cargo_bin("substrate"));
 
-	if dev {
-		cmd.arg("--dev");
-	}
-
 	let mut cmd = cmd
+		.args(&["--dev"])
 		.arg("-d")
 		.arg(base_path)
 		.spawn()

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

@@ -27,10 +27,10 @@ fn import_export_and_revert_work() {
 	let base_path = tempdir().expect("could not create a temp dir");
 	let exported_blocks = base_path.path().join("exported_blocks");
 
-	common::run_command_for_a_while(base_path.path(), false);
+	common::run_dev_node_for_a_while(base_path.path());
 
 	let status = Command::new(cargo_bin("substrate"))
-		.args(&["export-blocks", "-d"])
+		.args(&["export-blocks", "--dev", "--pruning", "archive", "-d"])
 		.arg(base_path.path())
 		.arg(&exported_blocks)
 		.status()
@@ -43,7 +43,7 @@ fn import_export_and_revert_work() {
 	let _ = fs::remove_dir_all(base_path.path().join("db"));
 
 	let status = Command::new(cargo_bin("substrate"))
-		.args(&["import-blocks", "-d"])
+		.args(&["import-blocks", "--dev", "--pruning", "archive", "-d"])
 		.arg(base_path.path())
 		.arg(&exported_blocks)
 		.status()
@@ -51,7 +51,7 @@ fn import_export_and_revert_work() {
 	assert!(status.success());
 
 	let status = Command::new(cargo_bin("substrate"))
-		.args(&["revert", "-d"])
+		.args(&["revert", "--dev", "--pruning", "archive", "-d"])
 		.arg(base_path.path())
 		.status()
 		.unwrap();

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

@@ -26,10 +26,10 @@ mod common;
 fn inspect_works() {
 	let base_path = tempdir().expect("could not create a temp dir");
 
-	common::run_command_for_a_while(base_path.path(), false);
+	common::run_dev_node_for_a_while(base_path.path());
 
 	let status = Command::new(cargo_bin("substrate"))
-		.args(&["inspect", "-d"])
+		.args(&["inspect", "--dev", "--pruning", "archive", "-d"])
 		.arg(base_path.path())
 		.args(&["block", "1"])
 		.status()

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

@@ -25,7 +25,7 @@ mod common;
 fn purge_chain_works() {
 	let base_path = tempdir().expect("could not create a temp dir");
 
-	common::run_command_for_a_while(base_path.path(), true);
+	common::run_dev_node_for_a_while(base_path.path());
 
 	let status = Command::new(cargo_bin("substrate"))
 		.args(&["purge-chain", "--dev", "-d"])

substrate/client/cli/src/commands/export_blocks_cmd.rs

@@ -22,15 +22,14 @@ use log::info;
 use structopt::StructOpt;
 use sc_service::{
 	Configuration, ChainSpecExtension, RuntimeGenesis, ServiceBuilderCommand, ChainSpec,
-	config::DatabaseConfig,
+	config::DatabaseConfig, Roles,
 };
 use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
 
 use crate::error;
 use crate::VersionInfo;
 use crate::runtime::run_until_exit;
-use crate::params::SharedParams;
-use crate::params::BlockNumber;
+use crate::params::{SharedParams, BlockNumber, PruningParams};
 
 /// The `export-blocks` command used to export blocks.
 #[derive(Debug, StructOpt, Clone)]
@@ -58,6 +57,10 @@ pub struct ExportBlocksCmd {
 	#[allow(missing_docs)]
 	#[structopt(flatten)]
 	pub shared_params: SharedParams,
+
+	#[allow(missing_docs)]
+	#[structopt(flatten)]
+	pub pruning_params: PruningParams,
 }
 
 impl ExportBlocksCmd {
@@ -106,6 +109,7 @@ impl ExportBlocksCmd {
 		F: FnOnce(&str) -> Result<Option<ChainSpec<G, E>>, String>,
 	{
 		self.shared_params.update_config(&mut config, spec_factory, version)?;
+		self.pruning_params.update_config(&mut config, Roles::FULL, true)?;
 		config.use_in_memory_keystore()?;
 
 		Ok(())

substrate/client/cli/src/commands/revert_cmd.rs

@@ -17,14 +17,13 @@
 use std::fmt::Debug;
 use structopt::StructOpt;
 use sc_service::{
-	Configuration, ChainSpecExtension, RuntimeGenesis, ServiceBuilderCommand, ChainSpec,
+	Configuration, ChainSpecExtension, RuntimeGenesis, ServiceBuilderCommand, ChainSpec, Roles,
 };
 use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
 
 use crate::error;
 use crate::VersionInfo;
-use crate::params::BlockNumber;
-use crate::params::SharedParams;
+use crate::params::{BlockNumber, SharedParams, PruningParams};
 
 /// The `revert` command used revert the chain to a previous state.
 #[derive(Debug, StructOpt, Clone)]
@@ -36,6 +35,10 @@ pub struct RevertCmd {
 	#[allow(missing_docs)]
 	#[structopt(flatten)]
 	pub shared_params: SharedParams,
+
+	#[allow(missing_docs)]
+	#[structopt(flatten)]
+	pub pruning_params: PruningParams,
 }
 
 impl RevertCmd {
@@ -72,6 +75,7 @@ impl RevertCmd {
 		F: FnOnce(&str) -> Result<Option<ChainSpec<G, E>>, String>,
 	{
 		self.shared_params.update_config(&mut config, spec_factory, version)?;
+		self.pruning_params.update_config(&mut config, Roles::FULL, true)?;
 		config.use_in_memory_keystore()?;
 
 		Ok(())

substrate/client/cli/src/params/import_params.rs

@@ -15,10 +15,7 @@
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
 use structopt::StructOpt;
-use sc_service::{
-	Configuration, RuntimeGenesis,
-	config::DatabaseConfig, PruningMode,
-};
+use sc_service::{Configuration, RuntimeGenesis, config::DatabaseConfig};
 
 use crate::error;
 use crate::arg_enums::{
@@ -26,17 +23,14 @@ use crate::arg_enums::{
 	DEFAULT_EXECUTION_IMPORT_BLOCK, DEFAULT_EXECUTION_OFFCHAIN_WORKER, DEFAULT_EXECUTION_OTHER,
 	DEFAULT_EXECUTION_SYNCING
 };
+use crate::params::PruningParams;
 
 /// Parameters for block import.
 #[derive(Debug, StructOpt, Clone)]
 pub struct ImportParams {
-	/// Specify the state pruning mode, a number of blocks to keep or 'archive'.
-	///
-	/// Default is to keep all block states if the node is running as a
-	/// validator (i.e. 'archive'), otherwise state is only kept for the last
-	/// 256 blocks.
-	#[structopt(long = "pruning", value_name = "PRUNING_MODE")]
-	pub pruning: Option<String>,
+	#[allow(missing_docs)]
+	#[structopt(flatten)]
+	pub pruning_params: PruningParams,
 
 	/// Force start with unsafe pruning settings.
 	///
@@ -87,7 +81,7 @@ impl ImportParams {
 	/// Put block import CLI params into `config` object.
 	pub fn update_config<G, E>(
 		&self,
-		config: &mut Configuration<G, E>,
+		mut config: &mut Configuration<G, E>,
 		role: sc_service::Roles,
 		is_dev: bool,
 	) -> error::Result<()>
@@ -102,27 +96,7 @@ impl ImportParams {
 
 		config.state_cache_size = self.state_cache_size;
 
-		// by default we disable pruning if the node is an authority (i.e.
-		// `ArchiveAll`), otherwise we keep state for the last 256 blocks. if the
-		// node is an authority and pruning is enabled explicitly, then we error
-		// unless `unsafe_pruning` is set.
-		config.pruning = match &self.pruning {
-			Some(ref s) if s == "archive" => PruningMode::ArchiveAll,
-			None if role == sc_service::Roles::AUTHORITY => PruningMode::ArchiveAll,
-			None => PruningMode::default(),
-			Some(s) => {
-				if role == sc_service::Roles::AUTHORITY && !self.unsafe_pruning {
-					return Err(error::Error::Input(
-						"Validators should run with state pruning disabled (i.e. archive). \
-						You can ignore this check with `--unsafe-pruning`.".to_string()
-					));
-				}
-
-				PruningMode::keep_blocks(s.parse()
-					.map_err(|_| error::Error::Input("Invalid pruning mode specified".to_string()))?
-				)
-			},
-		};
+		self.pruning_params.update_config(&mut config, role, self.unsafe_pruning)?;
 
 		config.wasm_method = self.wasm_method.into();
 
@@ -144,6 +118,7 @@ impl ImportParams {
 				exec_all_or(exec.execution_offchain_worker, DEFAULT_EXECUTION_OFFCHAIN_WORKER),
 			other: exec_all_or(exec.execution_other, DEFAULT_EXECUTION_OTHER),
 		};
+
 		Ok(())
 	}
 }

substrate/client/cli/src/params/mod.rs

@@ -19,6 +19,7 @@ mod transaction_pool_params;
 mod shared_params;
 mod node_key_params;
 mod network_configuration_params;
+mod pruning_params;
 
 use std::str::FromStr;
 use std::fmt::Debug;
@@ -28,6 +29,7 @@ pub use crate::params::transaction_pool_params::*;
 pub use crate::params::shared_params::*;
 pub use crate::params::node_key_params::*;
 pub use crate::params::network_configuration_params::*;
+pub use crate::params::pruning_params::*;
 
 /// Wrapper type of `String` that holds an unsigned integer of arbitrary size, formatted as a decimal.
 #[derive(Debug, Clone)]

substrate/client/cli/src/params/pruning_params.rs

@@ -0,0 +1,69 @@
+// Copyright 2020 Parity Technologies (UK) Ltd.
+// This file is part of Substrate.
+
+// Substrate is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Substrate is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
+
+use structopt::StructOpt;
+use sc_service::{Configuration, RuntimeGenesis, PruningMode};
+
+use crate::error;
+
+/// Parameters to define the pruning mode
+#[derive(Debug, StructOpt, Clone)]
+pub struct PruningParams {
+	/// Specify the state pruning mode, a number of blocks to keep or 'archive'.
+	///
+	/// Default is to keep all block states if the node is running as a
+	/// validator (i.e. 'archive'), otherwise state is only kept for the last
+	/// 256 blocks.
+	#[structopt(long = "pruning", value_name = "PRUNING_MODE")]
+	pub pruning: Option<String>,
+}
+
+impl PruningParams {
+	/// Put block pruning CLI params into `config` object.
+	pub fn update_config<G, E>(
+		&self,
+		mut config: &mut Configuration<G, E>,
+		role: sc_service::Roles,
+		unsafe_pruning: bool,
+	) -> error::Result<()>
+	where
+		G: RuntimeGenesis,
+	{
+		// by default we disable pruning if the node is an authority (i.e.
+		// `ArchiveAll`), otherwise we keep state for the last 256 blocks. if the
+		// node is an authority and pruning is enabled explicitly, then we error
+		// unless `unsafe_pruning` is set.
+		config.pruning = match &self.pruning {
+			Some(ref s) if s == "archive" => PruningMode::ArchiveAll,
+			None if role == sc_service::Roles::AUTHORITY => PruningMode::ArchiveAll,
+			None => PruningMode::default(),
+			Some(s) => {
+				if role == sc_service::Roles::AUTHORITY && !unsafe_pruning {
+					return Err(error::Error::Input(
+						"Validators should run with state pruning disabled (i.e. archive). \
+						You can ignore this check with `--unsafe-pruning`.".to_string()
+					));
+				}
+
+				PruningMode::keep_blocks(s.parse()
+					.map_err(|_| error::Error::Input("Invalid pruning mode specified".to_string()))?
+				)
+			},
+		};
+
+		Ok(())
+	}
+}