forge coverage considers branches partially covered if they have CALL

[adhusson]

Component

Forge

Have you ensured that all of these are up to date?

• Foundry
• Foundryup

What version of Foundry are you on?

forge 0.2.0 (1da2b65 2022-10-14T00:09:33.152136Z)

What command(s) is the bug in?

forge coverage

Operating System

No response

Describe the bug

I may be missing something, but I'm surprised by this:

where the tests are

    function test_true() public {
      counter.go(true);
    }
    function test_false() public {
      counter.go(false);
    }

in summary form:

+-----------------+---------------+---------------+--------------+---------------+
| File            | % Lines       | % Statements  | % Branches   | % Funcs       |
+================================================================================+
| src/Counter.sol | 100.00% (8/8) | 100.00% (6/6) | 75.00% (3/4) | 100.00% (2/2) |
|-----------------+---------------+---------------+--------------+---------------|
| Total           | 100.00% (8/8) | 100.00% (6/6) | 75.00% (3/4) | 100.00% (2/2) |
+-----------------+---------------+---------------+--------------+---------------+

[KittyFu307]

I have the same question
Hi @rkrasiuk Is there any solution for that?

[rkrasiuk]

@onbjerg could you take a look pls?

[KittyFu307]

~hello @rkrasiuk any updates for that?

[gakonst]

Sorry for the late response. Not sure exactly what the issue is here, what was the exact expected behavior and how does this differ? This looks like a partially covered given you pass true and the if branch is hit but not the else?

[adhusson]

Both branches are taken (the tests call go(false) and go(true)). The first if/else is marked as partially covered despite both branches being hit. The two if/else are the same, except that the first calls this.fn() and the second calls fn() (fn is a noop function).

Looking back, maybe fn() is compiled away, this.fn() is not, and the possibility of a non-tested revert inside this.fn() means coverage is not complete? Surprising, anyway.

[gakonst]

Looking back, maybe fn() is compiled away, this.fn() is not, and the possibility of a non-tested revert inside this.fn() means coverage is not complete?

Sounds right. Can you add a extra test for this.fn reverting, just to test the hypothesis?

[adhusson]

Confirmed. The try/catch will not display coverage info, btw.

I don't know if the issue can be totally closed. Letting errors bubble up are a normal part of writing contracts. Maybe a // forgecov: ignore-next-line-revert?

[frontier159]

Any update on where this stands @gakonst ?

Currently a blocker for me on fully replacing HH as the coverage here isn't accurate

[mds1]

Is the issue here that external calls contain "hidden" branches, because you also need to test the code path where the call reverts, which is only in the bytecode and not in the solidity itself? If so, I think we can close this and just document that in the book

[PaulRBerg]

Update: see my two comments below - the problem reported in this comment is a different bug, which I reported in #4310.

---

Is the issue here that external calls contain "hidden" branches, because you also need to test the code path where the call reverts, which is only in the bytecode and not in the solidity itself?

No, I don't think so, @mds1. Let me share the case that I bumped into, which I shared recently in #3600 (a potentially related issue):

Line 207 appears as partially covered, but I'm absolutely certain (I tested with console logs) that the if statement is covered properly by my tests.

This is the definition of the getStatus function:

function getStatus(
    uint256 streamId
) public view virtual override returns (Lockup.Status status) {
    status = _streams[streamId].status;
}

There's no potential revert in this function.

Somehow the mere existence of the calls to the _isApprovedOrOwner and the _withdraw functions makes line 207 to not appear as fully covered. But this is wrong and, FWIW, I also have extensive testing for the full gamut of possible reverts in these latter functions (the proof is in the coverage report itself, which shows these other functions as fully covered).

[PaulRBerg]

Update: I have managed to get to the bottom of the incorrect report I was getting, and it turns out it was unrelated to @adhusson's issue (though it definitely looked as if the situations were related):

#4310

[PaulRBerg]

