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

Forge Coverage does not include testFail_ unit test functions #4259

Closed
2 tasks done
relyt29 opened this issue Feb 2, 2023 · 5 comments · Fixed by #7670
Closed
2 tasks done

Forge Coverage does not include testFail_ unit test functions #4259

relyt29 opened this issue Feb 2, 2023 · 5 comments · Fixed by #7670
Labels
T-bug Type: bug

Comments

@relyt29
Copy link

relyt29 commented Feb 2, 2023

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 (cd7850b 2023-02-02T00:07:41.087456684Z)

What command(s) is the bug in?

forge coverage

Operating System

Linux

Describe the bug

Hi All,

Apologies if this bug is a duplicate or known issue, I did not see anything when I searched for it.

The forge coverage tool does not appear to consider any functions called inside a testFail type test as valid calls of a function for coverage. Example below:

➜  bugtest git:(main) ✗ forge coverage        
[⠒] Compiling...
[⠰] Compiling 17 files with 0.8.17
[⠒] Solc 0.8.17 finished in 2.23s
Compiler run successful
Analysing contracts...
Running tests...
| File            | % Lines     | % Statements | % Branches  | % Funcs     |
|-----------------|-------------|--------------|-------------|-------------|
| src/Counter.sol | 0.00% (0/2) | 0.00% (0/2)  | 0.00% (0/2) | 0.00% (0/1) |
| Total           | 0.00% (0/2) | 0.00% (0/2)  | 0.00% (0/2) | 0.00% (0/1) |

Sample Code

src/Counter.sol

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

contract Counter {
    uint256 private number;

    function setNumber(uint256 newNumber) public {
        require(newNumber != 666);
        number = newNumber;
    }
}

test/Counter.t.sol

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "../src/Counter.sol";

contract CounterTest is Test {
    Counter public counter;

    function setUp() public {
        counter = new Counter();
        counter.setNumber(0);
    }

    function testFail_setNumber() public {
        counter.setNumber(666);
    }
}

foundry.toml

[profile.default]
src = 'src'
out = 'out'
libs = ['lib']
optimizer = true
via_ir = true

[profile.default.optimizer_details]
yul = true

Expected Correct Output

The coverage tool should print that the percentage of functions called is greater than 0, since the setNumber function was called in the testFail_setNumber test.

@relyt29 relyt29 added the T-bug Type: bug label Feb 2, 2023
@PaulRBerg
Copy link
Contributor

FWIW, it is not recommended to use testFail anyway. It's become an anti-pattern, and vm.expectRevert is a much less error-prone way to test reverts. See related discussion here:

foundry-rs/book#740

@relyt29
Copy link
Author

relyt29 commented Feb 7, 2023

Agree that testFail is an anti-pattern, I use it anyways because I am lazy

if Foundry devs don't want us using the anti-pattern they should remove or deprecate the feature

if they do not want to remove or deprecate the feature then they should make it work with coverage

@PaulRBerg
Copy link
Contributor

IIRC the reason testFail is still kept is backwards compatibility.

Refactoring to use vm.expectRevert is as simple as:

function test_RevertWhen_Smth() external {
    vm.expectRevert();
    foo.call();
}

you don't necessarily have to pass a revert reason string or a custom error selector - passing no arg will pass the test for any revert.

@relyt29
Copy link
Author

relyt29 commented Feb 7, 2023

Yes I am well aware that you can refactor the code to get coverage to work

My point is it's a betrayal of user expectations, since you generally expect features in a dev environment to work with each other

If you don't want to make testFail work, then maybe a good move would be to explicitly inform folks testFail is deprecated in the foundry book and tell people not to use it, or to remove mentions of testFail at all in the foundry book

either way, I don't care, Foundry devs should do whatever they want, this is obviously a minor issue and if the devs want to #wontfix it then you can close the issue. I just wanted folks to be aware that the current way took at least 5 minutes out of my day to realize the coverage was not working with the testFail feature

@PaulRBerg
Copy link
Contributor

PaulRBerg commented Feb 7, 2023

Foundry is an open-source project, ser. It is as much your and my own responsibility to make this piece of software better - nat just the core maintainers'.

At any rate, I agree that the Foundry Book should be updated to indicate that testFail is an anti-pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
Status: Completed
Development

Successfully merging a pull request may close this issue.

2 participants