Don't fetch block numbers nor check rpc version for ignored tests #3742
Conversation
crates/forge/src/test_filter.rs
Outdated
| IgnoredFilter::Ignored => { | ||
| assert!(ignored); | ||
| true | ||
| } |
There was a problem hiding this comment.
Don't think we should be including this change, it's technically true given our current logic, but I think it's safer to still check if the test is ignored here since it's not very expensive.
| fork_config: if case.config.ignored { | ||
| None | ||
| } else { | ||
| resolve_fork_config(case.config.fork_config, block_number_map, fork_targets) | ||
| .await? | ||
| }, |
There was a problem hiding this comment.
This isn't correct, in case forge is run with --ignored or --include-ignored, they will not have their configs resolved.
We should probably split separate the steps test_target_with_config and resolve_config and call them as separate methods, not as a part of test_package_with_config_resolved.
Then we can pass TestsFilter to resolve_config and call resolve_fork_config based on if should_be_run is true for a test case.
There was a problem hiding this comment.
We could probably make do without splitting from test_package_with_config_resolved and just passing TestFilter to the method, but it seems a bit weird to me to use test filter without we even did any kind of filtering with it.
There was a problem hiding this comment.
Passing tests_filter to test_package_with_config_resolved would require marking the ignored_filter field pub, that looks like a change that would be needing approval?
Filtering is being done there, tests which the fork config should be resolved or not is going to be dependent on the tests_filter argument being passed there, if it wasn't for that, then we'll be running into the issue described above, unless there's another way to resolve that.
| fork_config: if ignored && matches!(ignored_filter, IgnoredFilter::NotIgnored) { | ||
| None | ||
| } else { | ||
| resolve_fork_config(case.config.fork_config, block_number_map, fork_targets) | ||
| .await? | ||
| }, |
There was a problem hiding this comment.
We are duplicating the filtering logic here, why not use TestsFilter.should_be_run instead?
If the test isn't going to be run for whatever reason (including being ignored), then we don't need to resolve fork config for it.
There was a problem hiding this comment.
I tried using TestsFilter.should_be_run but it expects test_case: &TestCaseWithResolvedConfig while in this scenario, the test case being filtered is TestCase<TestCaseConfig> that's why I did that, also worth mentioning that TestsFilter.should_be_run would require me sharing the whole TestsFilter instance instead of just the needed ignored_filter field.
There was a problem hiding this comment.
I tried using TestsFilter.should_be_run but it expects test_case: &TestCaseWithResolvedConfig while in this scenario, the test case being filtered is TestCase
What could be done in that case is we should introduce a trait that both TestCaseConfig and TestCaseResolvedConfig that contains one method like is_ignored. Then should_be_run and relevant TestCaseFilter can be made generic, so they take TestCase<T> where T implements the introduced trait.
This way it can be reused between both test_case types and logic can be shared.
also worth mentioning that TestsFilter.should_be_run would require me sharing the whole TestsFilter instance instead of just the needed ignored_filter field.
Passing a reference to it to the function doesn't incur any significant cost
Closes #1637
Introduced changes
Block number and RPC version for ignored tests is no longer being checked
fork_configreturnsNonefor ignored testsshould_be_runIgnoredFilter::Ignoredmatch arm asserts that test case is ignored.Checklist
CHANGELOG.md