-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
base: master
Are you sure you want to change the base?
Changes from 6 commits
60b8da2
33356ae
7ae6ed9
29bfecc
49ee580
4b0afee
20e3ca7
65220d7
c877a79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() { | ||
|
@@ -916,18 +921,81 @@ 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(pos) = content.rfind('_') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is correct as tests could have the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call. Created |
||
let (contract_part, test_part) = content.split_at(pos); | ||
let test_name = &test_part[1..]; // Remove the '_' prefix | ||
Some(vec![(contract_part.to_string(), test_name.to_string())]) | ||
} else { | ||
None | ||
} | ||
} else if content.contains('_') { | ||
// Multiple qualified failures separated by | | ||
let failures = content | ||
.split('|') | ||
.filter_map(|part| { | ||
if let Some(pos) = part.rfind('_') { | ||
let (contract_part, test_part) = part.split_at(pos); | ||
let test_name = &test_part[1..]; // Remove the '_' prefix | ||
Some((contract_part.to_string(), test_name.to_string())) | ||
} else { | ||
None | ||
} | ||
}) | ||
.collect::<Vec<_>>(); | ||
if failures.is_empty() { | ||
None | ||
} else { | ||
Some(failures) | ||
} | ||
} else { | ||
None | ||
} | ||
} | ||
Err(_) => 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); | ||
} | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -933,6 +933,58 @@ 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) | ||
cmd.forge_fuse().args(["test", "--rerun"]).assert_failure(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's check the stderr and make sure there's only one test executed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Modified test to capture output and assert stdout contains "0 tests passed, 1 failed, 0 skipped (1 total tests)" to ensure exactly one test runs. Does this work? |
||
|
||
// 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( | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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: