Test function early returns after `vm.expectRevert`

[0xbok]

Component

Forge

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

• [X] Foundry
• [X] Foundryup

What version of Foundry are you on?

forge 0.2.0 (d7733ee 2022-10-03T00:06:06.841223Z)

What command(s) is the bug in?

forge test

Operating System

macOS (Apple Silicon)

Describe the bug

pragma solidity 0.8.4;

import "forge-std/Test.sol";

contract ContractTest is Test {
    function setUp() public {}

    function rever() internal {
        revert();
    }

    function testExample() public {
        vm.expectRevert();
        rever();

        rever();
        console.log("Does not print");
    }
}

Running forge test -vvv passes this test and also doesn't print "Does not print" on console. In summary, after vm.expectRevert() the test just executes the call and does not run the remaining test.

[mattsse]

@DaniPopes there's something off here, do you have the bandwidth to look into this?

[DaniPopes]

This has something to do with reverting and expecting on the test contract itself since this works as expected:

// SPDX-License-Identifier: Unlicensed
pragma solidity ^0.8.4;

import "forge-std/Test.sol";

contract CustomContract {
    error CustomError();

    function rever() external {
        revert CustomError();
    }
}

contract ContractTest is Test {
    CustomContract internal c;

    function setUp() public {
        c = new CustomContract();
    }

    function testExample() public {
        // vm.expectRevert(CustomContract.CustomError.selector);
        vm.expectRevert();
        c.rever();

        c.rever();
        console.log("Does not print");
    }
}

Outputs:

$ forge test -vvv
[...]
[FAIL. Reason: CustomError()] testExample() (gas: 8795)
Traces:
  [8795] ContractTest::testExample() 
    ├─ [0] VM::expectRevert() 
    │   └─ ← ()
    ├─ [158] CustomContract::rever() 
    │   └─ ← "CustomError()"
    ├─ [158] CustomContract::rever() 
    │   └─ ← "CustomError()"
    └─ ← "CustomError()"

Test result: FAILED. 0 passed; 1 failed; finished in 244.88µs

Failing tests:
Encountered 1 failing test in test/a.t.sol:ContractTest
[FAIL. Reason: CustomError()] testExample() (gas: 8795)

Encountered a total of 1 failing tests, 0 tests succeeded

[0xbok]

This has something to do with reverting and expecting on the test contract

makes sense as that’s how I discovered this bug. I have an abstract contract that I’m testing by inheriting it from my test contract.

[DaniPopes]

I think what's happening is that the vm.expectRevert call makes the test expect a revert, so when the test contract itself reverts, stopping the execution of the test, it also succeeds since we expected a revert. You can create a mock contract that just inherits the abstract contract and deploy and test with external calls on that

[mds1]

Does this happen (i.e. the test erroneously passes) if the reason the test contract reverts is due to a solidity panic like overflow or divide by zero? I don't see why you'd intentionally revert in a test contract like this, but I can imagine unintentional reverts due to math errors and want to make sure that doesn't cause tests to incorrectly pass.

I think what's happening is that the vm.expectRevert call makes the test expect a revert, so when the test contract itself reverts, stopping the execution of the test, it also succeeds since we expected a revert.

Hmm this seems odd though because my understanding is that expectRevert only expects reverts for calls made from the test contract, I don't think it should count reverts within the test contract, that feels like a bug

[0xbok]

I don't see why you'd intentionally revert in a test contract like this

I have an abstract contract that I’m testing by inheriting it from my test contract. The abstract contract has a function that can revert. The PoC that I showed in this issue is just a toy example replicating this behavior. But I'll be moving to @DaniPopes's workaround by creating a mock contract which just calls the abstract contract.

[mds1]

Got it, makes sense! I do still think we should verify the below, to make sure users can't have tests that are passing when you'd normally expect them to fail due to a revert in the test contract

