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

feat(cheatcodes): V1 expectRevert behavior #7238

Open
Tracked by #7613
Evalir opened this issue Feb 25, 2024 · 0 comments
Open
Tracked by #7613

feat(cheatcodes): V1 expectRevert behavior #7238

Evalir opened this issue Feb 25, 2024 · 0 comments
Labels
A-cheatcodes Area: cheatcodes T-likely-breaking Type: requires changes that can be breaking T-to-discuss Type: requires discussion

Comments

@Evalir
Copy link
Member

Evalir commented Feb 25, 2024

Motivation

expect* cheatcodes are ALL intended to function only for the next call. That means, if the next call does not fulfill the expect* set up before making it, tests should fail. However, this is not the case for expectRevert works right now.

See: #3723, #3437, #4832 .

expectRevert, since inception, works differently from intended: it matches reverts at the test level, not at the next call level. This means that reverting cheatcodes were being incorrectly catched by expectRevert (we're supposed to bypass checks for cheatcode calls) and also led users to use cheatcode incorrectly as illustrated in #3723, sometimes becoming actual accepted foundry patterns. It also meant that, if used to catch any revert without matching revert data, any revert at any point in the test after the expectRevert cheatcode was used could make the test pass. This is especially dangerous as it can hide actual code failures from intended reverts.

New behavior

expectRevert now only works on the next CALL. Cheatcode calls are now ignored properly, even if those cheatcode calls revert. To illustrate, after introducing this new behavior, these are examples of now both passing and failing tests:

// OK
function testExpectRevertString() public {
    Reverter reverter = new Reverter();
    cheats.expectRevert("revert");
    reverter.revertWithMessage("revert");
}

// FAIL
function testFailRevertNotOnImmediateNextCall() public {
    Reverter reverter = new Reverter();
    // expectRevert should only work for the next call. However,
    // we do not inmediately revert, so,
    // we fail.
    cheats.expectRevert("revert");
    reverter.doNotRevert();
    reverter.revertWithMessage("revert");
}

// FAIL
function testFailCheatcodeRevert() public {
  // This expectRevert is hanging, as the next cheatcode call is ignored.
  vm.expectRevert();
  vm.fsMetadata(fakePath) // try to go to some non-existent path to cause a revert
}

@Evalir Evalir added the A-cheatcodes Area: cheatcodes label Feb 25, 2024
@Evalir Evalir added this to the v1.0.0 milestone Feb 25, 2024
@zerosnacks zerosnacks added the T-likely-breaking Type: requires changes that can be breaking label Jul 11, 2024
@zerosnacks zerosnacks added the T-to-discuss Type: requires discussion label Jul 31, 2024
@zerosnacks zerosnacks removed this from the v1.0.0 milestone Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cheatcodes Area: cheatcodes T-likely-breaking Type: requires changes that can be breaking T-to-discuss Type: requires discussion
Projects
None yet
Development

No branches or pull requests

2 participants