diff --git a/crates/common/src/traits.rs b/crates/common/src/traits.rs index f5f3ea14ce460..369f26d41845a 100644 --- a/crates/common/src/traits.rs +++ b/crates/common/src/traits.rs @@ -15,6 +15,14 @@ pub trait TestFilter: Send + Sync { /// Returns a contract with the given path should be included. fn matches_path(&self, path: &Path) -> bool; + + /// Returns whether a specific test in a specific contract should be included. + /// This enables more precise filtering for scenarios like --rerun where we need + /// to match exact contract/test combinations rather than just test names. + fn matches_qualified_test(&self, contract_name: &str, test_name: &str) -> bool { + // Default implementation for backward compatibility + self.matches_contract(contract_name) && self.matches_test(test_name) + } } /// Extension trait for `Function`. diff --git a/crates/forge/src/cmd/test/filter.rs b/crates/forge/src/cmd/test/filter.rs index ec2e9b01b50e8..c591467553159 100644 --- a/crates/forge/src/cmd/test/filter.rs +++ b/crates/forge/src/cmd/test/filter.rs @@ -42,6 +42,11 @@ pub struct FilterArgs { /// Only show coverage for files that do not match the specified regex pattern. #[arg(long = "no-match-coverage", visible_alias = "nmco", value_name = "REGEX")] pub coverage_pattern_inverse: Option, + + /// Qualified test failures from --rerun (contract, test) pairs. + /// This is not a CLI argument, but is populated internally when --rerun is used. + #[arg(skip)] + pub qualified_failures: Option>, } impl FilterArgs { @@ -52,7 +57,8 @@ impl FilterArgs { self.contract_pattern.is_none() && self.contract_pattern_inverse.is_none() && self.path_pattern.is_none() && - self.path_pattern_inverse.is_none() + self.path_pattern_inverse.is_none() && + self.qualified_failures.is_none() } /// Merges the set filter globs with the config's values @@ -138,6 +144,21 @@ impl TestFilter for FilterArgs { } ok } + + fn matches_qualified_test(&self, contract_name: &str, test_name: &str) -> bool { + // If we have qualified failures, only match those specific combinations + if let Some(qualified_failures) = &self.qualified_failures { + for (failed_contract, failed_test) in qualified_failures { + if failed_contract == contract_name && failed_test == test_name { + return true; + } + } + return false; + } + + // Fall back to default behavior for non-qualified scenarios + self.matches_contract(contract_name) && self.matches_test(test_name) + } } impl fmt::Display for FilterArgs { @@ -220,6 +241,10 @@ impl TestFilter for ProjectPathsAwareFilter { path = path.strip_prefix(&self.paths.root).unwrap_or(path); self.args_filter.matches_path(path) && !self.paths.has_library_ancestor(path) } + + fn matches_qualified_test(&self, contract_name: &str, test_name: &str) -> bool { + self.args_filter.matches_qualified_test(contract_name, test_name) + } } impl fmt::Display for ProjectPathsAwareFilter { diff --git a/crates/forge/src/cmd/test/mod.rs b/crates/forge/src/cmd/test/mod.rs index 9e8a4b2d26643..dda944f5ac4d1 100644 --- a/crates/forge/src/cmd/test/mod.rs +++ b/crates/forge/src/cmd/test/mod.rs @@ -823,7 +823,12 @@ impl TestArgs { pub fn filter(&self, config: &Config) -> Result { let mut filter = self.filter.clone(); if self.rerun { - filter.test_pattern = last_run_failures(config); + // Try to load qualified failures first, fall back to legacy pattern matching + if let Some(qualified_failures) = last_run_failures_qualified(config) { + filter.qualified_failures = Some(qualified_failures); + } else { + filter.test_pattern = last_run_failures(config); + } } if filter.path_pattern.is_some() { if self.path.is_some() { @@ -916,18 +921,93 @@ fn last_run_failures(config: &Config) -> Option { } } +/// Load persisted failures as qualified contract/test combinations +pub fn last_run_failures_qualified(config: &Config) -> Option> { + match fs::read_to_string(&config.test_failures_file) { + Ok(content) => { + // Parse qualified pattern format: ContractName_testName or legacy format + if content.contains('_') && !content.contains('|') { + // Single qualified failure + if let Some((contract_name, test_name)) = split_qualified_test_name(&content) { + Some(vec![(contract_name, test_name)]) + } else { + None + } + } else if content.contains('_') { + // Multiple qualified failures separated by | + let failures = + content.split('|').filter_map(split_qualified_test_name).collect::>(); + if failures.is_empty() { + None + } else { + Some(failures) + } + } else { + None + } + } + Err(_) => None, + } +} + +/// Split a qualified test name into contract name and test name parts. +/// Looks for the pattern ContractName_test... since test functions must start with "test". +fn split_qualified_test_name(qualified_name: &str) -> Option<(String, String)> { + // Look for _test, _testFail, _testFuzz, _invariant_ patterns + if let Some(test_start) = qualified_name.find("_test") { + let contract_part = &qualified_name[..test_start]; + let test_part = &qualified_name[test_start + 1..]; // Skip the '_' + Some((contract_part.to_string(), test_part.to_string())) + } else if let Some(test_start) = qualified_name.find("_invariant_") { + let contract_part = &qualified_name[..test_start]; + let test_part = &qualified_name[test_start + 1..]; // Skip the '_' + Some((contract_part.to_string(), test_part.to_string())) + } else { + // Fallback to the original behavior if no test pattern is found + if let Some(pos) = qualified_name.rfind('_') { + let (contract_part, test_part) = qualified_name.split_at(pos); + let test_name = &test_part[1..]; // Remove the '_' prefix + Some((contract_part.to_string(), test_name.to_string())) + } else { + None + } + } +} + /// Persist filter with last test run failures (only if there's any failure). fn persist_run_failures(config: &Config, outcome: &TestOutcome) { if outcome.failed() > 0 && fs::create_file(&config.test_failures_file).is_ok() { + let failures: Vec<_> = + outcome.into_tests_cloned().filter(|test| test.result.status.is_failure()).collect(); + + if failures.is_empty() { + return; + } + + // Use qualified format if there are multiple contracts in the test run + // This is a conservative approach that ensures no ambiguity + let use_qualified = outcome.results.len() > 1; + let mut filter = String::new(); - let mut failures = outcome.failures().peekable(); - while let Some((test_name, _)) = failures.next() { - if test_name.is_any_test() { - if let Some(test_match) = test_name.split("(").next() { - filter.push_str(test_match); - if failures.peek().is_some() { + let mut first = true; + + for test in failures { + if test.signature.is_any_test() { + if let Some(test_match) = test.signature.split("(").next() { + if !first { filter.push('|'); } + first = false; + + if use_qualified { + // Use qualified format when failures come from multiple contracts + let contract_name = + test.artifact_id.split(':').next_back().unwrap_or(&test.artifact_id); + filter.push_str(&format!("{contract_name}_{test_match}")); + } else { + // Use legacy format when all failures come from the same contract + filter.push_str(test_match); + } } } } diff --git a/crates/forge/src/multi_runner.rs b/crates/forge/src/multi_runner.rs index 7b10328d5908e..358b19e653886 100644 --- a/crates/forge/src/multi_runner.rs +++ b/crates/forge/src/multi_runner.rs @@ -95,8 +95,9 @@ impl MultiContractRunner { filter: &'b dyn TestFilter, ) -> impl Iterator + 'b { self.matching_contracts(filter) - .flat_map(|(_, c)| c.abi.functions()) - .filter(|func| is_matching_test(func, filter)) + .flat_map(|(id, c)| c.abi.functions().map(move |func| (id, func))) + .filter(|(id, func)| is_matching_test_in_context(func, id, filter)) + .map(|(_, func)| func) } /// Returns an iterator over all test functions in contracts that match the filter. @@ -120,7 +121,7 @@ impl MultiContractRunner { let tests = c .abi .functions() - .filter(|func| is_matching_test(func, filter)) + .filter(|func| is_matching_test_in_context(func, id, filter)) .map(|func| func.name.clone()) .collect::>(); (source, name, tests) @@ -551,10 +552,30 @@ 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)) + abi.functions().any(|func| is_matching_test_in_context(func, id, filter)) } /// 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()) } + +/// Returns `true` if the function is a test function that matches the given filter in the context +/// of a specific contract. +pub(crate) fn is_matching_test_in_context( + func: &Function, + artifact_id: &ArtifactId, + filter: &dyn TestFilter, +) -> bool { + if !func.is_any_test() { + return false; + } + + // Extract the test name from the function signature + let signature = func.signature(); + let test_name = signature.split("(").next().unwrap_or(&signature); + let contract_name = artifact_id.name.as_str(); + + // Use qualified test matching which properly handles both qualified and legacy scenarios + filter.matches_qualified_test(contract_name, test_name) +} diff --git a/crates/forge/tests/cli/test_cmd.rs b/crates/forge/tests/cli/test_cmd.rs index 6a6f067722cad..3ea4b6682e6d5 100644 --- a/crates/forge/tests/cli/test_cmd.rs +++ b/crates/forge/tests/cli/test_cmd.rs @@ -933,6 +933,65 @@ Encountered a total of 2 failing tests, 0 tests succeeded "#]]); }); +// +forgetest_init!(should_replay_failures_only_correct_contract, |prj, cmd| { + prj.wipe_contracts(); + + // Create two contracts with tests that have the same name + prj.add_test( + "Contract1.t.sol", + r#" +import {Test} from "forge-std/Test.sol"; + +contract Contract1Test is Test { + function testSameName() public pure { + require(2 > 1); // This should pass + } +} + "#, + ) + .unwrap(); + + prj.add_test( + "Contract2.t.sol", + r#" +import {Test} from "forge-std/Test.sol"; + +contract Contract2Test is Test { + function testSameName() public pure { + require(1 > 2, "testSameName failed"); // This should fail + } +} + "#, + ) + .unwrap(); + + // Run initial test - should have 1 failure and 1 pass + cmd.args(["test"]).assert_failure(); + + // Test failure filter should be persisted. + assert!(prj.root().join("cache/test-failures").exists()); + + // This is the key test: --rerun should NOT run both contracts with same test name + // Before the fix, this would run 2 tests (both testSameName functions) + // After the fix, this should run only 1 test (only the specific failing one) + let output = cmd.forge_fuse().args(["test", "--rerun"]).assert_failure(); + + // Verify that only 1 test is executed (not 2) + let stdout = String::from_utf8_lossy(&output.get_output().stdout); + assert!( + stdout.contains("0 tests passed, 1 failed, 0 skipped (1 total tests)"), + "Expected exactly 1 test to be executed (the failing one), but got different count in stdout: {stdout}" + ); + + // Additional verification: Check the test failures file exists and contains our test + let failures_content = std::fs::read_to_string(prj.root().join("cache/test-failures")).unwrap(); + assert!( + failures_content.contains("testSameName"), + "Expected test name in failure file, got: {failures_content}" + ); +}); + // forgetest_init!(should_not_record_setup_failures, |prj, cmd| { prj.add_test(