Does this happen (i.e. the test erroneously passes) if the reason the test contract reverts is due to a solidity panic like overflow or divide by zero? I can imagine unintentional reverts due to math errors and want to make sure that doesn't cause tests to incorrectly pass.

[0xbok]

Yes, overflow passes as well:

pragma solidity 0.8.4;

import "forge-std/Test.sol";

contract ContractTest is Test {
    function setUp() public {}

    function rever(uint8 a) internal returns (uint8) {
        return a*2;
    }

    function testExample() public {
        vm.expectRevert();
        rever(254);

        rever(254);
        console.log("print");
    }
}

[mds1]

Thanks for testing! So IMO this is a bug that we should fix, cc @mattsse. According to the docs, expectRevert looks for a revert in the next call. In that example, there is never a call (just jumps to internal functions), so that test should fail from the overflow in rever(uint8)

[brockelmore]

So I looked into this. It is due to this line: https://github.com/foundry-rs/foundry/blob/250cc85a5a3a796454aa6a16f553f58035b10c6f/evm/src/executor/inspector/cheatcodes/mod.rs#L468 (and the create_end).

We evaluate based on <=, I dont know why we do <= instead of ==. I think the only possible case where <= is used is this exact case? So the answer may be just to switch to ==. But it is sort of hard to reason about. Switching this line to == makes the following tests pass/fail as expected:

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

import "forge-std/Test.sol";


contract ContractTest is Test {
    function setUp() public {}

    function rever() public {
        revert();
    }

    function delayed_rever() public {
        ContractTest(address(this)).rever();
    }

    function testExample() public {
        vm.expectRevert();
        rever();

        rever();
        console.log("Does not print");
    }

    function testExample2() public {
        vm.expectRevert();
        ContractTest(address(this)).rever();
    }

    function testExample3() public {
        vm.expectRevert();
        ContractTest(address(this)).delayed_rever();
    }
}

Running 3 tests for test/Counter.t.sol:ContractTest
[FAIL. Reason: EvmError: Revert] testExample() (gas: 3068)
Traces:
  [121] ContractTest::setUp()
    └─ ← ()

  [3068] ContractTest::testExample()
    ├─ [0] VM::expectRevert()
    │   └─ ← ()
    └─ ← "EvmError: Revert"

[PASS] testExample2() (gas: 3541)
Traces:
  [3541] ContractTest::testExample2()
    ├─ [0] VM::expectRevert()
    │   └─ ← ()
    ├─ [151] ContractTest::rever()
    │   └─ ← "EvmError: Revert"
    └─ ← ()

[PASS] testExample3() (gas: 4011)
Traces:
  [4011] ContractTest::testExample3()
    ├─ [0] VM::expectRevert()
    │   └─ ← ()
    ├─ [643] ContractTest::delayed_rever()
    │   ├─ [151] ContractTest::rever()
    │   │   └─ ← "EvmError: Revert"
    │   └─ ← "EvmError: Revert"
    └─ ← ()

The early return is due to the way expectRevert works, it just changes the status code of the return, so any revert/return will always stop current call context.

[0xbok]

@brockelmore the PR has been closed, so is this an intended behaviour now, or is there a different fix being discussed?

[brockelmore]

Leaving this issue open because I would like a better solution that what currently is happening:

pragma solidity 0.8.4;

import "forge-std/Test.sol";

contract ContractTest is Test {
    function rever() internal {
        revert();
    }

    // currently "intended" behavior (was not supposed to be supported but has been for too long on accident)
    function testExample() public {
        vm.expectRevert();
        rever();
        // anything after this will *NOT* execute. We cannot continue execution after a revert in the same call 
        // its a footgun that is probably just gonna be here for a little while until we can find a better solution
    }
    
}

Too many people rely on the above pattern already that we probably need to have a broader discussion and if we break the above pattern we let people know its going to break ahead of time. If you can avoid the above pattern, I would

[frangio]

The problem with using vm.expectRevert before an internall call is that, if the internal call does not revert, execution will continue and silently consume the next revert, which was not at all the developer intention. This seems highly problematic to me, I'd think it should be fixed as soon as possible.

