refactor: get_sources_to_compile (#11464)

* refactor: test filter

* refactor: handling of empty test filters

* dontfail

* fix

Author: DaniPopes

crates/common/src/traits.rs

@@ -8,7 +8,7 @@ use std::{fmt, path::Path};
 /// Test filter.
 pub trait TestFilter: Send + Sync {
     /// Returns whether the test should be included.
-    fn matches_test(&self, test_name: &str) -> bool;
+    fn matches_test(&self, test_signature: &str) -> bool;
 
     /// Returns whether the contract should be included.
     fn matches_contract(&self, contract_name: &str) -> bool;
@@ -17,6 +17,30 @@ pub trait TestFilter: Send + Sync {
     fn matches_path(&self, path: &Path) -> bool;
 }
 
+impl<'a> dyn TestFilter + 'a {
+    /// Returns `true` if the function is a test function that matches the given filter.
+    pub fn matches_test_function(&self, func: &Function) -> bool {
+        func.is_any_test() && self.matches_test(&func.signature())
+    }
+}
+
+/// A test filter that filters out nothing.
+#[derive(Clone, Debug, Default)]
+pub struct EmptyTestFilter(());
+impl TestFilter for EmptyTestFilter {
+    fn matches_test(&self, _test_signature: &str) -> bool {
+        true
+    }
+
+    fn matches_contract(&self, _contract_name: &str) -> bool {
+        true
+    }
+
+    fn matches_path(&self, _path: &Path) -> bool {
+        true
+    }
+}
+
 /// Extension trait for `Function`.
 pub trait TestFunctionExt {
     /// Returns the kind of test function.

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

@@ -106,13 +106,13 @@ impl FileFilter for FilterArgs {
 }
 
 impl TestFilter for FilterArgs {
-    fn matches_test(&self, test_name: &str) -> bool {
+    fn matches_test(&self, test_signature: &str) -> bool {
         let mut ok = true;
         if let Some(re) = &self.test_pattern {
-            ok = ok && re.is_match(test_name);
+            ok = ok && re.is_match(test_signature);
         }
         if let Some(re) = &self.test_pattern_inverse {
-            ok = ok && !re.is_match(test_name);
+            ok = ok && !re.is_match(test_signature);
         }
         ok
     }
@@ -207,8 +207,8 @@ impl FileFilter for ProjectPathsAwareFilter {
 }
 
 impl TestFilter for ProjectPathsAwareFilter {
-    fn matches_test(&self, test_name: &str) -> bool {
-        self.args_filter.matches_test(test_name)
+    fn matches_test(&self, test_signature: &str) -> bool {
+        self.args_filter.matches_test(test_signature)
     }
 
     fn matches_contract(&self, contract_name: &str) -> bool {

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

@@ -1,9 +1,9 @@
 use super::{install, test::filter::ProjectPathsAwareFilter, watch::WatchArgs};
 use crate::{
-    MultiContractRunner, MultiContractRunnerBuilder, TestFilter,
+    MultiContractRunner, MultiContractRunnerBuilder,
     decode::decode_console_logs,
     gas_report::GasReport,
-    multi_runner::matches_contract,
+    multi_runner::{is_test_contract, matches_artifact},
     result::{SuiteResult, TestOutcome, TestStatus},
     traces::{
         CallTraceDecoderBuilder, InternalTraceMode, TraceKind,
@@ -21,15 +21,12 @@ use foundry_cli::{
     opts::{BuildOpts, GlobalArgs},
     utils::{self, LoadConfig},
 };
-use foundry_common::{TestFunctionExt, compile::ProjectCompiler, evm::EvmArgs, fs, shell};
+use foundry_common::{
+    EmptyTestFilter, TestFunctionExt, compile::ProjectCompiler, evm::EvmArgs, fs, shell,
+};
 use foundry_compilers::{
-    ProjectCompileOutput,
-    artifacts::output_selection::OutputSelection,
-    compilers::{
-        Language,
-        multi::{MultiCompiler, MultiCompilerLanguage},
-    },
-    utils::source_files_iter,
+    ProjectCompileOutput, artifacts::output_selection::OutputSelection,
+    compilers::multi::MultiCompiler,
 };
 use foundry_config::{
     Config, figment,
@@ -209,76 +206,44 @@ impl TestArgs {
         self.execute_tests().await
     }
 
-    /// Returns sources which include any tests to be executed.
-    /// If no filters are provided, sources are filtered by existence of test/invariant methods in
-    /// them, If filters are provided, sources are additionally filtered by them.
+    /// Returns a list of files that need to be compiled in order to run all the tests that match
+    /// the given filter.
+    ///
+    /// This means that it will return all sources that are not test contracts or that match the
+    /// filter. We want to compile all non-test sources always because tests might depend on them
+    /// dynamically through cheatcodes.
+    ///
+    /// Returns `None` if all sources should be compiled.
     #[instrument(target = "forge::test", skip_all)]
     pub fn get_sources_to_compile(
         &self,
         config: &Config,
-        filter: &ProjectPathsAwareFilter,
-    ) -> Result<BTreeSet<PathBuf>> {
+        test_filter: &ProjectPathsAwareFilter,
+    ) -> Result<Option<BTreeSet<PathBuf>>> {
+        // An empty filter doesn't filter out anything.
+        if test_filter.is_empty() {
+            return Ok(None);
+        }
+
         let mut project = config.create_project(true, true)?;
         project.update_output_selection(|selection| {
             *selection = OutputSelection::common_output_selection(["abi".to_string()]);
         });
-
         let output = project.compile()?;
-
         if output.has_compiler_errors() {
             sh_println!("{output}")?;
             eyre::bail!("Compilation failed");
         }
 
-        // ABIs of all sources
-        let abis = output
-            .into_artifacts()
-            .filter_map(|(id, artifact)| artifact.abi.map(|abi| (id, abi)))
-            .collect::<BTreeMap<_, _>>();
-
-        // Filter sources by their abis and contract names.
-        let mut test_sources = abis
-            .iter()
-            .filter(|(id, abi)| matches_contract(id, abi, filter))
-            .map(|(id, _)| id.source.clone())
+        let sources = output
+            .artifact_ids()
+            .filter_map(|(id, artifact)| artifact.abi.as_ref().map(|abi| (id, abi)))
+            .filter(|(id, abi)| {
+                !is_test_contract(abi.functions()) || matches_artifact(test_filter, id, abi)
+            })
+            .map(|(id, _)| id.source)
             .collect::<BTreeSet<_>>();
-
-        if test_sources.is_empty() {
-            if filter.is_empty() {
-                sh_println!(
-                    "No tests found in project! \
-                        Forge looks for functions that starts with `test`."
-                )?;
-            } else {
-                sh_println!("No tests match the provided pattern:")?;
-                sh_print!("{filter}")?;
-
-                // Try to suggest a test when there's no match
-                if let Some(test_pattern) = &filter.args().test_pattern {
-                    let test_name = test_pattern.as_str();
-                    let candidates = abis
-                        .into_iter()
-                        .filter(|(id, _)| {
-                            filter.matches_path(&id.source) && filter.matches_contract(&id.name)
-                        })
-                        .flat_map(|(_, abi)| abi.functions.into_keys())
-                        .collect::<Vec<_>>();
-                    if let Some(suggestion) = utils::did_you_mean(test_name, candidates).pop() {
-                        sh_println!("\nDid you mean `{suggestion}`?")?;
-                    }
-                }
-            }
-
-            eyre::bail!("No tests to run");
-        }
-
-        // Always recompile all sources to ensure that `getCode` cheatcode can use any artifact.
-        test_sources.extend(source_files_iter(
-            &project.paths.sources,
-            MultiCompilerLanguage::FILE_EXTENSIONS,
-        ));
-
-        Ok(test_sources)
+        Ok(Some(sources))
     }
 
     /// Executes all the tests in the project.
@@ -312,13 +277,10 @@ impl TestArgs {
         let filter = self.filter(&config)?;
         trace!(target: "forge::test", ?filter, "using filter");
 
-        let sources_to_compile = self.get_sources_to_compile(&config, &filter)?;
-
         let compiler = ProjectCompiler::new()
             .dynamic_test_linking(config.dynamic_test_linking)
             .quiet(shell::is_json() || self.junit)
-            .files(sources_to_compile);
-
+            .files(self.get_sources_to_compile(&config, &filter)?.unwrap_or_default());
         let output = compiler.compile(&project)?;
 
         // Create test options from general project settings and compiler output.
@@ -457,6 +419,32 @@ impl TestArgs {
         let silent = self.gas_report && shell::is_json() || self.summary && shell::is_json();
 
         let num_filtered = runner.matching_test_functions(filter).count();
+
+        if num_filtered == 0 {
+            let mut total_tests = num_filtered;
+            if !filter.is_empty() {
+                total_tests = runner.matching_test_functions(&EmptyTestFilter::default()).count();
+            }
+            if total_tests == 0 {
+                sh_println!(
+                    "No tests found in project! Forge looks for functions that start with `test`"
+                )?;
+            } else {
+                let mut msg = format!("no tests match the provided pattern:\n{filter}");
+                // Try to suggest a test when there's no match.
+                if let Some(test_pattern) = &filter.args().test_pattern {
+                    let test_name = test_pattern.as_str();
+                    // Filter contracts but not test functions.
+                    let candidates = runner.all_test_functions(filter).map(|f| &f.name);
+                    if let Some(suggestion) = utils::did_you_mean(test_name, candidates).pop() {
+                        write!(msg, "\nDid you mean `{suggestion}`?")?;
+                    }
+                }
+                sh_warn!("{msg}")?;
+            }
+            return Ok(TestOutcome::empty(false));
+        }
+
         if num_filtered != 1 && (self.debug || self.flamegraph || self.flamechart) {
             let action = if self.flamegraph {
                 "generate a flamegraph"
@@ -915,7 +903,14 @@ fn list(runner: MultiContractRunner, filter: &ProjectPathsAwareFilter) -> Result
 /// Load persisted filter (with last test run failures) from file.
 fn last_run_failures(config: &Config) -> Option<regex::Regex> {
     match fs::read_to_string(&config.test_failures_file) {
-        Ok(filter) => Some(Regex::new(&filter).unwrap()),
+        Ok(filter) => Regex::new(&filter)
+            .inspect_err(|e| {
+                _ = sh_warn!(
+                    "failed to parse test filter from {:?}: {e}",
+                    config.test_failures_file
+                )
+            })
+            .ok(),
         Err(_) => None,
     }
 }

crates/forge/src/multi_runner.rs

@@ -89,7 +89,7 @@ impl MultiContractRunner {
         &'a self,
         filter: &'b dyn TestFilter,
     ) -> impl Iterator<Item = (&'a ArtifactId, &'a TestContract)> + 'b {
-        self.contracts.iter().filter(|&(id, c)| matches_contract(id, &c.abi, filter))
+        self.contracts.iter().filter(|&(id, c)| matches_artifact(filter, id, &c.abi))
     }
 
     /// Returns an iterator over all test functions that match the filter.
@@ -99,7 +99,7 @@ impl MultiContractRunner {
     ) -> impl Iterator<Item = &'a Function> + 'b {
         self.matching_contracts(filter)
             .flat_map(|(_, c)| c.abi.functions())
-            .filter(|func| is_matching_test(func, filter))
+            .filter(|func| filter.matches_test_function(func))
     }
 
     /// Returns an iterator over all test functions in contracts that match the filter.
@@ -123,7 +123,7 @@ impl MultiContractRunner {
                 let tests = c
                     .abi
                     .functions()
-                    .filter(|func| is_matching_test(func, filter))
+                    .filter(|func| filter.matches_test_function(func))
                     .map(|func| func.name.clone())
                     .collect::<Vec<_>>();
                 (source, name, tests)
@@ -566,12 +566,23 @@ impl MultiContractRunnerBuilder {
     }
 }
 
-pub fn matches_contract(id: &ArtifactId, abi: &JsonAbi, filter: &dyn TestFilter) -> bool {
-    (filter.matches_path(&id.source) && filter.matches_contract(&id.name))
-        && abi.functions().any(|func| is_matching_test(func, filter))
+pub fn matches_artifact(filter: &dyn TestFilter, id: &ArtifactId, abi: &JsonAbi) -> bool {
+    matches_contract(filter, &id.source, &id.name, abi.functions())
 }
 
-/// Returns `true` if the function is a test function that matches the given filter.
-pub(crate) fn is_matching_test(func: &Function, filter: &dyn TestFilter) -> bool {
-    func.is_any_test() && filter.matches_test(&func.signature())
+pub(crate) fn matches_contract(
+    filter: &dyn TestFilter,
+    path: &Path,
+    contract_name: &str,
+    functions: impl IntoIterator<Item = impl std::borrow::Borrow<Function>>,
+) -> bool {
+    (filter.matches_path(path) && filter.matches_contract(contract_name))
+        && functions.into_iter().any(|func| filter.matches_test_function(func.borrow()))
+}
+
+/// Returns `true` if the given contract is a test contract.
+pub(crate) fn is_test_contract(
+    functions: impl IntoIterator<Item = impl std::borrow::Borrow<Function>>,
+) -> bool {
+    functions.into_iter().any(|func| func.borrow().is_any_test())
 }

crates/forge/src/runner.rs

@@ -3,7 +3,7 @@
 use crate::{
     MultiContractRunner, TestFilter,
     fuzz::BaseCounterExample,
-    multi_runner::{TestContract, TestRunnerConfig, is_matching_test},
+    multi_runner::{TestContract, TestRunnerConfig},
     progress::{TestsProgress, start_fuzz_progress},
     result::{SuiteResult, TestResult, TestSetup},
 };
@@ -369,7 +369,7 @@ impl<'a> ContractRunner<'a> {
             .contract
             .abi
             .functions()
-            .filter(|func| is_matching_test(func, filter))
+            .filter(|func| filter.matches_test_function(func))
             .collect::<Vec<_>>();
         debug!(
             "Found {} test functions out of {} in {:?}",