From 8bc859318b54e112a3946eb43d6a0f588ab426a6 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Tue, 7 Feb 2023 11:43:25 +0000 Subject: [PATCH] `[ink_e2e]` spawn a separate contracts node instance per test (#1642) * Spawn a contracts node instance per test * Allow user to specify contracts node executable * Improve CONTRACTS_NODE panic message * Update docs * Remove starting node from examples-test CI * Remove create_and_fund_account retries * Fmt * clippy * Remove tests * Fmt * spellcheck * spellcheck * Add a test for TestNodeProcess * Update CHANGELOG.md * Remove move --- .gitlab-ci.yml | 4 - CHANGELOG.md | 4 + crates/e2e/macro/Cargo.toml | 1 + crates/e2e/macro/src/codegen.rs | 43 ++++-- crates/e2e/macro/src/config.rs | 28 +--- crates/e2e/macro/src/lib.rs | 26 +--- crates/e2e/src/client.rs | 66 ++++----- crates/e2e/src/lib.rs | 7 +- crates/e2e/src/node_proc.rs | 233 ++++++++++++++++++++++++++++++++ crates/e2e/src/tests.rs | 11 -- crates/e2e/src/xts.rs | 30 ++-- examples/mother/lib.rs | 8 -- 12 files changed, 307 insertions(+), 154 deletions(-) create mode 100644 crates/e2e/src/node_proc.rs delete mode 100644 crates/e2e/src/tests.rs diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index abd73be748..d88e4c2710 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -87,9 +87,6 @@ workflow: tags: - kubernetes-parity-build -.start-substrate-contracts-node: &start-substrate-contracts-node - - substrate-contracts-node -linfo,runtime::contracts=debug 2>&1 | tee /tmp/contracts-node.log & - #### stage: lint # # Note: For all of these lints we `allow_failure` so that the rest of the build can @@ -375,7 +372,6 @@ examples-test: - job: clippy-std artifacts: false script: - - *start-substrate-contracts-node - for example in examples/*/; do if [ "$example" = "examples/upgradeable-contracts/" ]; then continue; fi; if [ "$example" = "examples/lang-err-integration-tests/" ]; then continue; fi; diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a71a52eda..3b033fb26c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] +### Changed +- E2E: spawn a separate contracts node instance per test ‒ [#1642](https://github.com/paritytech/ink/pull/1642) + ## Version 4.0.0-rc The first release candidate is here! This is the first release which could become the final diff --git a/crates/e2e/macro/Cargo.toml b/crates/e2e/macro/Cargo.toml index d7e970a812..9fa931e054 100644 --- a/crates/e2e/macro/Cargo.toml +++ b/crates/e2e/macro/Cargo.toml @@ -28,3 +28,4 @@ serde_json = "1.0.89" syn = "1" proc-macro2 = "1" quote = "1" +which = "4.4.0" \ No newline at end of file diff --git a/crates/e2e/macro/src/codegen.rs b/crates/e2e/macro/src/codegen.rs index e43d0f3aed..b8167c19f3 100644 --- a/crates/e2e/macro/src/codegen.rs +++ b/crates/e2e/macro/src/codegen.rs @@ -72,8 +72,6 @@ impl InkE2ETest { syn::ReturnType::Type(rarrow, ret_type) => quote! { #rarrow #ret_type }, }; - let ws_url = &self.test.config.ws_url(); - let mut additional_contracts: Vec = self.test.config.additional_contracts(); let default_main_contract_manifest_path = String::from("Cargo.toml"); @@ -113,6 +111,24 @@ impl InkE2ETest { quote! { #bundle_path } }); + const DEFAULT_CONTRACTS_NODE: &str = "substrate-contracts-node"; + + // use the user supplied `CONTRACTS_NODE` or default to `substrate-contracts-node` + let contracts_node: &'static str = + option_env!("CONTRACTS_NODE").unwrap_or(DEFAULT_CONTRACTS_NODE); + + // check the specified contracts node. + if which::which(contracts_node).is_err() { + if contracts_node == DEFAULT_CONTRACTS_NODE { + panic!( + "The '{DEFAULT_CONTRACTS_NODE}' executable was not found. Install '{DEFAULT_CONTRACTS_NODE}' on the PATH, \ + or specify the `CONTRACTS_NODE` environment variable.", + ) + } else { + panic!("The contracts node executable '{contracts_node}' was not found.") + } + } + quote! { #( #attrs )* #[test] @@ -126,28 +142,25 @@ impl InkE2ETest { ::ink_e2e::INIT.call_once(|| { ::ink_e2e::env_logger::init(); - let check_async = ::ink_e2e::Client::< - ::ink_e2e::PolkadotConfig, - ink::env::DefaultEnvironment - >::new(&#ws_url, []); - - ::ink_e2e::tokio::runtime::Builder::new_current_thread() - .enable_all() - .build() - .unwrap_or_else(|err| - panic!("Failed building the Runtime during initialization: {}", err)) - .block_on(check_async); }); log_info("creating new client"); let run = async { - // TODO(#xxx) Make those two generic environments customizable. + // spawn a contracts node process just for this test + let node_proc = ::ink_e2e::TestNodeProcess::<::ink_e2e::PolkadotConfig> + ::build(#contracts_node) + .spawn() + .await + .unwrap_or_else(|err| + ::core::panic!("Error spawning substrate-contracts-node: {:?}", err) + ); + let mut client = ::ink_e2e::Client::< ::ink_e2e::PolkadotConfig, ink::env::DefaultEnvironment >::new( - &#ws_url, + node_proc.client(), [ #( #contracts ),* ] ).await; diff --git a/crates/e2e/macro/src/config.rs b/crates/e2e/macro/src/config.rs index 515a30833a..58b53ca2ef 100644 --- a/crates/e2e/macro/src/config.rs +++ b/crates/e2e/macro/src/config.rs @@ -24,8 +24,6 @@ use ink_ir::{ /// The End-to-End test configuration. #[derive(Debug, Default, PartialEq, Eq)] pub struct E2EConfig { - /// The WebSocket URL where to connect with the node. - ws_url: Option, /// The set of attributes that can be passed to call builder in the codegen. whitelisted_attributes: WhitelistedAttributes, /// Additional contracts that have to be built before executing the test. @@ -36,24 +34,11 @@ impl TryFrom for E2EConfig { type Error = syn::Error; fn try_from(args: ast::AttributeArgs) -> Result { - let mut ws_url: Option<(syn::LitStr, ast::MetaNameValue)> = None; let mut whitelisted_attributes = WhitelistedAttributes::default(); let mut additional_contracts: Option<(syn::LitStr, ast::MetaNameValue)> = None; for arg in args.into_iter() { - if arg.name.is_ident("ws_url") { - if let Some((_, ast)) = ws_url { - return Err(duplicate_config_err(ast, arg, "ws_url", "e2e test")) - } - if let ast::PathOrLit::Lit(syn::Lit::Str(lit_str)) = &arg.value { - ws_url = Some((lit_str.clone(), arg)) - } else { - return Err(format_err_spanned!( - arg, - "expected a string literal for `ws_url` ink! e2e test configuration argument", - )) - } - } else if arg.name.is_ident("keep_attr") { + if arg.name.is_ident("keep_attr") { whitelisted_attributes.parse_arg_value(&arg)?; } else if arg.name.is_ident("additional_contracts") { if let Some((_, ast)) = additional_contracts { @@ -83,7 +68,6 @@ impl TryFrom for E2EConfig { .map(|(value, _)| value.value().split(' ').map(String::from).collect()) .unwrap_or_else(Vec::new); Ok(E2EConfig { - ws_url: ws_url.map(|(value, _)| value), additional_contracts, whitelisted_attributes, }) @@ -91,15 +75,6 @@ impl TryFrom for E2EConfig { } impl E2EConfig { - /// Returns the WebSocket URL where to connect to the RPC endpoint - /// of the node, if specified. Otherwise returns the default URL - /// `ws://localhost:9944`. - pub fn ws_url(&self) -> syn::LitStr { - let default_ws_url = - syn::LitStr::new("ws://0.0.0.0:9944", proc_macro2::Span::call_site()); - self.ws_url.clone().unwrap_or(default_ws_url) - } - /// Returns a vector of additional contracts that have to be built /// and imported before executing the test. pub fn additional_contracts(&self) -> Vec { @@ -175,7 +150,6 @@ mod tests { keep_attr = "foo, bar" }, Ok(E2EConfig { - ws_url: None, whitelisted_attributes: attrs, additional_contracts: Vec::new(), }), diff --git a/crates/e2e/macro/src/lib.rs b/crates/e2e/macro/src/lib.rs index 705fcb1e39..9c0510ee86 100644 --- a/crates/e2e/macro/src/lib.rs +++ b/crates/e2e/macro/src/lib.rs @@ -29,11 +29,10 @@ use syn::Result; /// /// The system requirements are: /// -/// - A Substrate node with `pallet-contracts` running in the background. +/// - A Substrate node with `pallet-contracts` installed on the local system. /// You can e.g. use [`substrate-contracts-node`](https://github.com/paritytech/substrate-contracts-node) -/// and launch it with -/// `substrate-contracts-node -lerror,runtime::contracts=debug > /tmp/contracts-node.log 2>&1`. -/// - A `cargo-contract` installation that can build the contract. +/// and install it on your PATH, or provide a path to an executable using the `CONTRACTS_NODE` +/// environment variable. /// /// Before the test function is invoked the contract will have been build. Any errors /// that occur during the contract build will prevent the test function from being @@ -44,25 +43,6 @@ use syn::Result; /// The `#[ink::e2e_test]` macro can be provided with some additional comma-separated /// header arguments: /// -/// - `ws_url: String` -/// -/// The `ws_url` denotes the WebSocket URL where to connect to the RPC -/// endpoint of the node. -/// -/// **Usage Example:** -/// ```no_compile -/// # // TODO(#xxx) Remove the `no_compile`. -/// type E2EResult = std::result::Result>; -/// #[ink::e2e_test(ws_url = "ws://localhost:9944")] -/// async fn e2e_contract_must_transfer_value_to_sender( -/// mut client: ::ink_e2e::Client, -/// ) -> E2EResult<()> { -/// Ok(()) -/// } -/// ``` -/// -/// **Default value:** `"ws://localhost:9944"`. -/// /// # Example /// /// ```no_compile diff --git a/crates/e2e/src/client.rs b/crates/e2e/src/client.rs index 08984fd097..94ccd35a94 100644 --- a/crates/e2e/src/client.rs +++ b/crates/e2e/src/client.rs @@ -374,21 +374,11 @@ where E::Balance: Debug + scale::HasCompact + serde::Serialize, E::Hash: Debug + scale::Encode, { - /// Creates a new [`Client`] instance. - pub async fn new(url: &str, contracts: impl IntoIterator) -> Self { - let client = subxt::OnlineClient::from_url(url) - .await - .unwrap_or_else(|err| { - if let subxt::Error::Rpc(subxt::error::RpcError::ClientError(_)) = err { - let error_msg = format!("Error establishing connection to a node at {url}. Make sure you run a node behind the given url!"); - log_error(&error_msg); - panic!("{}", error_msg); - } - log_error( - "Unable to create client! Please check that your node is running.", - ); - panic!("Unable to create client: {err:?}"); - }); + /// Creates a new [`Client`] instance using a `subxt` client. + pub async fn new( + client: subxt::OnlineClient, + contracts: impl IntoIterator, + ) -> Self { let contracts = contracts .into_iter() .map(|path| { @@ -405,7 +395,7 @@ where .collect(); Self { - api: ContractsApi::new(client, url).await, + api: ContractsApi::new(client).await, contracts, } } @@ -421,38 +411,30 @@ where ) -> Signer where E::Balance: Clone, - C::AccountId: Clone + core::fmt::Display + core::fmt::Debug, + C::AccountId: Clone + core::fmt::Display + Debug, C::AccountId: From, { let (pair, _, _) = ::generate_with_phrase(None); let pair_signer = PairSigner::::new(pair); let account_id = pair_signer.account_id().to_owned(); - for _ in 0..6 { - let transfer_result = self - .api - .try_transfer_balance(origin, account_id.clone(), amount) - .await; - match transfer_result { - Ok(_) => { - log_info(&format!( - "transfer from {} to {} succeeded", - origin.account_id(), - account_id, - )); - break - } - Err(err) => { - log_info(&format!( - "transfer from {} to {} failed with {:?}", - origin.account_id(), - account_id, - err - )); - tokio::time::sleep(std::time::Duration::from_secs(1)).await; - } - } - } + self.api + .try_transfer_balance(origin, account_id.clone(), amount) + .await + .unwrap_or_else(|err| { + panic!( + "transfer from {} to {} failed with {:?}", + origin.account_id(), + account_id, + err + ) + }); + + log_info(&format!( + "transfer from {} to {} succeeded", + origin.account_id(), + account_id, + )); pair_signer } diff --git a/crates/e2e/src/lib.rs b/crates/e2e/src/lib.rs index d6a7788dc1..8591310532 100644 --- a/crates/e2e/src/lib.rs +++ b/crates/e2e/src/lib.rs @@ -22,8 +22,7 @@ mod builders; mod client; mod default_accounts; -#[cfg(test)] -mod tests; +mod node_proc; pub mod utils; mod xts; @@ -42,6 +41,10 @@ pub use client::{ pub use default_accounts::*; pub use env_logger; pub use ink_e2e_macro::test; +pub use node_proc::{ + TestNodeProcess, + TestNodeProcessBuilder, +}; pub use sp_core::H256; pub use sp_keyring::AccountKeyring; pub use subxt::{ diff --git a/crates/e2e/src/node_proc.rs b/crates/e2e/src/node_proc.rs new file mode 100644 index 0000000000..a462f68bb1 --- /dev/null +++ b/crates/e2e/src/node_proc.rs @@ -0,0 +1,233 @@ +// Copyright 2018-2022 Parity Technologies (UK) Ltd. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use sp_keyring::AccountKeyring; +use std::{ + ffi::{ + OsStr, + OsString, + }, + io::{ + BufRead, + BufReader, + Read, + }, + process, +}; +use subxt::{ + Config, + OnlineClient, +}; + +/// Spawn a local substrate node for testing. +pub struct TestNodeProcess { + proc: process::Child, + client: OnlineClient, + url: String, +} + +impl Drop for TestNodeProcess +where + R: Config, +{ + fn drop(&mut self) { + let _ = self.kill(); + } +} + +impl TestNodeProcess +where + R: Config, +{ + /// Construct a builder for spawning a test node process. + pub fn build(program: S) -> TestNodeProcessBuilder + where + S: AsRef + Clone, + { + TestNodeProcessBuilder::new(program) + } + + /// Attempt to kill the running substrate process. + pub fn kill(&mut self) -> Result<(), String> { + log::info!("Killing node process {}", self.proc.id()); + if let Err(err) = self.proc.kill() { + let err = format!("Error killing node process {}: {}", self.proc.id(), err); + log::error!("{}", err); + return Err(err) + } + Ok(()) + } + + /// Returns the `subxt` client connected to the running node. + pub fn client(&self) -> OnlineClient { + self.client.clone() + } + + /// Returns the URL of the running node. + pub fn url(&self) -> &str { + &self.url + } +} + +/// Construct a test node process. +pub struct TestNodeProcessBuilder { + node_path: OsString, + authority: Option, + marker: std::marker::PhantomData, +} + +impl TestNodeProcessBuilder +where + R: Config, +{ + pub fn new

(node_path: P) -> TestNodeProcessBuilder + where + P: AsRef, + { + Self { + node_path: node_path.as_ref().into(), + authority: None, + marker: Default::default(), + } + } + + /// Set the authority development account for a node in validator mode e.g. --alice. + pub fn with_authority(&mut self, account: AccountKeyring) -> &mut Self { + self.authority = Some(account); + self + } + + /// Spawn the substrate node at the given path, and wait for RPC to be initialized. + pub async fn spawn(&self) -> Result, String> { + let mut cmd = process::Command::new(&self.node_path); + cmd.env("RUST_LOG", "info") + .arg("--dev") + .stdout(process::Stdio::piped()) + .stderr(process::Stdio::piped()) + .arg("--port=0") + .arg("--rpc-port=0") + .arg("--ws-port=0"); + + if let Some(authority) = self.authority { + let authority = format!("{authority:?}"); + let arg = format!("--{}", authority.as_str().to_lowercase()); + cmd.arg(arg); + } + + let mut proc = cmd.spawn().map_err(|e| { + format!( + "Error spawning substrate node '{}': {}", + self.node_path.to_string_lossy(), + e + ) + })?; + + // Wait for RPC port to be logged (it's logged to stderr): + let stderr = proc.stderr.take().unwrap(); + let ws_port = find_substrate_port_from_output(stderr); + let ws_url = format!("ws://127.0.0.1:{ws_port}"); + + // Connect to the node with a `subxt` client: + let client = OnlineClient::from_url(ws_url.clone()).await; + match client { + Ok(client) => { + Ok(TestNodeProcess { + proc, + client, + url: ws_url.clone(), + }) + } + Err(err) => { + let err = format!("Failed to connect to node rpc at {ws_url}: {err}"); + log::error!("{}", err); + proc.kill().map_err(|e| { + format!("Error killing substrate process '{}': {}", proc.id(), e) + })?; + Err(err) + } + } + } +} + +// Consume a stderr reader from a spawned substrate command and +// locate the port number that is logged out to it. +fn find_substrate_port_from_output(r: impl Read + Send + 'static) -> u16 { + BufReader::new(r) + .lines() + .find_map(|line| { + let line = + line.expect("failed to obtain next line from stdout for port discovery"); + + // does the line contain our port (we expect this specific output from substrate). + let line_end = line + .rsplit_once("Listening for new connections on 127.0.0.1:") + .or_else(|| { + line.rsplit_once("Running JSON-RPC WS server: addr=127.0.0.1:") + }) + .map(|(_, port_str)| port_str)?; + + // trim non-numeric chars from the end of the port part of the line. + let port_str = line_end.trim_end_matches(|b| !('0'..='9').contains(&b)); + + // expect to have a number here (the chars after '127.0.0.1:') and parse them into a u16. + let port_num = port_str.parse().unwrap_or_else(|_| { + panic!("valid port expected for log line, got '{port_str}'") + }); + + Some(port_num) + }) + .expect("We should find a port before the reader ends") +} + +#[cfg(test)] +mod tests { + use super::*; + use subxt::PolkadotConfig as SubxtConfig; + + #[tokio::test] + #[allow(unused_assignments)] + async fn spawning_and_killing_nodes_works() { + let mut client1: Option> = None; + let mut client2: Option> = None; + + { + let node_proc1 = + TestNodeProcess::::build("substrate-contracts-node") + .spawn() + .await + .unwrap(); + client1 = Some(node_proc1.client()); + + let node_proc2 = + TestNodeProcess::::build("substrate-contracts-node") + .spawn() + .await + .unwrap(); + client2 = Some(node_proc2.client()); + + let res1 = node_proc1.client().rpc().block_hash(None).await; + let res2 = node_proc1.client().rpc().block_hash(None).await; + + assert!(res1.is_ok()); + assert!(res2.is_ok()); + } + + // node processes should have been killed by `Drop` in the above block. + let res1 = client1.unwrap().rpc().block_hash(None).await; + let res2 = client2.unwrap().rpc().block_hash(None).await; + + assert!(res1.is_err()); + assert!(res2.is_err()); + } +} diff --git a/crates/e2e/src/tests.rs b/crates/e2e/src/tests.rs deleted file mode 100644 index 5feb43d664..0000000000 --- a/crates/e2e/src/tests.rs +++ /dev/null @@ -1,11 +0,0 @@ -#[tokio::test] -#[should_panic( - expected = "Error establishing connection to a node at ws://0.0.0.0:9999. Make sure you run a node behind the given url!" -)] -async fn fail_initialization_with_no_node() { - let _ = crate::Client::::new( - "ws://0.0.0.0:9999", - [], - ) - .await; -} diff --git a/crates/e2e/src/xts.rs b/crates/e2e/src/xts.rs index a1f37791ed..c79027827b 100644 --- a/crates/e2e/src/xts.rs +++ b/crates/e2e/src/xts.rs @@ -23,14 +23,6 @@ use super::{ use ink_env::Environment; use core::marker::PhantomData; -use jsonrpsee::{ - core::client::ClientT, - rpc_params, - ws_client::{ - WsClient, - WsClientBuilder, - }, -}; use pallet_contracts_primitives::CodeUploadResult; use sp_core::{ Bytes, @@ -40,6 +32,7 @@ use sp_weights::Weight; use subxt::{ blocks::ExtrinsicEvents, config::ExtrinsicParams, + rpc_params, tx, OnlineClient, }; @@ -157,7 +150,6 @@ enum Code { /// Provides functions for interacting with the `pallet-contracts` API. pub struct ContractsApi { pub client: OnlineClient, - ws_client: WsClient, _phantom: PhantomData (C, E)>, } @@ -173,18 +165,9 @@ where E::Balance: scale::HasCompact + serde::Serialize, { /// Creates a new [`ContractsApi`] instance. - pub async fn new(client: OnlineClient, url: &str) -> Self { - let ws_client = - WsClientBuilder::default() - .build(&url) - .await - .unwrap_or_else(|err| { - panic!("error on ws request: {err:?}"); - }); - + pub async fn new(client: OnlineClient) -> Self { Self { client, - ws_client, _phantom: Default::default(), } } @@ -246,7 +229,8 @@ where let func = "ContractsApi_instantiate"; let params = rpc_params![func, Bytes(scale::Encode::encode(&call_request))]; let bytes: Bytes = self - .ws_client + .client + .rpc() .request("state_call", params) .await .unwrap_or_else(|err| { @@ -341,7 +325,8 @@ where let func = "ContractsApi_upload_code"; let params = rpc_params![func, Bytes(scale::Encode::encode(&call_request))]; let bytes: Bytes = self - .ws_client + .client + .rpc() .request("state_call", params) .await .unwrap_or_else(|err| { @@ -395,7 +380,8 @@ where let func = "ContractsApi_call"; let params = rpc_params![func, Bytes(scale::Encode::encode(&call_request))]; let bytes: Bytes = self - .ws_client + .client + .rpc() .request("state_call", params) .await .unwrap_or_else(|err| { diff --git a/examples/mother/lib.rs b/examples/mother/lib.rs index 6aecb4719c..8261964f11 100755 --- a/examples/mother/lib.rs +++ b/examples/mother/lib.rs @@ -235,12 +235,4 @@ mod mother { let _ = contract.revert_or_trap(Some(Failure::Panic)); } } - - // todo: restore this test once new `contract-build` crate released satisfying clippy lint - // #[cfg(test)] - // mod e2e_tests { - // #[ink_e2e::test(ws_url = "ws:://0.0.0.0:9944")] - // #[should_panic] - // async fn e2e_must_fail_without_connection(mut client: ink_e2e::Client) {} - // } }