[grandizzy]

The problem with using vm.expectRevert before an internall call is that, if the internal call does not revert, execution will continue and silently consume the next revert, which was not at all the developer intention. This seems highly problematic to me, I'd think it should be fixed as soon as possible.

In latest versions this is no longer the case, given test below

contract X {
    function externalRevert() public {
        revert();
    }
}

contract ContractTest is Test {
    function noRevert() internal {}

    function testExternalRevert() public {
        X x = new X();
        vm.expectRevert();
        x.externalRevert();
        noRevert();
    }

    function testInternalRevert() public {
        vm.expectRevert();
        noRevert();
    }
}

the testInternalRevert fails with next call did not revert as expected message, while testExternalRevert pass

Ran 2 tests for test/Balance.t.sol:ContractTest
[PASS] testExternalRevert() (gas: 46535)
[FAIL: next call did not revert as expected] testInternalRevert() (gas: 3069)

@frangio is OK to close this one? thank you!

[frangio]

I think this is what I was talking about:

contract ContractTest is Test {
    function reverts(bool b) internal {
        if (b) revert();
    }

    function testInternalRevert() public {
        vm.expectRevert();
        reverts(false); // (a)

        reverts(true); // (b)
    }
}

This test succeeds because (b) reverts, satisfying the expectRevert assertion. This is not wrong per se, but I think it may be error prone. A developer may misunderstand the scope of the assertion and think expectRevert wil be scoped to (a), after all if (a) were an external call it would be. The scoping rules are subtle (and dynamic), and my guess is that they will be unintuitive for Solidity beginners.

Feel free to close this if you disagree that it's error prone or you've found this is not a common misconception among devs.

My feeling is that there should be at least two separate assertions for what this cheatcode does: (1) expect the next external call to revert, and (2) expect the current call context to revert.

[grandizzy]

My feeling is that there should be at least two separate assertions for what this cheatcode does: (1) expect the next external call to revert, and (2) expect the current call context to revert.

Thank you! Makes sense

[frangio]

I'm not sure what I'm describing is relevant to this open issue though.

[mds1]

In addition to the issue @frangio described, the other big issue which is what the issue title is about is this:

contract ContractTest is Test {
    function reverts(bool b) internal {
        if (b) revert();
    }

    function testInternalRevert() public {
        console.log("executes");
        vm.expectRevert();
        reverts(true);
        console.log("never executes"); // All code after expectRevert silently does not execute!
    }
}

Ran 1 test for test/Counter.t.sol:ContractTest
[PASS] testInternalRevert() (gas: 6247)
Logs:
  executes

[grandizzy]

@mds1 thank you, I agree this is not intuitive and dangerous as one would expect to continue execution. What happens is that in case of a successful expectRevert we just return https://github.com/foundry-rs/foundry/blob/eb046653de4047a27b181394338732e597965257/crates/cheatcodes/src/inspector.rs#L1278 so if done at call depth 0 (test) then will just end test. Looking into a fix (or at min just warn users when this happens).

[frangio]

IMO "silently does not execute" is the reasonable behavior given how the EVM works, but I see how that's inconsistent with the way vm.expectRevert affects external calls (the revert is "silently discarded" 😄).

Deprecating vm.expectRevert and splitting this assertion in two might help by making the scope of the cheatcode clearer:

My feeling is that there should be at least two separate assertions for what this cheatcode does: (1) expect the next external call to revert, and (2) expect the current call context to revert.

[grandizzy]

(2) expect the current call context to revert.

Hm, wouldn't the same confusion be happening with this one too?

[frangio]

I think a good name for the cheatcode could help mitigate that confusion. I don't have any concrete suggestions though.

[0xbok]

Clarifying the docs for this cheatcode and having a new cheatcode vm.expectExternalRevert should be good enough fix if we don't want to change the behavior of vm.expectRevert