Skip to content

Don't fetch block numbers nor check rpc version for ignored tests #3742

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.

Sign up for GitHub

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 12 commits into
base: master
Choose a base branch
from
+583 −66
Open

Don't fetch block numbers nor check rpc version for ignored tests #3742

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
824de94
dev: Don't fetch block numbers nor check rpc version for ignored tests
Abeeujah Sep 22, 2025
5fd44ff
Merge remote-tracking branch 'origin/master' into ignore-flag
Abeeujah Sep 22, 2025
bb4e267
dev: Revert to inexpensive safety check
Abeeujah Sep 24, 2025
8546e3e
dev: check tests filter before resolving fork config
Abeeujah Sep 24, 2025
d09df8f
Merge branch 'master' into ignore-flag
Abeeujah Sep 24, 2025
dee6faf
dev: Fix Visibility and Ownership
Abeeujah Sep 24, 2025
c0eeffb
dev: Introduce TestCaseIsIgnored trait
Abeeujah Sep 26, 2025
5b466f8
dev: Implement Default Trait for TestsFilter
Abeeujah Sep 26, 2025
a6cba94
dev: Test Fork Config Resolution
Abeeujah Sep 30, 2025
11463a7
dev: Update branch
Abeeujah Sep 30, 2025
a6d2fdc
Merge branch 'master' into ignore-flag
Abeeujah Oct 1, 2025
0da75a2
dev: Remove Default impl for TestsFilter
Abeeujah Oct 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions crates/forge/src/run_tests/resolve_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ pub async fn resolve_config(
ignored: case.config.ignored
|| (env_ignore_fork_tests && case.config.fork_config.is_some()),
expected_result: case.config.expected_result,
fork_config: resolve_fork_config(
case.config.fork_config,
block_number_map,
fork_targets,
)
.await?,
fork_config: if case.config.ignored {
None
} else {
resolve_fork_config(case.config.fork_config, block_number_map, fork_targets)
.await?
},
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

fuzzer_config: case.config.fuzzer_config,
disable_predeployed_contracts: case.config.disable_predeployed_contracts,
},
Expand Down
5 changes: 4 additions & 1 deletion crates/forge/src/test_filter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,10 @@ impl TestCaseFilter for TestsFilter {

match self.ignored_filter {
IgnoredFilter::All => true,
IgnoredFilter::Ignored => ignored,
IgnoredFilter::Ignored => {
assert!(ignored);
true
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

IgnoredFilter::NotIgnored => !ignored,
}
}
Expand Down
Loading