fix: various coverage fixes #2504
Conversation
onbjerg
force-pushed
the
onbjerg/coverage-fixes
branch
from
28b073a
to
ec8367b
Compare
|
Interestingly enough internal functions and return-only functions are covered sometimes. Not entirely sure what makes or breaks it right now. See e.g. my example in #2194 Edit: Ok, it seems for internal functions, we can't find anchors if they are not called... Makes sense - if they are not called, there is no reason to emit bytecode for them As for return-only functions, I can't really seem to reproduce them not working |
|
Great to see this! Just tried it. My repo has a |
|
@gigamesh The Your test contracts should only be included in the output if there are contracts in there that do not have functions starting with |
|
Should be good to review, the only unsupported contract now should be libraries that have internal functions - currently figuring out what the best way is to detect those without having to walk the entire AST. Will start work on fuzz testing support |
| let mut report = CoverageReport::default(); | ||
|
|
||
| // Collect ASTs and sources | ||
| let mut versioned_asts: HashMap<Version, HashMap<usize, Ast>> = HashMap::new(); |
cli/src/cmd/forge/coverage.rs
Outdated
| // Filter out dependencies | ||
| // NOTE(onbjerg): For some reason this only returns Some for non-library imports... Is | ||
| // there a better way? | ||
| if project_paths.resolve_library_import(std::path::Path::new(&path)).is_none() { |
There was a problem hiding this comment.
is path here absolute or relative to project dir?
I can add a util function that checks if a path is part of libs
There was a problem hiding this comment.
It seems to be relative to the project dir, and after remappings are applied.
A function to check if it is a part of libs would be nice
|
Should we consider merging anyway and maybe filter out libraries from the terminal output, until we get coverage for them? |
|
We could |
|
@onbjerg LGTM as well. The latest ethers has the helpers requested, if you want to use them |
onbjerg
force-pushed
the
onbjerg/coverage-fixes
branch
from
7bc1877
to
14e712e
Compare
* fix: account for inheritance in coverage * refactor: move stuff around * docs: add some docs * fix: dummy link contracts using libs for analysis * build: bump ethers * refactor: use lib detection helper
Now accounts for inheritance and abstract contracts, however it seems there are still some issues with internal functions and libraries. For internal functions, no instructions can be found, which is pretty weird considering Godbolt does display a range of bytecode that matches internal functions when I test on there. For libraries, my suspicion is that it is related to unlinked bytecode
Opening up a draft to get a sanity check on the way I find matching instructions, perhaps it is wrong in some cases and that is why internal functions don't work
This also relaxes the coverage analysis part a bit, so instead of panicking when we can't find anchors, items will now just show up as uncovered, and
RUST_LOG=warnwill show more info. I think this is ok; we fail irrecoverably and we don't end up showing things as covered if they aren't. This is possible because finding anchors and coverage items is two separate stages nowStill want to fix these before merge, but we can also merge before that if it makes more sense:
I will also do a bit of clean up before merge (some stuff should be in other modules, some stuff is missing docs)