[expectRevert v1 changes] expectRevert seems to cause a revert to not happen rather than asserting that it does

[thedavidmeister]

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 (e488e2b 2023-07-10T15:17:42.605282000Z)

What command(s) is the bug in?

forge test

Operating System

macOS (Apple Silicon)

Describe the bug

I wrote this test

    function testEmptyOracleSimple() external {
        vm.expectRevert(bytes(""));
        uint256 price = LibChainlink.price(address(0), type(uint256).max, 0);
        (price);
    }

for this function

    function price(address feed, uint256 staleAfter, uint256 scalingFlags) internal view returns (uint256) {
        (uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound) =
            AggregatorV3Interface(feed).latestRoundData();
        (roundId);
        (startedAt);
        (answeredInRound);

        return roundDataToPrice(
            block.timestamp, staleAfter, scalingFlags, answer, updatedAt, AggregatorV3Interface(feed).decimals()
        );
    }

so that AggregatorV3Interface(feed).latestRoundData() is called on address(0) which should revert.

but i get this

Encountered 1 failing test in test/lib/LibChainlink.badOracle.t.sol:LibChainlinkBadOracleTest
[FAIL. Reason: Call did not revert as expected] testEmptyOracleSimple() (gas: 6186)

I get the same thing with vm.expectRevert() (without the bytes argument).

then removing vm.expectRevert() entirely gives

Encountered 1 failing test in test/lib/LibChainlink.badOracle.t.sol:LibChainlinkBadOracleTest
[FAIL. Reason: EvmError: Revert] testEmptyOracleSimple() (gas: 3018)

So it does revert, and the debugger shows REVERT opcode.

Somehow the expectRevert is not seeing the REVERT.

here is it reverting https://github.com/rainprotocol/rain.chainlink/actions/runs/5530236425/jobs/10089379218?pr=1

here it is failing to revert https://github.com/rainprotocol/rain.chainlink/actions/runs/5530261812/jobs/10089440012

[mds1]

This is because of recent expectRevert changes—you must refactor to only use expectRevert on calls.

When you do uint256 price = LibChainlink.price(address(0), type(uint256).max, 0);, since price is an internal method, there is no CALL there, and instead you JUMP to the method.

Updated docs are still a WIP but you can find more info in the PR for them here: foundry-rs/book#922

[thedavidmeister]

@mds1 why would you change expectRevert to work that way?

clearly the ability to revert internal functions needs testing, how can i do that?

refactoring code is not a good way to approach this, because the refactor you're suggesting would require putting an external interface between the test and the code being tested, which introduces risk in the testing process

[thedavidmeister]

@thedavidmeister i read the linked docs and the fundamental idea of these changes precludes testing that code that calls other code (a.k.a "an abstraction") behaves the same way as the composition of the underlying code

this is a bug in foundry, despite it being intentionally introduced and documented, and we should reopen this issue

[gakonst]

Hey @thedavidmeister thanks for the feedback.

• Would love to have a productive conversation about this, while being open minded about what the right behavior moving forward is.
• Would also strongly appreciate if we could keep the tone conversational vs strongly opinionated, as we're juggling a lot of balls, and user feedback doesn't always come in with the right timing - and we may miss some important use cases as we make changes.
• We all want to keep the vibe nice and friendly vs mildly tilted (while acknowledging that this is very serious!), you know what I mean?

Now on the actual problem...

We have asked some users about this before, and they reported a "soundness issue" around dangling expectReverts:

• Issue discussion: Test function early returns after vm.expectRevert #3437, cheatcodes: expectRevert mangles return data on low level calls #5159
• Proposed Fix: fix(forge): Change depth check for expect revert #3820 (this didn't make it in then, but we cleaned it up and added it later

Fixing that soundness issue would require a breaking change, which indeed required a refactor to fix. We discussed (again) with users and people were OK with doing the refactor.

Clearly there's a soundness issue on the one hand. On the other hand there's downstream cost imposed on developers due to the breaking change. And obviously you want to test internal functions on libraries.

Our intent post-Foundry 1.0 is to be stable, and not have breaking changes like this. Pre 1.0 though we could not promise something around it, and we wanted to be open to making fixes that set us up for the long term.

I understand that it is a PITA to have to do this. I would like to understand better what you mean by "introduces risk in the testing process". Could you also explain your concrete requirements from expectRevert, so we can try to meet them?

Does that make sense? Thanks for using Foundry, we take customer feedback very seriously :)

[thedavidmeister]

@gakonst i need to get a better idea of how the foundry team expects people to write tests

obviously you want to test internal functions on libraries.

this basically, so what's the plan?

what would a canonical refactor to the snippet I posted in the issue description look like?

i don't really care exactly how it is done, as long as it is not unreasonably clunky/boilerplatey

The risk that i'm talking about is adding layers of abstraction between the test and the thing being tested, each layer could itself harbour bugs

For the same reason that i want to test each abstraction in my production code its own right, i also don't want to introduce untestable abstractions in the testing code to make the test tooling work

[grandizzy]

what would a canonical refactor to the snippet I posted in the issue description look like?

@thedavidmeister the snippet can be refactored to sample below (see another sample here #4405 (comment)) in order to make it work

    function testEmptyOracleSimple() external {
        vm.expectRevert(bytes(""));
        uint256 price = this.getChainlinkPrice(address(0), type(uint256).max, 0);
    }

    function getChainlinkPrice(address feed, uint256 staleAfter, uint256 scalingFlags) public returns (uint256) {
        return LibChainlink.price(feed, staleAfter, scalingFlags);
    }

[thedavidmeister]

@grandizzy thanks, i've been doing similar