Separately, I have managed to reproduce the issue reported by @adhusson. Take the following code (hidden by default for brevity reasons, click the toggle below to collapse it):

Click me to toggle the contracts

interface IERC20 {
    function balanceOf(address account) external view returns (uint256);
}

contract Bar {
    mapping(address => bool) internal _listed;

    function isListed(address asset) external view returns (bool) {
        return _listed[asset];
    }

    function setListed(address asset, bool listed) external {
        _listed[asset] = listed;
    }
}

contract Foo {
    Bar public bar;

    constructor(Bar bar_) {
        bar = bar_;
    }

    function maxFlashLoan(address asset) external view returns (uint256 amount) {
        if (bar.isListed(asset)) {
            amount = IERC20(asset).balanceOf(address(this));
        }
    }
}

And the following tests (again, hidden for brevity):

Click me to toggle the tests

import { ERC20 } from "@openzeppelin/token/ERC20/ERC20.sol";
import { Test } from "forge-std/Test.sol";

import { Bar, Foo } from "../src/Foo.sol";

contract FooTest is PRBTest {
    Bar internal bar = new Bar();
    Foo internal foo = new Foo(bar);
    ERC20 internal token = new ERC20("Token", "TKN");

    function test_MaxFlashLoan_NotListed() external {
        bar.setListed(address(token), false);
        address asset = address(token);
        uint256 actualAmount = foo.maxFlashLoan(asset);
        uint256 expectedAmount = 0;
        assertEq(actualAmount, expectedAmount);
    }

    function test_MaxFlashLoan_Listed() external {
        bar.setListed(address(token), true);
        address asset = address(token);
        uint256 actualAmount = foo.maxFlashLoan(asset);
        uint256 expectedAmount = 0;
        assertEq(actualAmount, expectedAmount);
    }
}

Now, run forge coverage. You will get this report:

| File        | % Lines       | % Statements  | % Branches   | % Funcs       |
|-------------|---------------|---------------|--------------|---------------|
| src/Foo.sol | 100.00% (4/4) | 100.00% (4/4) | 50.00% (1/2) | 100.00% (3/3) |
| Total       | 100.00% (4/4) | 100.00% (4/4) | 50.00% (1/2) | 100.00% (3/3) |

Notice that the branch coverage is 50% instead of 100%, even if the tests cover both possible branches of the if statement bar.isListed(asset).

Is this intended? Does Forge consider that there may be a revert in balanceOf that technically the user did not handle? If the answer is yes, it would certainly be great to have a way to ignore this potential revent, e.g. with an annotation like the one suggested by @adlerjohn above: // forgecov: ignore-next-line-revert.

Finally, I should note that this seems to happen only for calls made within if statements. Calls made outside if statements are not reported as having partial coverage.

[dansimpson]

I am also experiencing this issue or a similar issue. Branches are not covered, even though all lines within the branch have hits.

| src/finance/MyContract.sol        | 100.00% (160/160) | 100.00% (166/166) | 79.35% (73/92) | 100.00% (51/51) |

[PaulRBerg]

@dansimpson thanks for the report. Could you post the code you are trying to cover?

The problem you are experiencing might be unrelated to this GitHub issue (see this comment).

[dansimpson]

@PaulRBerg thanks for compiling all of that. After further review I found I was simply confused by the reporting. I had a nested require statement that wasn't being tested for a revert. Coverage reported parent branches were not covered, which I found very confusing. From a DX perspective, I think the report should show 0 hits on branches that are not tested, but not their ancestors. I became fixated on the top level branch, knowing that I had tests covering it.

I started working on the coverage from the bottom up, and figured it out.

[gakonst]

From a DX perspective, I think the report should show 0 hits on branches that are not tested, but not their ancestors.

Understood. This needs some thinking on how exactly to implement, but is an interesting point.

[grandizzy]

confirmed issues reported here fixed with #8414 please reopen if you hit similar failures