Forge doesn't work with multiple "vm.expectRevert" when testing free functions

[PaulRBerg]

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 (cb925b1 2022-11-21T00:09:19.970948Z)

What command(s) is the bug in?

forge test

Operating System

macOS (Apple Silicon)

Describe the bug

Given the following test:

function testMultipleReverts() public {
    vm.expectRevert();
    revert();

    vm.expectRevert();
    console2.log("Do not revert");
}

Forge will consider the test to be passed, even if it should be failed, because there is no second revert.

[mds1]

Forge does work with multiple vm.expectReverts, but it operates on external calls only. For example the below test will fail

contract X {
  function foo(bool shouldRevert) public {
    if (shouldRevert) revert();
  }
}

contract Test6 is Test {
    function testMultipleReverts() public {
      X x = new X();
      
      vm.expectRevert();
      x.foo(true);

      vm.expectRevert();
      x.foo(false);
  }
}

Though there may still be a problem in your example, not certain so deferring to @mattsse for these questions:

• revert() is not an external call so that should be ignored and cause the test to fail? Execution does stop after that line, but the test is marked passed, not failed
• I believe console.log calls are intercepted and not considered for cheatcodes, which relates to the next item
• Presumably a trailing expectRevert with no call following should cause a failure? Similarly for all expect* cheats, but not sure if this was previously discussed and decided to be this way for some reason

[PaulRBerg]

Context

You're right @mds1, thanks for explaining. You have helped me get to the bottom of this issue.

My problem situation is that I am using free functions that can revert, e.g. the ceil function which I defined like this:

function ceil(SD59x18 x) pure returns (SD59x18 result) {
    if (x.gt(MAX_WHOLE_SD59x18)) {
        revert PRBMathSD59x18__CeilOverflow(x);
    }
    // ...
}

Sample Tests

I played a little with Forge and I managed to identify two scenarios starting from the following free function:

function doRevert(bool shouldRevert) pure {
    if (shouldRevert) {
        revert();
    }
}

Simple Revert

Works well ✅

function testExpectRevertPasses() external {
    vm.expectRevert();
    doRevert(true);
}

function testExpectRevertFails() external {
    doRevert(false);
}

Multiple Reverts

Doesn't fail when it should fail ❌

// Doesn't fail!
function testExpectRevertFails() external {
    vm.expectRevert();
    doRevert(true);

    // The test execution doesn't reach this
    vm.expectRevert();
    doRevert(false);
}

Workarounds

As I see it, there are two immediate solutions to this:

1. Wrap all of my free functions in an intermediary mock contract used for testing (adds maintenance costs though).
2. Write one test function per revert case.

I opted for the 2nd solution for the time being, since I do not have that many inputs for the revert cases. One input per revert suffices.

However, I think that this is a big footgun when testing free functions with Forge. My expectation is that free functions will become more popular with time, thanks to the introduction of global binding of using for introduced in Solidity v0.8.13.

What was the rationale behind making Forge look only for reverts in external calls?

[mds1]

Hooking into CALLs is much easier than hooking into JUMPs, so this applies to many cheatcodes, more info in #864 and #432 and probably some other issues.

FWIW I think there are a few EOF-related EIPs in consideration that should make this easier in the future. But until then unfortunately you'll have to use one of those workarounds unless @mattsse has other ideas.

Though I do agree the current handling is not ideal, we probably should:

1. Mark a test failed if the test method itself reverts, no matter what, since I think the fact that your testExpectRevertPasses works is not intended behavior
2. Fail whenever there is a dangling expect* cheat

@PaulRBerg @mattsse lmk your thoughts on that

Maybe also related to #1745

[PaulRBerg]

Hooking into CALLs is much easier than hooking into JUMPs, so this applies to many cheatcodes

Oh, I see.

more info in #3723

A bit of infinite recursion referencing #3723 in #3723!

there are a few EOF-related EIPs in consideration

Yeah, looking forward to EOF. Seems like it's scheduled to be included in Shanghai.

Mark a test failed if the test method itself reverts

This would be a breaking change for many Forge users.

since I think the fact that your testExpectRevertPasses works is not intended behavior

Actually, it is. As I said, I am testing an external function. I want the test to pass when I expect a revert and the free function reverts.

Fail whenever there is a dangling expect* cheat

This would be useful.

[mds1]

A bit of infinite recursion referencing #3723 in #3723!

Oops, just updated to #864

since I think the fact that your testExpectRevertPasses works is not intended behavior

Actually, it is. As I said, I am testing an external function. I want the test to pass when I expect a revert and the free function reverts.

To clarify I mean at the forge-level it's (afaik) not intended behavior, as shown in #864: expectRevert should only be changing the result when there are CALLs, but the above example shows that expectRevert changes a the pass/fail status of a test which reverts directly in the test function, with no external calls

[PaulRBerg]

at the forge-level it's (afaik) not intended behavior

Oh, I see.

Whatever the case, if you do end up modifying the current behavior of expectRevert, that may warrant posting an update in the Telegram group chats, to notify everyone of the new behavior.

[0xbok]

related issue: #3437

[brockelmore]

This code:

function testMultipleReverts() public {
    vm.expectRevert();
    revert();

    vm.expectRevert();
    console2.log("Do not revert");
}

Makes no sense because the optimizer will always optimize anything out that it knows is unreachable fwiw

[PaulRBerg]

@brockelmore with that code snippet I have tried to simulate a real-world scenario in which multiple reverts are needed, e.g. the case of table tests for tests that expect a revert.

See this related issue in my PRBMath repository: PaulRBerg/prb-math#129.

[brockelmore]

See this related issue in my PRBMath repository: PaulRBerg/prb-math#129.

FWIW, technically most of the test pattern usage here was actually not suppose to be allowed (i.e. expectRevert working at the same call depth & without an external call), but there was a bug in the code (see #3820). We have generally decided that we will likely continue to support this due to how many people rely on this pattern now and try to make the footgun less painful by emitting a warning when you may try to do something you can't.

There is a non-zero chance we could eventually support multiple conditional reverts at the same call depth, but not promising anything as of now (I have an MVP but its not robust & is easily broken).