Add tests & Service's Configuration has optional fields that shouldn't be optional

bin/node-template/node/src/command.rs

@@ -15,32 +15,35 @@
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
 use sp_consensus_aura::sr25519::{AuthorityPair as AuraPair};
-use sc_cli::{VersionInfo, error};
+use sc_cli::VersionInfo;
 use crate::service;
 use crate::chain_spec;
 use crate::cli::Cli;
 
 /// Parse and run command line arguments
-pub fn run(version: VersionInfo) -> error::Result<()> {
+pub fn run(version: VersionInfo) -> sc_cli::Result<()> {
 	let opt = sc_cli::from_args::<Cli>(&version);
 
-	let config = sc_service::Configuration::new(&version);
+	let mut config = sc_service::Configuration::from_version(&version);
 
 	match opt.subcommand {
-		Some(subcommand) => sc_cli::run_subcommand(
-			config,
-			subcommand,
-			chain_spec::load_spec,
-			|config: _| Ok(new_full_start!(config).0),
-			&version,
-		),
-		None => sc_cli::run(
-			config,
-			opt.run,
-			service::new_light,
-			service::new_full,
-			chain_spec::load_spec,
-			&version,
-		)
+		Some(subcommand) => {
+			subcommand.init(&version)?;
+			subcommand.update_config(&mut config, chain_spec::load_spec, &version)?;
+			subcommand.run(
+				config,
+				|config: _| Ok(new_full_start!(config).0),
+			)
+		},
+		None => {
+			opt.run.init(&version)?;
+			opt.run.update_config(&mut config, chain_spec::load_spec, &version)?;
+			opt.run.run(
+				config,
+				service::new_light,
+				service::new_full,
+				&version,
+			)
+		},
 	}
 }

bin/node-template/node/src/main.rs

@@ -7,10 +7,8 @@ mod service;
 mod cli;
 mod command;
 
-pub use sc_cli::{VersionInfo, error};
-
-fn main() -> Result<(), error::Error> {
-	let version = VersionInfo {
+fn main() -> sc_cli::Result<()> {
+	let version = sc_cli::VersionInfo {
 		name: "Substrate Node",
 		commit: env!("VERGEN_SHA_SHORT"),
 		version: env!("CARGO_PKG_VERSION"),

bin/node/cli/Cargo.toml

@@ -107,6 +107,7 @@ futures = "0.3.1"
 tempfile = "3.1.0"
 assert_cmd = "0.12"
 nix = "0.17"
+serde_json = "1.0"
 
 [build-dependencies]
 build-script-utils = { version = "2.0.0", package = "substrate-build-script-utils", path = "../../../utils/build-script-utils" }

bin/node/cli/bin/main.rs

@@ -18,10 +18,8 @@
 
 #![warn(missing_docs)]
 
-use sc_cli::VersionInfo;
-
-fn main() -> Result<(), sc_cli::error::Error> {
-	let version = VersionInfo {
+fn main() -> sc_cli::Result<()> {
+	let version = sc_cli::VersionInfo {
 		name: "Substrate Node",
 		commit: env!("VERGEN_SHA_SHORT"),
 		version: env!("CARGO_PKG_VERSION"),

bin/node/cli/src/cli.rs

@@ -19,11 +19,6 @@ use structopt::StructOpt;
 
 /// An overarching CLI command definition.
 #[derive(Clone, Debug, StructOpt)]
-#[structopt(settings = &[
-	structopt::clap::AppSettings::GlobalVersion,
-	structopt::clap::AppSettings::ArgsNegateSubcommands,
-	structopt::clap::AppSettings::SubcommandsNegateReqs,
-])]
 pub struct Cli {
 	/// Possible subcommand with parameters.
 	#[structopt(subcommand)]

bin/node/cli/src/command.rs

@@ -14,33 +14,36 @@
 // You should have received a copy of the GNU General Public License
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
-use sc_cli::{VersionInfo, error};
+use sc_cli::VersionInfo;
 use sc_service::{Roles as ServiceRoles};
 use node_transaction_factory::RuntimeAdapter;
 use crate::{Cli, service, ChainSpec, load_spec, Subcommand, factory_impl::FactoryState};
 
 /// Parse command line arguments into service configuration.
-pub fn run<I, T>(args: I, version: VersionInfo) -> error::Result<()>
+pub fn run<I, T>(args: I, version: VersionInfo) -> sc_cli::Result<()>
 where
 	I: Iterator<Item = T>,
 	T: Into<std::ffi::OsString> + Clone,
 {
 	let args: Vec<_> = args.collect();
 	let opt = sc_cli::from_iter::<Cli, _>(args.clone(), &version);
 
-	let mut config = sc_service::Configuration::new(&version);
+	let mut config = sc_service::Configuration::from_version(&version);
 
 	match opt.subcommand {
-		None => sc_cli::run(
-			config,
-			opt.run,
-			service::new_light,
-			service::new_full,
-			load_spec,
-			&version,
-		),
+		None => {
+			opt.run.init(&version)?;
+			opt.run.update_config(&mut config, load_spec, &version)?;
+			opt.run.run(
+				config,
+				service::new_light,
+				service::new_full,
+				&version,
+			)
+		},
 		Some(Subcommand::Inspect(cmd)) => {
-			cmd.init(&mut config, load_spec, &version)?;
+			cmd.init(&version)?;
+			cmd.update_config(&mut config, load_spec, &version)?;
 
 			let client = sc_service::new_full_client::<
 				node_runtime::Block, node_runtime::RuntimeApi, node_executor::Executor, _, _,
@@ -50,25 +53,27 @@ where
 			cmd.run(inspect)
 		},
 		Some(Subcommand::Benchmark(cmd)) => {
-			cmd.init(&mut config, load_spec, &version)?;
+			cmd.init(&version)?;
+			cmd.update_config(&mut config, load_spec, &version)?;
 
 			cmd.run::<_, _, node_runtime::Block, node_executor::Executor>(config)
 		},
 		Some(Subcommand::Factory(cli_args)) => {
-			sc_cli::init(&cli_args.shared_params, &version)?;
-			sc_cli::init_config(&mut config, &cli_args.shared_params, &version, load_spec)?;
-			sc_cli::fill_import_params(
+			cli_args.shared_params.init(&version)?;
+			cli_args.shared_params.update_config(&mut config, load_spec, &version)?;
+			cli_args.import_params.update_config(
 				&mut config,
-				&cli_args.import_params,
 				ServiceRoles::FULL,
 				cli_args.shared_params.dev,
 			)?;
 
-			sc_cli::fill_config_keystore_in_memory(&mut config)?;
+			config.use_in_memory_keystore()?;
 
 			match ChainSpec::from(config.expect_chain_spec().id()) {
 				Some(ref c) if c == &ChainSpec::Development || c == &ChainSpec::LocalTestnet => {},
-				_ => panic!("Factory is only supported for development and local testnet."),
+				_ => return Err(
+					"Factory is only supported for development and local testnet.".into()
+				),
 			}
 
 			// Setup tracing.
@@ -77,7 +82,9 @@ where
 					cli_args.import_params.tracing_receiver.into(), tracing_targets
 				);
 				if let Err(e) = tracing::subscriber::set_global_default(subscriber) {
-					panic!("Unable to set global default subscriber {}", e);
+					return Err(
+						format!("Unable to set global default subscriber {}", e).into()
+					);
 				}
 			}
 
@@ -96,12 +103,13 @@ where
 
 			Ok(())
 		},
-		Some(Subcommand::Base(subcommand)) => sc_cli::run_subcommand(
-			config,
-			subcommand,
-			load_spec,
-			|config: service::NodeConfiguration| Ok(new_full_start!(config).0),
-			&version,
-		),
+		Some(Subcommand::Base(subcommand)) => {
+			subcommand.init(&version)?;
+			subcommand.update_config(&mut config, load_spec, &version)?;
+			subcommand.run(
+				config,
+				|config: service::NodeConfiguration| Ok(new_full_start!(config).0),
+			)
+		},
 	}
 }

bin/node/cli/tests/build_spec_works.rs

@@ -0,0 +1,37 @@
+// 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 assert_cmd::cargo::cargo_bin;
+use std::process::Command;
+use tempfile::tempdir;
+
+#[test]
+fn build_spec_works() {
+	let base_path = tempdir().expect("could not create a temp dir");
+
+	let output = Command::new(cargo_bin("substrate"))
+		.args(&["build-spec", "--dev", "-d"])
+		.arg(base_path.path())
+		.output()
+		.unwrap();
+	assert!(output.status.success());
+
+	// Make sure that the `dev` chain folder exists, but the `db` doesn't
+	assert!(base_path.path().join("chains/dev/").exists());
+	assert!(!base_path.path().join("chains/dev/db").exists());
+
+	let _value: serde_json::Value = serde_json::from_slice(output.stdout.as_slice()).unwrap();
+}

client/cli/src/traits.rs → bin/node/cli/tests/check_block_works.rs

@@ -1,4 +1,4 @@
-// Copyright 2017-2020 Parity Technologies (UK) Ltd.
+// Copyright 2020 Parity Technologies (UK) Ltd.
 // This file is part of Substrate.
 
 // Substrate is free software: you can redistribute it and/or modify
@@ -14,10 +14,25 @@
 // You should have received a copy of the GNU General Public License
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
-use crate::params::SharedParams;
+#![cfg(unix)]
 
-/// Supports getting common params.
-pub trait GetSharedParams {
-	/// Returns shared params if any.
-	fn shared_params(&self) -> Option<&SharedParams>;
+use assert_cmd::cargo::cargo_bin;
+use std::process::Command;
+use tempfile::tempdir;
+
+mod common;
+
+#[test]
+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);
+
+	let status = Command::new(cargo_bin("substrate"))
+		.args(&["check-block", "-d"])
+		.arg(base_path.path())
+		.arg("1")
+		.status()
+		.unwrap();
+	assert!(status.success());
 }

bin/node/cli/tests/common.rs

@@ -14,7 +14,14 @@
 // You should have received a copy of the GNU General Public License
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
-use std::{process::{Child, ExitStatus}, thread, time::Duration};
+#![cfg(unix)]
+#![allow(dead_code)]
+
+use std::{process::{Child, ExitStatus}, thread, time::Duration, path::Path};
+use assert_cmd::cargo::cargo_bin;
+use std::{convert::TryInto, process::Command};
+use nix::sys::signal::{kill, Signal::SIGINT};
+use nix::unistd::Pid;
 
 /// Wait for the given `child` the given number of `secs`.
 ///
@@ -32,3 +39,26 @@ pub fn wait_for(child: &mut Child, secs: usize) -> Option<ExitStatus> {
 
 	None
 }
+
+/// Run the node for a while (30 seconds)
+pub fn run_command_for_a_while(base_path: &Path, dev: bool) {
+	let mut cmd = Command::new(cargo_bin("substrate"));
+
+	if dev {
+		cmd.arg("--dev");
+	}
+
+	let mut cmd = cmd
+		.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");
+
+	// Stop the process
+	kill(Pid::from_raw(cmd.id().try_into().unwrap()), SIGINT).unwrap();
+	assert!(wait_for(&mut cmd, 20).map(|x| x.success()).unwrap_or_default());
+}

bin/node/cli/tests/factory.rs

@@ -0,0 +1,40 @@
+// 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/>.
+
+#![cfg(unix)]
+
+use assert_cmd::cargo::cargo_bin;
+use std::process::{Command, Stdio};
+use tempfile::tempdir;
+
+mod common;
+
+#[test]
+fn factory_works() {
+	let base_path = tempdir().expect("could not create a temp dir");
+
+	let status = Command::new(cargo_bin("substrate"))
+		.stdout(Stdio::null())
+		.args(&["factory", "--dev", "-d"])
+		.arg(base_path.path())
+		.status()
+		.unwrap();
+	assert!(status.success());
+
+	// Make sure that the `dev` chain folder exists & `db`
+	assert!(base_path.path().join("chains/dev/").exists());
+	assert!(base_path.path().join("chains/dev/db").exists());
+}