Skip to content
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

Bytecode level coverage reporting #6563

Merged
merged 7 commits into from
Dec 19, 2023

Conversation

mountainpath9
Copy link
Contributor

Motivation

The coverage testing in foundry is useful, but has many outstanding issues, eg those mentioned here:

#1961

The barrier to entry in addressing these issues is relatively high, as the pipeline used to generate the reports is somewhat complex:

1.    build code ->
2.    execute tests recording EVM instruction level coverage ->
3.    construct mapping from bytecode addresses to source level coverage items -> 
4.    apply the mapping to the instruction level coverage to generage an abstract CoverageReport ->
5.    process  the CoverageReport to generate user friendly output as requested

and there is no useful visibility into the interior stages of this pipeline.

Solution

This PR adds a new coverage report, that provided human readable bytecode level (ie per instruction) output.

forge coverage --report bytecode

This generates a directory of disassembled bytecode files that include hit counts for each instruction, along with a precise source reference. This output corresponds to the results from stage 2 in the pipeline description above.

An example snippet from a bytecode coverage file:

[002] 000012b0: JUMPDEST                       // contracts/common/Range.sol: 13:5-19:6 (251-484)
[002] 000012b1: DUP1                           // contracts/common/Range.sol: 14:21-14:28 (347-354)
[002] 000012b2: PUSH1 0xff                     // contracts/common/Range.sol: 14:13-14:28 (339-354)
[002] 000012c3: AND                            // contracts/common/Range.sol: 14:13-14:28 (339-354)
[002] 000012c4: DUP3                           // contracts/common/Range.sol: 14:13-14:18 (339-344)
[002] 000012c5: PUSH1 0xff                     // contracts/common/Range.sol: 14:13-14:28 (339-354)
[002] 000012d6: AND                            // contracts/common/Range.sol: 14:13-14:28 (339-354)
[002] 000012d7: GT                             // contracts/common/Range.sol: 14:13-14:28 (339-354)
[002] 000012d8: ISZERO                         // contracts/common/Range.sol: 14:9-16:10 (335-416)
[002] 000012d9: PUSH2 0x131b                   // contracts/common/Range.sol: 14:9-16:10 (335-416)
[002] 000012dc: JUMPI                          // contracts/common/Range.sol: 14:9-16:10 (335-416)
      000012dd: DUP2                           // contracts/common/Range.sol: 15:33-15:38 (390-395)

@onbjerg
Copy link
Member

onbjerg commented Dec 11, 2023

I imagine this is good for debugging coverage, but I'm not sure how this is useful to end users?

@mountainpath9
Copy link
Contributor Author

Yes - it's intended for debugging coverage, and is, IMO, more useful that the existing debug coverage report.

Having said that, apart from investigating bugs in the existing source level coverage reporting, it also exposes fragments of the bytecode that do not correspond to coverage items in the existing reports. It could be argued that 100% test coverage at the bytecode level is a worthwhile goal.

@Evalir
Copy link
Member

Evalir commented Dec 14, 2023

@onbjerg your call on what we should do here

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

Coverage needs a revisit—this can come useful while debugging new impls, and might be interesting to some optimizers. Let's go with this for now, and we can readjust later.

@Evalir Evalir merged commit eb2141c into foundry-rs:master Dec 19, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants