Skip to content

fix(forge test): ensure --rerun only runs failing tests from correct contracts #10753

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
8 changes: 8 additions & 0 deletions crates/common/src/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down
27 changes: 26 additions & 1 deletion crates/forge/src/cmd/test/filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<regex::Regex>,

/// Qualified test failures from --rerun (contract, test) pairs.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should make this pub arg but rather set (or reuse) the rerun flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove pub in next commit.

Not sure of a better option. The rerun flag is insufficient as a boolean when this is holding internal state that contains the actual list of (contract, test) pairs to rerun.

The flow is:
f--rerun flag → load failure file → populate qualified_failures → use in filtering

As I see it, the alternatives would be:

  1. Pass the data separately through multiple function parameters (messier)
  2. Global state (bad practice)
  3. Reload the file multiple times (inefficient)

/// This is not a CLI argument, but is populated internally when --rerun is used.
#[arg(skip)]
pub qualified_failures: Option<Vec<(String, String)>>,
}

impl FilterArgs {
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
94 changes: 87 additions & 7 deletions crates/forge/src/cmd/test/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -823,7 +823,12 @@ impl TestArgs {
pub fn filter(&self, config: &Config) -> Result<ProjectPathsAwareFilter> {
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() {
Expand Down Expand Up @@ -916,18 +921,93 @@ fn last_run_failures(config: &Config) -> Option<regex::Regex> {
}
}

/// Load persisted failures as qualified contract/test combinations
pub fn last_run_failures_qualified(config: &Config) -> Option<Vec<(String, String)>> {
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::<Vec<_>>();
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);
}
}
}
}
Expand Down
29 changes: 25 additions & 4 deletions crates/forge/src/multi_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,9 @@ impl MultiContractRunner {
filter: &'b dyn TestFilter,
) -> impl Iterator<Item = &'a Function> + '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.
Expand All @@ -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::<Vec<_>>();
(source, name, tests)
Expand Down Expand Up @@ -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)
}
59 changes: 59 additions & 0 deletions crates/forge/tests/cli/test_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -933,6 +933,65 @@ Encountered a total of 2 failing tests, 0 tests succeeded
"#]]);
});

// <https://github.com/foundry-rs/foundry/issues/10693>
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}"
);
});

// <https://github.com/foundry-rs/foundry/issues/9285>
forgetest_init!(should_not_record_setup_failures, |prj, cmd| {
prj.add_test(
Expand Down