-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
fix(forge test): ensure --rerun only runs failing tests from correct contracts #10753
Conversation
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.
thank you for your PR, left some comments
crates/forge/tests/cli/test_cmd.rs
Outdated
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.
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 comment
The 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?
crates/forge/src/cmd/test/mod.rs
Outdated
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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:
- Pass the data separately through multiple function parameters (messier)
- Global state (bad practice)
- Reload the file multiple times (inefficient)
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.
Thank you @dghelm I think this can be simplified and we can get rid of bunch of custom parsing logic if we save failed tests in different format (and not directly the filter per se) like json
[
{
"test_suite": "CounterTest",
"failures": ["test_counter1", "test_counter2"]
},
{
"test_suite": "AnotherCounterTest",
"failures": ["test_counter1", "test_counter2"]
}
]then we load it and compose the test filter accordingly
Thank you @dghelm I think this can be simplified and we can get rid of bunch of custom parsing logic if we save failed tests in different format (and not directly the filter per se) like json
[ { "test_suite": "CounterTest", "failures": ["test_counter1", "test_counter2"] }, { "test_suite": "AnotherCounterTest", "failures": ["test_counter1", "test_counter2"] } ]then we load it and compose the test filter accordingly
This makes sense to me. I assume this would be a breaking change since these files may be used by external tooling. Would you like me to implement this approach? And if so, do you prefer the changes here or that I raise a new PR?
This makes sense to me. I assume this would be a breaking change since these files may be used by external tooling. Would you like me to implement this approach? And if so, do you prefer the changes here or that I raise a new PR?
Yes, please go ahead and implement in this PR. Should not be a biggie as the file is already removed on forge clean and if we cannot load the old format then we can just ignore / consider it doesn't exist. thank you!
@dghelm would you still have capacity to finalize the PR or should I take it over? thank you!
- Switch rerun cache to JSON and filter by exact (contract, test) pairs.
- Aggregate failures per contract across files; normalize and dedup test names.
- Contract-aware filtering in runners; avoid signature allocations by using func.name.
- Updated CLI tests to validate new JSON format and strict rerun behavior.
Hopefully this gets us closer -- but feel free to take it over at any time!
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.
this comment is unnecessary
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.
please add a optional contract_name argument to <dyn TestFilter>::matches_test_function instead of this function
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.
this should return Result and inspect_err just like before to log the error
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.
we should keep the full signature instead of just the test name, tests are matched against their signature.
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.
same here, do not match against just the test name but the full signature instead
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.