fix(coverage): sync implementation with forge test (#11801)

Coverage was calling a low level function of forge test args, which
does not setup stuff like config.fuzz.gas_report_samples, EvmArgs, etc.,
as well as missing options in the test runner builder etc.

This means that `forge coverage` falls out of sync gradually with
`forge test`. One of the options which was thus missed,
`gas_report_samples`, controls how many traces are saved from fuzz
and invariant tests, which with the recent addition of tracing for
backtraces means we are storing a lot of traces unnecessarily,
massively increasing memory usage.

Author: DaniPopes

crates/evm/evm/src/executors/mod.rs

@@ -1131,7 +1131,7 @@ impl FuzzTestTimer {
 }
 
 /// Helper struct to enable fail fast behavior: when one test fails, all other tests stop early.
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 pub struct FailFast {
     /// Shared atomic flag set to `true` when a failure occurs.
     /// None if fail-fast is disabled.

crates/forge/src/cmd/coverage.rs

@@ -1,12 +1,9 @@
 use super::{install, test::TestArgs, watch::WatchArgs};
-use crate::{
-    MultiContractRunnerBuilder,
-    coverage::{
-        BytecodeReporter, ContractId, CoverageReport, CoverageReporter, CoverageSummaryReporter,
-        DebugReporter, ItemAnchor, LcovReporter,
-        analysis::{SourceAnalysis, SourceFiles},
-        anchors::find_anchors,
-    },
+use crate::coverage::{
+    BytecodeReporter, ContractId, CoverageReport, CoverageReporter, CoverageSummaryReporter,
+    DebugReporter, ItemAnchor, LcovReporter,
+    analysis::{SourceAnalysis, SourceFiles},
+    anchors::find_anchors,
 };
 use alloy_primitives::{Address, Bytes, U256, map::HashMap};
 use clap::{Parser, ValueEnum, ValueHint};
@@ -16,17 +13,13 @@ use foundry_common::{compile::ProjectCompiler, errors::convert_solar_errors};
 use foundry_compilers::{
     Artifact, ArtifactId, Project, ProjectCompileOutput, ProjectPathsConfig,
     artifacts::{CompactBytecode, CompactDeployedBytecode, SolcLanguage, sourcemap::SourceMap},
-    compilers::multi::MultiCompiler,
 };
 use foundry_config::Config;
 use foundry_evm::opts::EvmOpts;
 use foundry_evm_core::ic::IcPcMap;
 use rayon::prelude::*;
 use semver::{Version, VersionReq};
-use std::{
-    path::{Path, PathBuf},
-    sync::Arc,
-};
+use std::path::{Path, PathBuf};
 
 // Loads project's figment and merges the build cli arguments into it
 foundry_config::impl_figment_convert!(CoverageArgs, test);
@@ -109,7 +102,7 @@ impl CoverageArgs {
         let report = self.prepare(&paths, &mut output)?;
 
         sh_println!("Running tests...")?;
-        self.collect(&paths.root, &output, report, Arc::new(config), evm_opts).await
+        self.collect(&paths.root, &output, report, config, evm_opts).await
     }
 
     fn populate_reporters(&mut self, root: &Path) {
@@ -255,31 +248,19 @@ impl CoverageArgs {
     #[instrument(name = "Coverage::collect", skip_all)]
     async fn collect(
         mut self,
-        root: &Path,
+        project_root: &Path,
         output: &ProjectCompileOutput,
         mut report: CoverageReport,
-        config: Arc<Config>,
+        config: Config,
         evm_opts: EvmOpts,
     ) -> Result<()> {
-        let verbosity = evm_opts.verbosity;
-
-        // Build the contract runner
-        let env = evm_opts.evm_env().await?;
-        let runner = MultiContractRunnerBuilder::new(config.clone())
-            .initial_balance(evm_opts.initial_balance)
-            .evm_spec(config.evm_spec_id())
-            .sender(evm_opts.sender)
-            .with_fork(evm_opts.get_fork(&config, env.clone()))
-            .set_coverage(true)
-            .build::<MultiCompiler>(root, output, env, evm_opts)?;
-
-        let known_contracts = runner.known_contracts.clone();
-
         let filter = self.test.filter(&config)?;
-        let outcome = self.test.run_tests(runner, config, verbosity, &filter, output).await?;
-
+        let outcome =
+            self.test.run_tests(project_root, config, evm_opts, output, &filter, true).await?;
         outcome.ensure_ok(false)?;
 
+        let known_contracts = outcome.runner.as_ref().unwrap().known_contracts.clone();
+
         // Add hit data to the coverage report
         let data = outcome.results.iter().flat_map(|(_, suite)| {
             let mut hits = Vec::new();

crates/forge/src/cmd/snapshot.rs

@@ -94,7 +94,7 @@ impl GasSnapshotArgs {
         // Set fuzz seed so gas snapshots are deterministic
         self.test.fuzz_seed = Some(U256::from_be_bytes(STATIC_FUZZ_SEED));
 
-        let outcome = self.test.execute_tests().await?;
+        let outcome = self.test.compile_and_run().await?;
         outcome.ensure_ok(false)?;
         let tests = self.config.apply(outcome);
 

crates/forge/src/cmd/test/mod.rs

@@ -39,12 +39,15 @@ use foundry_config::{
     filter::GlobMatcher,
 };
 use foundry_debugger::Debugger;
-use foundry_evm::traces::{backtrace::BacktraceBuilder, identifier::TraceIdentifiers};
+use foundry_evm::{
+    opts::EvmOpts,
+    traces::{backtrace::BacktraceBuilder, identifier::TraceIdentifiers},
+};
 use regex::Regex;
 use std::{
     collections::{BTreeMap, BTreeSet},
     fmt::Write,
-    path::PathBuf,
+    path::{Path, PathBuf},
     sync::{Arc, mpsc::channel},
     time::{Duration, Instant},
 };
@@ -199,9 +202,9 @@ pub struct TestArgs {
 }
 
 impl TestArgs {
-    pub async fn run(self) -> Result<TestOutcome> {
+    pub async fn run(mut self) -> Result<TestOutcome> {
         trace!(target: "forge::test", "executing test command");
-        self.execute_tests().await
+        self.compile_and_run().await
     }
 
     /// Returns a list of files that need to be compiled in order to run all the tests that match
@@ -250,18 +253,9 @@ impl TestArgs {
     /// configured filter will be executed
     ///
     /// Returns the test results for all matching tests.
-    pub async fn execute_tests(mut self) -> Result<TestOutcome> {
+    pub async fn compile_and_run(&mut self) -> Result<TestOutcome> {
         // Merge all configs.
-        let (mut config, mut evm_opts) = self.load_config_and_evm_opts()?;
-
-        // Explicitly enable isolation for gas reports for more correct gas accounting.
-        if self.gas_report {
-            evm_opts.isolate = true;
-        } else {
-            // Do not collect gas report traces if gas report is not enabled.
-            config.fuzz.gas_report_samples = 0;
-            config.invariant.gas_report_samples = 0;
-        }
+        let (mut config, evm_opts) = self.load_config_and_evm_opts()?;
 
         // Install missing dependencies.
         if install::install_missing_dependencies(&mut config) && config.auto_detect_remappings {
@@ -281,9 +275,31 @@ impl TestArgs {
             .files(self.get_sources_to_compile(&config, &filter)?);
         let output = compiler.compile(&project)?;
 
-        // Create test options from general project settings and compiler output.
-        let project_root = &project.paths.root;
+        self.run_tests(&project.paths.root, config, evm_opts, &output, &filter, false).await
+    }
 
+    /// Executes all the tests in the project.
+    ///
+    /// See [`Self::compile_and_run`] for more details.
+    pub async fn run_tests(
+        &mut self,
+        project_root: &Path,
+        mut config: Config,
+        mut evm_opts: EvmOpts,
+        output: &ProjectCompileOutput,
+        filter: &ProjectPathsAwareFilter,
+        coverage: bool,
+    ) -> Result<TestOutcome> {
+        // Explicitly enable isolation for gas reports for more correct gas accounting.
+        if self.gas_report {
+            evm_opts.isolate = true;
+        } else {
+            // Do not collect gas report traces if gas report is not enabled.
+            config.fuzz.gas_report_samples = 0;
+            config.invariant.gas_report_samples = 0;
+        }
+
+        // Create test options from general project settings and compiler output.
         let should_debug = self.debug;
         let should_draw = self.flamegraph || self.flamechart;
 
@@ -321,10 +337,11 @@ impl TestArgs {
             .enable_isolation(evm_opts.isolate)
             .networks(evm_opts.networks)
             .fail_fast(self.fail_fast)
-            .build::<MultiCompiler>(project_root, &output, env, evm_opts)?;
+            .set_coverage(coverage)
+            .build::<MultiCompiler>(project_root, output, env, evm_opts)?;
 
         let libraries = runner.libraries.clone();
-        let mut outcome = self.run_tests(runner, config, verbosity, &filter, &output).await?;
+        let mut outcome = self.run_tests_inner(runner, config, verbosity, filter, output).await?;
 
         if should_draw {
             let (suite_name, test_name, mut test_result) =
@@ -373,7 +390,7 @@ impl TestArgs {
                 outcome.remove_first().ok_or_eyre("no tests were executed")?;
 
             let sources =
-                ContractSources::from_project_output(&output, project.root(), Some(&libraries))?;
+                ContractSources::from_project_output(output, project_root, Some(&libraries))?;
 
             // Run the debugger.
             let mut builder = Debugger::builder()
@@ -388,8 +405,8 @@ impl TestArgs {
             }
 
             let mut debugger = builder.build();
-            if let Some(dump_path) = self.dump {
-                debugger.dump_to_file(&dump_path)?;
+            if let Some(dump_path) = &self.dump {
+                debugger.dump_to_file(dump_path)?;
             } else {
                 debugger.try_run_tui()?;
             }
@@ -399,7 +416,7 @@ impl TestArgs {
     }
 
     /// Run all tests that matches the filter predicate from a test runner
-    pub async fn run_tests(
+    async fn run_tests_inner(
         &self,
         mut runner: MultiContractRunner,
         config: Arc<Config>,
@@ -440,7 +457,7 @@ impl TestArgs {
                 }
                 sh_warn!("{msg}")?;
             }
-            return Ok(TestOutcome::empty(false));
+            return Ok(TestOutcome::empty(Some(runner), false));
         }
 
         if num_filtered != 1 && (self.debug || self.flamegraph || self.flamechart) {
@@ -482,13 +499,13 @@ impl TestArgs {
                 }
             });
             sh_println!("{}", serde_json::to_string(&results)?)?;
-            return Ok(TestOutcome::new(results, self.allow_failure));
+            return Ok(TestOutcome::new(Some(runner), results, self.allow_failure));
         }
 
         if self.junit {
             let results = runner.test_collect(filter)?;
             sh_println!("{}", junit_xml_report(&results, verbosity).to_string()?)?;
-            return Ok(TestOutcome::new(results, self.allow_failure));
+            return Ok(TestOutcome::new(Some(runner), results, self.allow_failure));
         }
 
         let remote_chain_id = runner.evm_opts.get_remote_chain_id().await;
@@ -502,7 +519,7 @@ impl TestArgs {
         let show_progress = config.show_progress;
         let handle = tokio::task::spawn_blocking({
             let filter = filter.clone();
-            move || runner.test(&filter, tx, show_progress)
+            move || runner.test(&filter, tx, show_progress).map(|()| runner)
         });
 
         // Set up trace identifiers.
@@ -542,7 +559,7 @@ impl TestArgs {
 
         let mut gas_snapshots = BTreeMap::<String, BTreeMap<String, String>>::new();
 
-        let mut outcome = TestOutcome::empty(self.allow_failure);
+        let mut outcome = TestOutcome::empty(None, self.allow_failure);
 
         let mut any_test_failed = false;
         let mut backtrace_builder = None;
@@ -823,11 +840,12 @@ impl TestArgs {
         }
 
         // Reattach the task.
-        if let Err(e) = handle.await {
-            match e.try_into_panic() {
+        match handle.await {
+            Ok(result) => outcome.runner = Some(result?),
+            Err(e) => match e.try_into_panic() {
                 Ok(payload) => std::panic::resume_unwind(payload),
                 Err(e) => return Err(e.into()),
-            }
+            },
         }
 
         // Persist test run failures to enable replaying.
@@ -919,7 +937,7 @@ fn list(runner: MultiContractRunner, filter: &ProjectPathsAwareFilter) -> Result
             }
         }
     }
-    Ok(TestOutcome::empty(false))
+    Ok(TestOutcome::empty(Some(runner), false))
 }
 
 /// Load persisted filter (with last test run failures) from file.

crates/forge/src/multi_runner.rs

@@ -50,6 +50,7 @@ pub type DeployableContracts = BTreeMap<ArtifactId, TestContract>;
 
 /// A multi contract runner receives a set of contracts deployed in an EVM instance and proceeds
 /// to run all test functions in these contracts.
+#[derive(Clone, Debug)]
 pub struct MultiContractRunner {
     /// Mapping of contract name to JsonAbi, creation bytecode and library bytecode which
     /// needs to be deployed & linked against
@@ -274,7 +275,7 @@ impl MultiContractRunner {
 /// Configuration for the test runner.
 ///
 /// This is modified after instantiation through inline config.
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 pub struct TestRunnerConfig {
     /// Project config.
     pub config: Arc<Config>,

crates/forge/src/result.rs

@@ -1,6 +1,7 @@
 //! Test outcomes.
 
 use crate::{
+    MultiContractRunner,
     fuzz::{BaseCounterExample, FuzzedCases},
     gas_report::GasReport,
 };
@@ -42,17 +43,23 @@ pub struct TestOutcome {
     pub last_run_decoder: Option<CallTraceDecoder>,
     /// The gas report, if requested.
     pub gas_report: Option<GasReport>,
+    /// The runner used to execute the tests.
+    pub runner: Option<MultiContractRunner>,
 }
 
 impl TestOutcome {
     /// Creates a new test outcome with the given results.
-    pub fn new(results: BTreeMap<String, SuiteResult>, allow_failure: bool) -> Self {
-        Self { results, allow_failure, last_run_decoder: None, gas_report: None }
+    pub fn new(
+        runner: Option<MultiContractRunner>,
+        results: BTreeMap<String, SuiteResult>,
+        allow_failure: bool,
+    ) -> Self {
+        Self { results, allow_failure, last_run_decoder: None, gas_report: None, runner }
     }
 
     /// Creates a new empty test outcome.
-    pub fn empty(allow_failure: bool) -> Self {
-        Self::new(BTreeMap::new(), allow_failure)
+    pub fn empty(runner: Option<MultiContractRunner>, allow_failure: bool) -> Self {
+        Self::new(runner, BTreeMap::new(), allow_failure)
     }
 
     /// Returns an iterator over all individual succeeding tests and their names.
@@ -399,6 +406,8 @@ pub struct TestResult {
     pub traces: Traces,
 
     /// Additional traces to use for gas report.
+    ///
+    /// These are cleared after the gas report is analyzed.
     #[serde(skip)]
     pub gas_report_traces: Vec<Vec<CallTraceArena>>,