-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
feat(coverage): add option to ignore directories and files from coverage report #8321
Conversation
/// 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>, |
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've opted for no-match-coverage
to stay in line with the no-match-*
pattern established (no-match-test
, no-match-contract
, no-match-path
)
See: #7301 (comment)
crates/evm/coverage/src/lib.rs
Outdated
/// | ||
/// This function should only be called after all the sources were used, otherwise, the output | ||
/// will be missing the ones that are dependent on them. | ||
pub fn filter_out_ignored_sources(&mut self, filter: &impl CoverageFilter) { |
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 just replace this with an Fn(&Path) -> bool
crates/evm/coverage/src/lib.rs
Outdated
let mut new_items = HashMap::new(); | ||
for (version, items) in self.items.iter() { | ||
let new_items_for_version: Vec<_> = items | ||
.iter() | ||
.filter(|item| { | ||
self.source_paths | ||
.get(&(version.clone(), item.loc.source_id)) | ||
.map(|path| filter.matches_file_path(path)) | ||
.unwrap_or(false) | ||
}) | ||
.cloned() | ||
.collect(); | ||
if !new_items_for_version.is_empty() { | ||
new_items.insert(version.clone(), new_items_for_version); | ||
} | ||
} | ||
self.items = new_items; |
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.
isn't this just a HashMap::retain
?
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.
lgtm
pending @klkvr
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.
lgtm
thanks for this 🥇 |
Motivation
Follow-up of #7301 as I'm not able to push directly to the fork
This PR includes the commits of https://github.com/dimazhornyk
Closes: #2988
Solution
Contrary to #7301 I decided that for flexibility it is preferable to generalize beyond
glob
and instead use aregex
on the filepath (not just file name)contractA
is filtered out,contractB
is preserved)This allows for a syntax like:
forge coverage --no-match-coverage "(script|test|Foo|Bar)"
wherescript|test
are obviously paths andFoo|Bar
files where a glob would be limited to a single folder and not individual filesThere is currently no warning applied when all sources are filtered out, that could be a follow up nice to have.