From 4976461d43f7ba8547ce33a8f80997d22ee551aa Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Wed, 23 Feb 2022 18:13:38 +0700 Subject: [PATCH 01/10] Added streaming to tests stdout, misc capitalization fixes --- cli/src/cmd/mod.rs | 6 ++-- cli/src/cmd/test.rs | 73 ++++++++++++++++++--------------------- forge/src/multi_runner.rs | 51 +++++++++++++++++++++++---- 3 files changed, 81 insertions(+), 49 deletions(-) diff --git a/cli/src/cmd/mod.rs b/cli/src/cmd/mod.rs index c29014618d78..d5ac5a7a225b 100644 --- a/cli/src/cmd/mod.rs +++ b/cli/src/cmd/mod.rs @@ -81,15 +81,15 @@ If you are in a subdirectory in a Git repository, try adding `--root .`"#, ); } - println!("compiling..."); + println!("Compiling..."); let output = project.compile()?; if output.has_compiler_errors() { eyre::bail!(output.to_string()) } else if output.is_unchanged() { - println!("no files changed, compilation skipped."); + println!("No files changed, compilation skipped."); } else { println!("{}", output); - println!("success."); + println!("Success."); } Ok(output) } diff --git a/cli/src/cmd/test.rs b/cli/src/cmd/test.rs index cd3fc602eb9b..3f81c814c705 100644 --- a/cli/src/cmd/test.rs +++ b/cli/src/cmd/test.rs @@ -13,7 +13,7 @@ use evm_adapters::{ use forge::{MultiContractRunnerBuilder, TestFilter}; use foundry_config::{figment::Figment, Config}; use regex::Regex; -use std::{collections::BTreeMap, str::FromStr}; +use std::{collections::BTreeMap, str::FromStr, sync::{Arc, Mutex}}; #[derive(Debug, Clone, Parser)] pub struct Filter { @@ -309,47 +309,38 @@ fn test( allow_failure: bool, gas_reports: (bool, Vec), ) -> eyre::Result { + let verbosity = evm_opts.verbosity; let gas_reporting = gas_reports.0; - if gas_reporting && evm_opts.verbosity < 3 { // force evm to do tracing, but dont hit the verbosity print path evm_opts.verbosity = 3; } - let mut runner = builder.build(project, evm_opts)?; - let results = runner.test(&filter)?; - - let mut gas_report = GasReport::new(gas_reports.1); - - let (funcs, events, errors) = runner.execution_info; if json { - let res = serde_json::to_string(&results)?; + let results = runner.test(&filter)?; + let res = serde_json::to_string(&results)?; // TODO: Make this work normally println!("{}", res); + Ok(TestOutcome::new(results, allow_failure)) } else { // Dapptools-style printing of test results - for (i, (contract_name, tests)) in results.iter().enumerate() { - if i > 0 { + let mut gas_report = GasReport::new(gas_reports.1); + let execution_info = runner.execution_info.clone(); + let known_contracts = runner.known_contracts.clone(); + let index = Arc::new(Mutex::new(0)); + let results = runner.test_stream(&filter, |contract_name, tests| { + let mut index = index.lock().unwrap(); + if *index > 0 { println!() } + *index += 1; if !tests.is_empty() { let term = if tests.len() > 1 { "tests" } else { "test" }; println!("Running {} {} for {}", tests.len(), term, contract_name); } - for (name, result) in tests { - // build up gas report - if gas_reporting { - if let (Some(traces), Some(identified_contracts)) = - (&result.traces, &result.identified_contracts) - { - gas_report.analyze(traces, identified_contracts); - } - } - - short_test_result(name, result); - + short_test_result(&name, &result); // adds a linebreak only if there were any traces or logs, so that the // output does not look like 1 big block. let mut add_newline = false; @@ -360,33 +351,29 @@ fn test( println!(" {}", log); } } - if verbosity > 2 { if let (Some(traces), Some(identified_contracts)) = - (&result.traces, &result.identified_contracts) - { + (&result.traces, &result.identified_contracts) { if !result.success && verbosity == 3 || verbosity > 3 { // add a new line if any logs were printed & to separate them from // the traces to be printed if !result.logs.is_empty() { println!(); } - let mut ident = identified_contracts.clone(); let mut exec_info = ExecutionInfo::new( - &runner.known_contracts, + &known_contracts, &mut ident, &result.labeled_addresses, - &funcs, - &events, - &errors, + &execution_info.0, + &execution_info.1, + &execution_info.2, ); let vm = vm(); let mut trace_string = "".to_string(); if verbosity > 4 || !result.success { add_newline = true; println!("Traces:"); - // print setup calls as well traces.iter().for_each(|trace| { trace.construct_trace_string( @@ -417,18 +404,24 @@ fn test( } } } - if add_newline { println!(); } } - } - } + })?; - if gas_reporting { - gas_report.finalize(); - println!("{}", gas_report); + if gas_reporting { + for (_, tests) in results.iter() { + for (_, result) in tests { + if let (Some(traces), Some(identified_contracts)) = + (&result.traces, &result.identified_contracts) { + gas_report.analyze(traces, identified_contracts); + } + } + } + gas_report.finalize(); + println!("{}", gas_report); + } + Ok(TestOutcome::new(results, allow_failure)) } - - Ok(TestOutcome::new(results, allow_failure)) } diff --git a/forge/src/multi_runner.rs b/forge/src/multi_runner.rs index 7cc411dbc342..55a83cb082fe 100644 --- a/forge/src/multi_runner.rs +++ b/forge/src/multi_runner.rs @@ -17,7 +17,10 @@ use proptest::test_runner::TestRunner; use eyre::Result; use rayon::prelude::*; -use std::collections::BTreeMap; +use std::{ + collections::BTreeMap, + marker::Sync, +}; /// Builder used for instantiating the multi-contract runner #[derive(Debug, Default)] @@ -44,15 +47,15 @@ impl MultiContractRunnerBuilder { // TODO: Can we remove the static? It's due to the `into_artifacts()` call below A: ArtifactOutput + 'static, { - println!("compiling..."); + println!("Compiling..."); let output = project.compile()?; if output.has_compiler_errors() { // return the diagnostics error back to the user. eyre::bail!(output.to_string()) } else if output.is_unchanged() { - println!("no files changed, compilation skipped."); + println!("No files changed, compilation skipped"); } else { - println!("success."); + println!("Success"); } // This is just the contracts compiled, but we need to merge this with the read cached @@ -191,13 +194,49 @@ pub struct MultiContractRunner { } impl MultiContractRunner { - pub fn test( + pub fn test_stream( &mut self, filter: &(impl TestFilter + Send + Sync), + stream_result: impl Fn(String, BTreeMap) -> () + Sync, ) -> Result>> { - // TODO: Convert to iterator, ideally parallel one? let contracts = std::mem::take(&mut self.contracts); + let vicinity = self.evm_opts.vicinity()?; + let backend = self.evm_opts.backend(&vicinity)?; + let source_paths = self.source_paths.clone(); + + let results = contracts + .par_iter() + .filter(|(name, _)| filter.matches_path(source_paths.get(*name).unwrap())) + .filter(|(name, _)| filter.matches_contract(name)) + .map(|(name, (abi, deploy_code, libs))| { + // unavoidable duplication here? + let result = match backend { + BackendKind::Simple(ref backend) => { + self.run_tests(name, abi, backend, deploy_code.clone(), libs, filter)? + } + BackendKind::Shared(ref backend) => { + self.run_tests(name, abi, backend, deploy_code.clone(), libs, filter)? + } + }; + Ok((name.clone(), result)) + }) + .filter_map(|x: Result<_>| x.ok()) + .filter_map(|(name, result)| if result.is_empty() { None } else { Some((name, result)) }) + .map(|(name, result)| { + stream_result(name.clone(), result.clone()); + (name.clone(), result) + }) + .collect::>(); + self.contracts = contracts; + Ok(results) + } + + pub fn test( + &mut self, + filter: &(impl TestFilter + Send + Sync), + ) -> Result>> { + let contracts = std::mem::take(&mut self.contracts); let vicinity = self.evm_opts.vicinity()?; let backend = self.evm_opts.backend(&vicinity)?; let source_paths = self.source_paths.clone(); From 02458169486fc2104fb118a68d6ea68baed3e96f Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Wed, 23 Feb 2022 18:24:11 +0700 Subject: [PATCH 02/10] Formatting --- cli/src/cmd/test.rs | 13 +++++++++---- forge/src/multi_runner.rs | 9 ++++----- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/cli/src/cmd/test.rs b/cli/src/cmd/test.rs index 3f81c814c705..e9a4f8ab359f 100644 --- a/cli/src/cmd/test.rs +++ b/cli/src/cmd/test.rs @@ -13,7 +13,11 @@ use evm_adapters::{ use forge::{MultiContractRunnerBuilder, TestFilter}; use foundry_config::{figment::Figment, Config}; use regex::Regex; -use std::{collections::BTreeMap, str::FromStr, sync::{Arc, Mutex}}; +use std::{ + collections::BTreeMap, + str::FromStr, + sync::{Arc, Mutex}, +}; #[derive(Debug, Clone, Parser)] pub struct Filter { @@ -309,7 +313,6 @@ fn test( allow_failure: bool, gas_reports: (bool, Vec), ) -> eyre::Result { - let verbosity = evm_opts.verbosity; let gas_reporting = gas_reports.0; if gas_reporting && evm_opts.verbosity < 3 { @@ -353,7 +356,8 @@ fn test( } if verbosity > 2 { if let (Some(traces), Some(identified_contracts)) = - (&result.traces, &result.identified_contracts) { + (&result.traces, &result.identified_contracts) + { if !result.success && verbosity == 3 || verbosity > 3 { // add a new line if any logs were printed & to separate them from // the traces to be printed @@ -414,7 +418,8 @@ fn test( for (_, tests) in results.iter() { for (_, result) in tests { if let (Some(traces), Some(identified_contracts)) = - (&result.traces, &result.identified_contracts) { + (&result.traces, &result.identified_contracts) + { gas_report.analyze(traces, identified_contracts); } } diff --git a/forge/src/multi_runner.rs b/forge/src/multi_runner.rs index 55a83cb082fe..d42bd76d2476 100644 --- a/forge/src/multi_runner.rs +++ b/forge/src/multi_runner.rs @@ -17,10 +17,7 @@ use proptest::test_runner::TestRunner; use eyre::Result; use rayon::prelude::*; -use std::{ - collections::BTreeMap, - marker::Sync, -}; +use std::{collections::BTreeMap, marker::Sync}; /// Builder used for instantiating the multi-contract runner #[derive(Debug, Default)] @@ -221,7 +218,9 @@ impl MultiContractRunner { Ok((name.clone(), result)) }) .filter_map(|x: Result<_>| x.ok()) - .filter_map(|(name, result)| if result.is_empty() { None } else { Some((name, result)) }) + .filter_map( + |(name, result)| if result.is_empty() { None } else { Some((name, result)) }, + ) .map(|(name, result)| { stream_result(name.clone(), result.clone()); (name.clone(), result) From a8176708d8747c576da22cac0a3d3ea86693e80b Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Wed, 23 Feb 2022 18:31:57 +0700 Subject: [PATCH 03/10] Fix clippy warnings --- cli/src/cmd/test.rs | 4 ++-- forge/src/multi_runner.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cli/src/cmd/test.rs b/cli/src/cmd/test.rs index e9a4f8ab359f..5cd9ce191789 100644 --- a/cli/src/cmd/test.rs +++ b/cli/src/cmd/test.rs @@ -415,8 +415,8 @@ fn test( })?; if gas_reporting { - for (_, tests) in results.iter() { - for (_, result) in tests { + for tests in results.values() { + for result in tests.values() { if let (Some(traces), Some(identified_contracts)) = (&result.traces, &result.identified_contracts) { diff --git a/forge/src/multi_runner.rs b/forge/src/multi_runner.rs index d42bd76d2476..4a88b997fbe2 100644 --- a/forge/src/multi_runner.rs +++ b/forge/src/multi_runner.rs @@ -194,7 +194,7 @@ impl MultiContractRunner { pub fn test_stream( &mut self, filter: &(impl TestFilter + Send + Sync), - stream_result: impl Fn(String, BTreeMap) -> () + Sync, + stream_result: impl Fn(String, BTreeMap) + Sync, ) -> Result>> { let contracts = std::mem::take(&mut self.contracts); let vicinity = self.evm_opts.vicinity()?; @@ -223,7 +223,7 @@ impl MultiContractRunner { ) .map(|(name, result)| { stream_result(name.clone(), result.clone()); - (name.clone(), result) + (name, result) }) .collect::>(); From cde74d3aed8bbca891fab8a1a03a70e8158b3d3d Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Wed, 23 Feb 2022 18:40:57 +0700 Subject: [PATCH 04/10] Added doc to mutex --- cli/src/cmd/test.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/cli/src/cmd/test.rs b/cli/src/cmd/test.rs index 5cd9ce191789..94d067921c4f 100644 --- a/cli/src/cmd/test.rs +++ b/cli/src/cmd/test.rs @@ -333,6 +333,7 @@ fn test( let known_contracts = runner.known_contracts.clone(); let index = Arc::new(Mutex::new(0)); let results = runner.test_stream(&filter, |contract_name, tests| { + // This mutex syncs stdout per contract, do not remove even if the newline below is let mut index = index.lock().unwrap(); if *index > 0 { println!() From 42a823bbecacf7fa3453e84e56c3ac88ae45eb1f Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Wed, 23 Feb 2022 18:58:30 +0700 Subject: [PATCH 05/10] Made compilation messages consistent and fixed broken tests --- cli/README.md | 4 ++-- cli/src/cmd/mod.rs | 8 ++++---- cli/tests/cmd.rs | 8 ++++---- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/cli/README.md b/cli/README.md index 09e4e90462c0..a39db0636218 100644 --- a/cli/README.md +++ b/cli/README.md @@ -265,7 +265,7 @@ You can optionally specify a regular expression, to only run matching functions: ```bash $ forge test -m Cannot $HOME/oss/foundry/target/release/forge test -m Cannot -no files changed, compilation skipped. +no files changed, compilation skipped Running 1 test for "Greet.json":Greet [PASS] testCannotGm (gas: 6819) @@ -279,7 +279,7 @@ the `--json` flag ```bash $ forge test --json -no files changed, compilation skipped. +no files changed, compilation skipped {"\"Gm.json\":Gm":{"testNonOwnerCannotGm":{"success":true,"reason":null,"gas_used":3782,"counterexample":null,"logs":[]},"testOwnerCannotGmOnBadBlocks":{"success":true,"reason":null,"gas_used":7771,"counterexample":null,"logs":[]},"testOwnerCanGmOnGoodBlocks":{"success":true,"reason":null,"gas_used":31696,"counterexample":null,"logs":[]}},"\"Greet.json\":Greet":{"testWorksForAllGreetings":{"success":true,"reason":null,"gas_used":null,"counterexample":null,"logs":[]},"testCannotGm":{"success":true,"reason":null,"gas_used":6819,"counterexample":null,"logs":[]},"testCanSetGreeting":{"success":true,"reason":null,"gas_used":31070,"counterexample":null,"logs":[]}}} ``` diff --git a/cli/src/cmd/mod.rs b/cli/src/cmd/mod.rs index d5ac5a7a225b..c9be901dbf84 100644 --- a/cli/src/cmd/mod.rs +++ b/cli/src/cmd/mod.rs @@ -86,22 +86,22 @@ If you are in a subdirectory in a Git repository, try adding `--root .`"#, if output.has_compiler_errors() { eyre::bail!(output.to_string()) } else if output.is_unchanged() { - println!("No files changed, compilation skipped."); + println!("No files changed, compilation skipped"); } else { println!("{}", output); - println!("Success."); + println!("Success"); } Ok(output) } /// Compile a set of files not necessarily included in the `project`'s source dir pub fn compile_files(project: &Project, files: Vec) -> eyre::Result { - println!("compiling..."); + println!("Compiling..."); let output = project.compile_files(files)?; if output.has_compiler_errors() { eyre::bail!(output.to_string()) } - println!("success."); + println!("Success"); Ok(output) } diff --git a/cli/tests/cmd.rs b/cli/tests/cmd.rs index 3715932ec250..e0451646c24e 100644 --- a/cli/tests/cmd.rs +++ b/cli/tests/cmd.rs @@ -282,11 +282,11 @@ contract Greeter {} cmd.arg("build"); assert!(cmd.stdout_lossy().ends_with( - "compiling... + "Compiling... Compiling 1 files with 0.8.10 Compilation finished successfully Compiler run successful -success. +Success ", )); }); @@ -335,11 +335,11 @@ library FooLib { cmd.arg("build"); assert_eq!( - "compiling... + "Compiling... Compiling 2 files with 0.8.10 Compilation finished successfully Compiler run successful -success. +Success ", cmd.stdout_lossy() ); From 1aa10619359cedaa97979b94b214359994c84a08 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Wed, 23 Feb 2022 21:31:32 +0700 Subject: [PATCH 06/10] Combine test and test_stream --- cli/src/cmd/test.rs | 162 +++++++++++++++++++------------------- forge/src/lib.rs | 2 +- forge/src/multi_runner.rs | 44 ++--------- 3 files changed, 91 insertions(+), 117 deletions(-) diff --git a/cli/src/cmd/test.rs b/cli/src/cmd/test.rs index 94d067921c4f..8969e4029f21 100644 --- a/cli/src/cmd/test.rs +++ b/cli/src/cmd/test.rs @@ -10,7 +10,7 @@ use ethers::solc::{ArtifactOutput, Project}; use evm_adapters::{ call_tracing::ExecutionInfo, evm_opts::EvmOpts, gas_report::GasReport, sputnik::helpers::vm, }; -use forge::{MultiContractRunnerBuilder, TestFilter}; +use forge::{MultiContractRunnerBuilder, TestFilter, TestResult, TestResultStreamFn}; use foundry_config::{figment::Figment, Config}; use regex::Regex; use std::{ @@ -322,7 +322,7 @@ fn test( let mut runner = builder.build(project, evm_opts)?; if json { - let results = runner.test(&filter)?; + let results = runner.test(&filter, None)?; let res = serde_json::to_string(&results)?; // TODO: Make this work normally println!("{}", res); Ok(TestOutcome::new(results, allow_failure)) @@ -332,88 +332,92 @@ fn test( let execution_info = runner.execution_info.clone(); let known_contracts = runner.known_contracts.clone(); let index = Arc::new(Mutex::new(0)); - let results = runner.test_stream(&filter, |contract_name, tests| { - // This mutex syncs stdout per contract, do not remove even if the newline below is - let mut index = index.lock().unwrap(); - if *index > 0 { - println!() - } - *index += 1; - if !tests.is_empty() { - let term = if tests.len() > 1 { "tests" } else { "test" }; - println!("Running {} {} for {}", tests.len(), term, contract_name); - } - for (name, result) in tests { - short_test_result(&name, &result); - // adds a linebreak only if there were any traces or logs, so that the - // output does not look like 1 big block. - let mut add_newline = false; - if verbosity > 1 && !result.logs.is_empty() { - add_newline = true; - println!("Logs:"); - for log in &result.logs { - println!(" {}", log); - } + + let results = runner.test( + &filter, + Some(Box::new(move |contract_name: String, tests: BTreeMap| { + // This mutex syncs stdout per contract, do not remove even if the newline below is + let mut index = index.lock().unwrap(); + if *index > 0 { + println!() } - if verbosity > 2 { - if let (Some(traces), Some(identified_contracts)) = - (&result.traces, &result.identified_contracts) - { - if !result.success && verbosity == 3 || verbosity > 3 { - // add a new line if any logs were printed & to separate them from - // the traces to be printed - if !result.logs.is_empty() { - println!(); - } - let mut ident = identified_contracts.clone(); - let mut exec_info = ExecutionInfo::new( - &known_contracts, - &mut ident, - &result.labeled_addresses, - &execution_info.0, - &execution_info.1, - &execution_info.2, - ); - let vm = vm(); - let mut trace_string = "".to_string(); - if verbosity > 4 || !result.success { - add_newline = true; - println!("Traces:"); - // print setup calls as well - traces.iter().for_each(|trace| { - trace.construct_trace_string( - 0, - &mut exec_info, - &vm, - " ", - &mut trace_string, - ); - }); - } else if !traces.is_empty() { - add_newline = true; - println!("Traces:"); - traces - .last() - .expect("no last but not empty") - .construct_trace_string( - 0, - &mut exec_info, - &vm, - " ", - &mut trace_string, - ); - } - if !trace_string.is_empty() { - println!("{}", trace_string); + *index += 1; + if !tests.is_empty() { + let term = if tests.len() > 1 { "tests" } else { "test" }; + println!("Running {} {} for {}", tests.len(), term, contract_name); + } + for (name, result) in tests { + short_test_result(&name, &result); + // adds a linebreak only if there were any traces or logs, so that the + // output does not look like 1 big block. + let mut add_newline = false; + if verbosity > 1 && !result.logs.is_empty() { + add_newline = true; + println!("Logs:"); + for log in &result.logs { + println!(" {}", log); + } + } + if verbosity > 2 { + if let (Some(traces), Some(identified_contracts)) = + (&result.traces, &result.identified_contracts) + { + if !result.success && verbosity == 3 || verbosity > 3 { + // add a new line if any logs were printed & to separate them from + // the traces to be printed + if !result.logs.is_empty() { + println!(); + } + let mut ident = identified_contracts.clone(); + let mut exec_info = ExecutionInfo::new( + &known_contracts, + &mut ident, + &result.labeled_addresses, + &execution_info.0, + &execution_info.1, + &execution_info.2, + ); + let vm = vm(); + let mut trace_string = "".to_string(); + if verbosity > 4 || !result.success { + add_newline = true; + println!("Traces:"); + // print setup calls as well + traces.iter().for_each(|trace| { + trace.construct_trace_string( + 0, + &mut exec_info, + &vm, + " ", + &mut trace_string, + ); + }); + } else if !traces.is_empty() { + add_newline = true; + println!("Traces:"); + traces + .last() + .expect("no last but not empty") + .construct_trace_string( + 0, + &mut exec_info, + &vm, + " ", + &mut trace_string, + ); + } + if !trace_string.is_empty() { + println!("{}", trace_string); + } } } } + if add_newline { + println!(); + } } - if add_newline { - println!(); - } - } - })?; + }) as TestResultStreamFn), + )?; if gas_reporting { for tests in results.values() { diff --git a/forge/src/lib.rs b/forge/src/lib.rs index 6d288439f72f..fab4cd3fe55f 100644 --- a/forge/src/lib.rs +++ b/forge/src/lib.rs @@ -2,7 +2,7 @@ mod runner; pub use runner::{ContractRunner, TestKind, TestKindGas, TestResult}; mod multi_runner; -pub use multi_runner::{MultiContractRunner, MultiContractRunnerBuilder}; +pub use multi_runner::{MultiContractRunner, MultiContractRunnerBuilder, TestResultStreamFn}; pub trait TestFilter { fn matches_test(&self, test_name: impl AsRef) -> bool; diff --git a/forge/src/multi_runner.rs b/forge/src/multi_runner.rs index 4a88b997fbe2..4bf9980c546e 100644 --- a/forge/src/multi_runner.rs +++ b/forge/src/multi_runner.rs @@ -166,6 +166,8 @@ impl MultiContractRunnerBuilder { } } +pub type TestResultStreamFn = Box) + Sync + Send>; + /// A multi contract runner receives a set of contracts deployed in an EVM instance and proceeds /// to run all test functions in these contracts. pub struct MultiContractRunner { @@ -191,10 +193,10 @@ pub struct MultiContractRunner { } impl MultiContractRunner { - pub fn test_stream( + pub fn test( &mut self, filter: &(impl TestFilter + Send + Sync), - stream_result: impl Fn(String, BTreeMap) + Sync, + stream_result: Option, ) -> Result>> { let contracts = std::mem::take(&mut self.contracts); let vicinity = self.evm_opts.vicinity()?; @@ -222,7 +224,9 @@ impl MultiContractRunner { |(name, result)| if result.is_empty() { None } else { Some((name, result)) }, ) .map(|(name, result)| { - stream_result(name.clone(), result.clone()); + if let Some(stream_result) = stream_result.as_ref() { + stream_result(name.clone(), result.clone()); + } (name, result) }) .collect::>(); @@ -231,40 +235,6 @@ impl MultiContractRunner { Ok(results) } - pub fn test( - &mut self, - filter: &(impl TestFilter + Send + Sync), - ) -> Result>> { - let contracts = std::mem::take(&mut self.contracts); - let vicinity = self.evm_opts.vicinity()?; - let backend = self.evm_opts.backend(&vicinity)?; - let source_paths = self.source_paths.clone(); - - let results = contracts - .par_iter() - .filter(|(name, _)| filter.matches_path(source_paths.get(*name).unwrap())) - .filter(|(name, _)| filter.matches_contract(name)) - .map(|(name, (abi, deploy_code, libs))| { - // unavoidable duplication here? - let result = match backend { - BackendKind::Simple(ref backend) => { - self.run_tests(name, abi, backend, deploy_code.clone(), libs, filter)? - } - BackendKind::Shared(ref backend) => { - self.run_tests(name, abi, backend, deploy_code.clone(), libs, filter)? - } - }; - Ok((name.clone(), result)) - }) - .filter_map(|x: Result<_>| x.ok()) - .filter_map(|(name, res)| if res.is_empty() { None } else { Some((name, res)) }) - .collect::>(); - - self.contracts = contracts; - - Ok(results) - } - // The _name field is unused because we only want it for tracing #[tracing::instrument( name = "contract", From 84b6b872dc56714c50e06ec0a229d68dcbd73e49 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Thu, 24 Feb 2022 08:19:22 +0700 Subject: [PATCH 07/10] Fix broken tests --- forge/src/multi_runner.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/forge/src/multi_runner.rs b/forge/src/multi_runner.rs index 4bf9980c546e..3741dbc11a70 100644 --- a/forge/src/multi_runner.rs +++ b/forge/src/multi_runner.rs @@ -293,7 +293,7 @@ mod tests { fn test_multi_runner() { let mut runner = runner(); - let results = runner.test(&Filter::matches_all()).unwrap(); + let results = runner.test(&Filter::matches_all(), None).unwrap(); // 9 contracts being built assert_eq!(results.keys().len(), 9); @@ -309,7 +309,7 @@ mod tests { // can also filter let filter = Filter::new("testGm.*", ".*", ".*"); - let only_gm = runner.test(&filter).unwrap(); + let only_gm = runner.test(&filter, None).unwrap(); assert_eq!(only_gm.len(), 1); assert_eq!(only_gm["GmTest.json:GmTest"].len(), 1); @@ -318,7 +318,7 @@ mod tests { fn test_abstract_contract() { let mut runner = runner(); - let results = runner.test(&Filter::matches_all()).unwrap(); + let results = runner.test(&Filter::matches_all(), None).unwrap(); assert!(results.get("Tests.json:Tests").is_none()); assert!(results.get("ATests.json:ATests").is_some()); assert!(results.get("BTests.json:BTests").is_some()); @@ -331,7 +331,7 @@ mod tests { #[test] fn test_sputnik_debug_logs() { let mut runner = runner(); - let results = runner.test(&Filter::matches_all()).unwrap(); + let results = runner.test(&Filter::matches_all(), None).unwrap(); let reasons = results["DebugLogsTest.json:DebugLogsTest"] .iter() From ae918ef93c2cf177905bdebad3bab8f4a574c041 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Thu, 24 Feb 2022 08:52:19 +0700 Subject: [PATCH 08/10] Fix more broken tests --- cli/tests/cmd.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cli/tests/cmd.rs b/cli/tests/cmd.rs index a08874d8edc0..d0b9b2fa8e61 100644 --- a/cli/tests/cmd.rs +++ b/cli/tests/cmd.rs @@ -380,11 +380,11 @@ contract Foo { prj.write_config(config); assert!(cmd.stdout_lossy().ends_with( - "compiling... + "Compiling... Compiling 1 files with 0.8.10 Compilation finished successfully Compiler run successful -success. +Success ", )); }); @@ -412,10 +412,10 @@ contract Demo { let output = cmd.stdout_lossy(); assert_eq!( format!( - "compiling... + "Compiling... Compiling 1 files with 0.8.10 Compilation finished successfully -success. +Success {} Gas Used: 1751 == Logs == From ab5492fe494a3e19689d6065e8283c5f9a954157 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Thu, 24 Feb 2022 11:10:23 +0700 Subject: [PATCH 09/10] Refactored MultiContractRunner::test to accept mpsc::Sender instead of a closure --- cli/src/cmd/test.rs | 169 ++++++++++++++++++-------------------- forge/src/lib.rs | 2 +- forge/src/multi_runner.rs | 10 +-- 3 files changed, 84 insertions(+), 97 deletions(-) diff --git a/cli/src/cmd/test.rs b/cli/src/cmd/test.rs index 8969e4029f21..9e52e6183d5b 100644 --- a/cli/src/cmd/test.rs +++ b/cli/src/cmd/test.rs @@ -10,14 +10,10 @@ use ethers::solc::{ArtifactOutput, Project}; use evm_adapters::{ call_tracing::ExecutionInfo, evm_opts::EvmOpts, gas_report::GasReport, sputnik::helpers::vm, }; -use forge::{MultiContractRunnerBuilder, TestFilter, TestResult, TestResultStreamFn}; +use forge::{MultiContractRunnerBuilder, TestFilter, TestResult}; use foundry_config::{figment::Figment, Config}; use regex::Regex; -use std::{ - collections::BTreeMap, - str::FromStr, - sync::{Arc, Mutex}, -}; +use std::{collections::BTreeMap, str::FromStr, sync::mpsc::channel}; #[derive(Debug, Clone, Parser)] pub struct Filter { @@ -329,95 +325,88 @@ fn test( } else { // Dapptools-style printing of test results let mut gas_report = GasReport::new(gas_reports.1); - let execution_info = runner.execution_info.clone(); - let known_contracts = runner.known_contracts.clone(); - let index = Arc::new(Mutex::new(0)); - - let results = runner.test( - &filter, - Some(Box::new(move |contract_name: String, tests: BTreeMap| { - // This mutex syncs stdout per contract, do not remove even if the newline below is - let mut index = index.lock().unwrap(); - if *index > 0 { - println!() - } - *index += 1; - if !tests.is_empty() { - let term = if tests.len() > 1 { "tests" } else { "test" }; - println!("Running {} {} for {}", tests.len(), term, contract_name); - } - for (name, result) in tests { - short_test_result(&name, &result); - // adds a linebreak only if there were any traces or logs, so that the - // output does not look like 1 big block. - let mut add_newline = false; - if verbosity > 1 && !result.logs.is_empty() { - add_newline = true; - println!("Logs:"); - for log in &result.logs { - println!(" {}", log); - } + let (tx, rx) = channel::<(String, BTreeMap)>(); + let results = runner.test(&filter, Some(tx))?; + + while let Ok((contract_name, tests)) = rx.recv() { + let (funcs, events, errors) = &runner.execution_info; + + println!(); + if !tests.is_empty() { + let term = if tests.len() > 1 { "tests" } else { "test" }; + println!("Running {} {} for {}", tests.len(), term, contract_name); + } + for (name, result) in tests { + short_test_result(&name, &result); + // adds a linebreak only if there were any traces or logs, so that the + // output does not look like 1 big block. + let mut add_newline = false; + if verbosity > 1 && !result.logs.is_empty() { + add_newline = true; + println!("Logs:"); + for log in &result.logs { + println!(" {}", log); } - if verbosity > 2 { - if let (Some(traces), Some(identified_contracts)) = - (&result.traces, &result.identified_contracts) - { - if !result.success && verbosity == 3 || verbosity > 3 { - // add a new line if any logs were printed & to separate them from - // the traces to be printed - if !result.logs.is_empty() { - println!(); - } - let mut ident = identified_contracts.clone(); - let mut exec_info = ExecutionInfo::new( - &known_contracts, - &mut ident, - &result.labeled_addresses, - &execution_info.0, - &execution_info.1, - &execution_info.2, - ); - let vm = vm(); - let mut trace_string = "".to_string(); - if verbosity > 4 || !result.success { - add_newline = true; - println!("Traces:"); - // print setup calls as well - traces.iter().for_each(|trace| { - trace.construct_trace_string( - 0, - &mut exec_info, - &vm, - " ", - &mut trace_string, - ); - }); - } else if !traces.is_empty() { - add_newline = true; - println!("Traces:"); - traces - .last() - .expect("no last but not empty") - .construct_trace_string( - 0, - &mut exec_info, - &vm, - " ", - &mut trace_string, - ); - } - if !trace_string.is_empty() { - println!("{}", trace_string); - } + } + if verbosity > 2 { + if let (Some(traces), Some(identified_contracts)) = + (&result.traces, &result.identified_contracts) + { + if !result.success && verbosity == 3 || verbosity > 3 { + // add a new line if any logs were printed & to separate them from + // the traces to be printed + if !result.logs.is_empty() { + println!(); + } + let mut ident = identified_contracts.clone(); + let mut exec_info = ExecutionInfo::new( + &runner.known_contracts, + &mut ident, + &result.labeled_addresses, + funcs, + events, + errors, + ); + let vm = vm(); + let mut trace_string = "".to_string(); + if verbosity > 4 || !result.success { + add_newline = true; + println!("Traces:"); + // print setup calls as well + traces.iter().for_each(|trace| { + trace.construct_trace_string( + 0, + &mut exec_info, + &vm, + " ", + &mut trace_string, + ); + }); + } else if !traces.is_empty() { + add_newline = true; + println!("Traces:"); + traces + .last() + .expect("no last but not empty") + .construct_trace_string( + 0, + &mut exec_info, + &vm, + " ", + &mut trace_string, + ); + } + if !trace_string.is_empty() { + println!("{}", trace_string); } } } - if add_newline { - println!(); - } } - }) as TestResultStreamFn), - )?; + if add_newline { + println!(); + } + } + } if gas_reporting { for tests in results.values() { diff --git a/forge/src/lib.rs b/forge/src/lib.rs index fab4cd3fe55f..6d288439f72f 100644 --- a/forge/src/lib.rs +++ b/forge/src/lib.rs @@ -2,7 +2,7 @@ mod runner; pub use runner::{ContractRunner, TestKind, TestKindGas, TestResult}; mod multi_runner; -pub use multi_runner::{MultiContractRunner, MultiContractRunnerBuilder, TestResultStreamFn}; +pub use multi_runner::{MultiContractRunner, MultiContractRunnerBuilder}; pub trait TestFilter { fn matches_test(&self, test_name: impl AsRef) -> bool; diff --git a/forge/src/multi_runner.rs b/forge/src/multi_runner.rs index 3741dbc11a70..d37e919e856e 100644 --- a/forge/src/multi_runner.rs +++ b/forge/src/multi_runner.rs @@ -17,7 +17,7 @@ use proptest::test_runner::TestRunner; use eyre::Result; use rayon::prelude::*; -use std::{collections::BTreeMap, marker::Sync}; +use std::{collections::BTreeMap, marker::Sync, sync::mpsc::Sender}; /// Builder used for instantiating the multi-contract runner #[derive(Debug, Default)] @@ -166,8 +166,6 @@ impl MultiContractRunnerBuilder { } } -pub type TestResultStreamFn = Box) + Sync + Send>; - /// A multi contract runner receives a set of contracts deployed in an EVM instance and proceeds /// to run all test functions in these contracts. pub struct MultiContractRunner { @@ -196,7 +194,7 @@ impl MultiContractRunner { pub fn test( &mut self, filter: &(impl TestFilter + Send + Sync), - stream_result: Option, + stream_result: Option)>>, ) -> Result>> { let contracts = std::mem::take(&mut self.contracts); let vicinity = self.evm_opts.vicinity()?; @@ -223,9 +221,9 @@ impl MultiContractRunner { .filter_map( |(name, result)| if result.is_empty() { None } else { Some((name, result)) }, ) - .map(|(name, result)| { + .map_with(stream_result, |stream_result, (name, result)| { if let Some(stream_result) = stream_result.as_ref() { - stream_result(name.clone(), result.clone()); + stream_result.send((name.clone(), result.clone())).unwrap(); } (name, result) }) From d4ead3f623d6e8ac04ea10e1dfcdec077edb8ab5 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Fri, 25 Feb 2022 01:01:05 +0700 Subject: [PATCH 10/10] Fix unintentional blocking in streaming of test results --- cli/src/cmd/test.rs | 157 +++++++++++++++++++++++--------------------- 1 file changed, 82 insertions(+), 75 deletions(-) diff --git a/cli/src/cmd/test.rs b/cli/src/cmd/test.rs index 9e52e6183d5b..cdcafa4f5c6b 100644 --- a/cli/src/cmd/test.rs +++ b/cli/src/cmd/test.rs @@ -13,7 +13,7 @@ use evm_adapters::{ use forge::{MultiContractRunnerBuilder, TestFilter, TestResult}; use foundry_config::{figment::Figment, Config}; use regex::Regex; -use std::{collections::BTreeMap, str::FromStr, sync::mpsc::channel}; +use std::{collections::BTreeMap, str::FromStr, sync::mpsc::channel, thread}; #[derive(Debug, Clone, Parser)] pub struct Filter { @@ -326,87 +326,94 @@ fn test( // Dapptools-style printing of test results let mut gas_report = GasReport::new(gas_reports.1); let (tx, rx) = channel::<(String, BTreeMap)>(); - let results = runner.test(&filter, Some(tx))?; - - while let Ok((contract_name, tests)) = rx.recv() { - let (funcs, events, errors) = &runner.execution_info; + let known_contracts = runner.known_contracts.clone(); + let execution_info = runner.execution_info.clone(); - println!(); - if !tests.is_empty() { - let term = if tests.len() > 1 { "tests" } else { "test" }; - println!("Running {} {} for {}", tests.len(), term, contract_name); - } - for (name, result) in tests { - short_test_result(&name, &result); - // adds a linebreak only if there were any traces or logs, so that the - // output does not look like 1 big block. - let mut add_newline = false; - if verbosity > 1 && !result.logs.is_empty() { - add_newline = true; - println!("Logs:"); - for log in &result.logs { - println!(" {}", log); - } + let handle = thread::spawn(move || { + while let Ok((contract_name, tests)) = rx.recv() { + println!(); + if !tests.is_empty() { + let term = if tests.len() > 1 { "tests" } else { "test" }; + println!("Running {} {} for {}", tests.len(), term, contract_name); } - if verbosity > 2 { - if let (Some(traces), Some(identified_contracts)) = - (&result.traces, &result.identified_contracts) - { - if !result.success && verbosity == 3 || verbosity > 3 { - // add a new line if any logs were printed & to separate them from - // the traces to be printed - if !result.logs.is_empty() { - println!(); - } - let mut ident = identified_contracts.clone(); - let mut exec_info = ExecutionInfo::new( - &runner.known_contracts, - &mut ident, - &result.labeled_addresses, - funcs, - events, - errors, - ); - let vm = vm(); - let mut trace_string = "".to_string(); - if verbosity > 4 || !result.success { - add_newline = true; - println!("Traces:"); - // print setup calls as well - traces.iter().for_each(|trace| { - trace.construct_trace_string( - 0, - &mut exec_info, - &vm, - " ", - &mut trace_string, - ); - }); - } else if !traces.is_empty() { - add_newline = true; - println!("Traces:"); - traces - .last() - .expect("no last but not empty") - .construct_trace_string( - 0, - &mut exec_info, - &vm, - " ", - &mut trace_string, - ); - } - if !trace_string.is_empty() { - println!("{}", trace_string); + for (name, result) in tests { + short_test_result(&name, &result); + // adds a linebreak only if there were any traces or logs, so that the + // output does not look like 1 big block. + let mut add_newline = false; + if verbosity > 1 && !result.logs.is_empty() { + add_newline = true; + println!("Logs:"); + for log in &result.logs { + println!(" {}", log); + } + } + if verbosity > 2 { + if let (Some(traces), Some(identified_contracts)) = + (&result.traces, &result.identified_contracts) + { + if !result.success && verbosity == 3 || verbosity > 3 { + // add a new line if any logs were printed & to separate them from + // the traces to be printed + if !result.logs.is_empty() { + println!(); + } + let mut ident = identified_contracts.clone(); + let (funcs, events, errors) = &execution_info; + let mut exec_info = ExecutionInfo::new( + // &runner.known_contracts, + &known_contracts, + &mut ident, + &result.labeled_addresses, + funcs, + events, + errors, + ); + let vm = vm(); + let mut trace_string = "".to_string(); + if verbosity > 4 || !result.success { + add_newline = true; + println!("Traces:"); + // print setup calls as well + traces.iter().for_each(|trace| { + trace.construct_trace_string( + 0, + &mut exec_info, + &vm, + " ", + &mut trace_string, + ); + }); + } else if !traces.is_empty() { + add_newline = true; + println!("Traces:"); + traces + .last() + .expect("no last but not empty") + .construct_trace_string( + 0, + &mut exec_info, + &vm, + " ", + &mut trace_string, + ); + } + if !trace_string.is_empty() { + println!("{}", trace_string); + } } } } - } - if add_newline { - println!(); + if add_newline { + println!(); + } } } - } + }); + + let results = runner.test(&filter, Some(tx))?; + + handle.join().unwrap(); if gas_reporting { for tests in results.values() {