fix(forge test): ensure --rerun only runs failing tests from correct contracts #10753
Conversation
dghelm
requested review from
DaniPopes,
klkvr,
mattsse,
grandizzy,
yash-atreya and
zerosnacks
as code owners
crates/forge/tests/cli/test_cmd.rs
Outdated
| // 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.
let's check the stderr and make sure there's only one test executed
There was a problem hiding this comment.
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?
crates/forge/src/cmd/test/mod.rs
Outdated
| // 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.
I don't think this is correct as tests could have the test_ name format, so in this case a ContractName_test_Name pattern will result in contract_part being ContractName_test and test_part - Name
There was a problem hiding this comment.
Good call.
Created split_qualified_test_name() function that looks for _test patterns to properly separate contract and test names, with fallback to original behavior.
| @@ -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. | |||
There was a problem hiding this comment.
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.
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:
- Pass the data separately through multiple function parameters (messier)
- Global state (bad practice)
- Reload the file multiple times (inefficient)
Motivation
Fixes issue #10693 where
forge test --rerunincorrectly runs passing tests that have the same name as failing tests in different contracts.Solution
Implemented qualified test matching with contract-aware filtering while maintaining full backward compatibility:
Key Changes
matches_qualified_test(contract_name, test_name)method for context-aware filteringqualified_failuresfield to store specific contract/test combinationsContractName_testName) and legacy format (testName1|testName2)based on whether multiple contracts are involved
is_matching_test_in_contextto use qualified matching when appropriateArchitecture
Files Modified
crates/common/src/traits.rs- Extended TestFilter traitcrates/forge/src/cmd/test/filter.rs- Added qualified matching logic and fixed test name format consistencycrates/forge/src/cmd/test/mod.rs- Enhanced failure persistence and loading with intelligent format selectioncrates/forge/src/multi_runner.rs- Updated test matching logic to use qualified contextcrates/forge/tests/cli/test_cmd.rs- Added comprehensive regression testBackward Compatibility
All existing functionality is preserved:
Side Effects and Considerations
Potential Side Effects
ContractTest_testName) in multi-contract scenariosMitigation
PR Checklist
should_replay_failures_only_correct_contractregression testshould_replay_failures_onlytest continues to passmatches_qualified_testmethodThis contribution was made to explore the viability of bug-fixing work via Claude Code assistance, as prompted by this Tweet. If it's a mess, please don't spend too much time helping me get it over the line.