From 24780460217f018c5ce8a66addb6e81608733af7 Mon Sep 17 00:00:00 2001
From: Cecile Tonglet <cecile@parity.io>
Date: Tue, 25 Feb 2020 15:48:50 +0100
Subject: [PATCH] Fix: CI failing for some CLI tests (#5043)
* Initial commit
Forked at: ad90ab7ec92cf8cd0a974e55845d83e4505ef16e
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
---
.../bin/node/cli/tests/check_block_works.rs | 4 +-
substrate/bin/node/cli/tests/common.rs | 18 ++---
.../tests/import_export_and_revert_work.rs | 8 +--
substrate/bin/node/cli/tests/inspect_works.rs | 4 +-
.../bin/node/cli/tests/purge_chain_works.rs | 2 +-
.../cli/src/commands/export_blocks_cmd.rs | 10 ++-
.../client/cli/src/commands/revert_cmd.rs | 10 ++-
.../client/cli/src/params/import_params.rs | 41 +++--------
substrate/client/cli/src/params/mod.rs | 2 +
.../client/cli/src/params/pruning_params.rs | 69 +++++++++++++++++++
10 files changed, 112 insertions(+), 56 deletions(-)
create mode 100644 substrate/client/cli/src/params/pruning_params.rs
diff --git a/substrate/bin/node/cli/tests/check_block_works.rs b/substrate/bin/node/cli/tests/check_block_works.rs
index e4c93c9e885..6bfb82a8bfa 100644
--- a/substrate/bin/node/cli/tests/check_block_works.rs
+++ b/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()
diff --git a/substrate/bin/node/cli/tests/common.rs b/substrate/bin/node/cli/tests/common.rs
index 682a30bcc01..34e371195c1 100644
--- a/substrate/bin/node/cli/tests/common.rs
+++ b/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()
diff --git a/substrate/bin/node/cli/tests/import_export_and_revert_work.rs b/substrate/bin/node/cli/tests/import_export_and_revert_work.rs
index e109aa279eb..131265e3b4a 100644
--- a/substrate/bin/node/cli/tests/import_export_and_revert_work.rs
+++ b/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();
diff --git a/substrate/bin/node/cli/tests/inspect_works.rs b/substrate/bin/node/cli/tests/inspect_works.rs
index 0bd48c36938..441b08ccf46 100644
--- a/substrate/bin/node/cli/tests/inspect_works.rs
+++ b/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()
diff --git a/substrate/bin/node/cli/tests/purge_chain_works.rs b/substrate/bin/node/cli/tests/purge_chain_works.rs
index 42a5bc3ce14..020259d0c59 100644
--- a/substrate/bin/node/cli/tests/purge_chain_works.rs
+++ b/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"])
diff --git a/substrate/client/cli/src/commands/export_blocks_cmd.rs b/substrate/client/cli/src/commands/export_blocks_cmd.rs
index 8db650ae8c8..cdfa463c6d5 100644
--- a/substrate/client/cli/src/commands/export_blocks_cmd.rs
+++ b/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(())
diff --git a/substrate/client/cli/src/commands/revert_cmd.rs b/substrate/client/cli/src/commands/revert_cmd.rs
index 9ab86986cdd..f0c534898ef 100644
--- a/substrate/client/cli/src/commands/revert_cmd.rs
+++ b/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(())
diff --git a/substrate/client/cli/src/params/import_params.rs b/substrate/client/cli/src/params/import_params.rs
index 98809a38ae4..36c62bc979c 100644
--- a/substrate/client/cli/src/params/import_params.rs
+++ b/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(())
}
}
diff --git a/substrate/client/cli/src/params/mod.rs b/substrate/client/cli/src/params/mod.rs
index 75509afa425..f684cab3364 100644
--- a/substrate/client/cli/src/params/mod.rs
+++ b/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)]
diff --git a/substrate/client/cli/src/params/pruning_params.rs b/substrate/client/cli/src/params/pruning_params.rs
new file mode 100644
index 00000000000..ad64d757dcb
--- /dev/null
+++ b/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(())
+ }
+}
--
GitLab