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

fix(cheatcodes): Correct expectRevert behavior #4945

Merged
merged 15 commits into from
May 25, 2023

Conversation

Evalir
Copy link
Member

@Evalir Evalir commented May 15, 2023

** This is a big breaking change**

Motivation

Closes #3723 , Closes #3437 , Closes #4832 .

expectRevert currently has incorrect behavior: 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 let users use the cheatcode incorrectly as illustrated in #3723.

Solution and Behavior

expectRevert now only works on the next CALL. Cheatcode calls are now ignored properly, even if those cheatcode calls revert. To illustrate, these are examples of now legal/illegal behavior:

// 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
}

On tests marked as "not working properly"

As you can see, the diff is extremely minimal in terms of the change itself. This has uncovered a few tests which are not working as expected anymore—mainly because they were relying on the broken expectRevert behavior. Some of them seem to be bugs—mainly the fs/parsing related ones.

NOTE: This PR is done—just needs a coordinated forge-std release once this goes on main, which won't be soon.

@Evalir Evalir requested review from mds1 and mattsse May 15, 2023 16:08
@Evalir Evalir changed the title fix(cheatcodes): CorrectexpectRevert behavior fix(cheatcodes): Correct expectRevert behavior May 15, 2023
@Evalir
Copy link
Member Author

Evalir commented May 15, 2023

Working on the forge-std tests—the behavior change also breaks some tests there

@Evalir Evalir changed the base branch from master to expect-changes May 24, 2023 19:14
@Evalir Evalir force-pushed the evalir/expect_revert branch from 0085d51 to 3b6e86e Compare May 25, 2023 15:16
@Evalir Evalir merged commit 10baa06 into expect-changes May 25, 2023
@Evalir Evalir deleted the evalir/expect_revert branch May 25, 2023 15:47
Evalir added a commit that referenced this pull request Jun 26, 2023
* feat(`cheatcodes`): Make expectCall only work for the next call's subcalls (#5032)

* chore: make expect call only work for the next call

* chore: make expectCall actually check only the next call's subcalls

* chore: fmt

* chore: introduce checks at the main call level, not at the subcall level

* chore: handle dangling expected calls gracefully

* chore: fix tests

* chore: fmt

* chore: forge fmt

* chore: actually exclude depth the cheatcode was called from

* chore: tests

* chore: better docs

* chore: comment out impossible to check condition on expectCall

* chore: remove unused check

* fix(cheatcodes): Correct `expectRevert` behavior (#4945)

* chore: add repro test to pass

* chore: strictly check for the depth expectRevert was called in, instead of being able to peek at function end

* chore: tests

* chore: add more repro tests

* chore: fmt

* chore: clippy

* chore: fixup problematic tests, mark them as not working properly

* chore: forge fmt

* chore: forge fmt

* Update evm/src/executor/inspector/cheatcodes/mod.rs

* chore: add more info to changelog

* chore: fmt

* chore(tests): add more cases for `expectEmit` (#5076)

* chore(tests): add more extreme cases for expectEmit

* chore(tests): add next call fail case for expectEmit

* chore(`cheatcodes`): add more edge case tests on `expect*` cheatcodes (#5135)

* chore: add edge-cases

* chore: add edge case covering #4920 (comment)

* feat(`cheatcodes`): disallow usage of `expectRevert` with `expectCall` and `expectEmit`  (#5144)

* feat(cheatcodes): disallow usage of expectCall/Emit with expectRevert

* chore: add tests

* chore: fmt

* chore: fmt
Evalir added a commit that referenced this pull request Jul 10, 2023
* feat(`cheatcodes`): `1.0` cheatcode changes (#5045)

* feat(`cheatcodes`): Make expectCall only work for the next call's subcalls (#5032)

* chore: make expect call only work for the next call

* chore: make expectCall actually check only the next call's subcalls

* chore: fmt

* chore: introduce checks at the main call level, not at the subcall level

* chore: handle dangling expected calls gracefully

* chore: fix tests

* chore: fmt

* chore: forge fmt

* chore: actually exclude depth the cheatcode was called from

* chore: tests

* chore: better docs

* chore: comment out impossible to check condition on expectCall

* chore: remove unused check

* fix(cheatcodes): Correct `expectRevert` behavior (#4945)

* chore: add repro test to pass

* chore: strictly check for the depth expectRevert was called in, instead of being able to peek at function end

* chore: tests

* chore: add more repro tests

* chore: fmt

* chore: clippy

* chore: fixup problematic tests, mark them as not working properly

* chore: forge fmt

* chore: forge fmt

* Update evm/src/executor/inspector/cheatcodes/mod.rs

* chore: add more info to changelog

* chore: fmt

* chore(tests): add more cases for `expectEmit` (#5076)

* chore(tests): add more extreme cases for expectEmit

* chore(tests): add next call fail case for expectEmit

* chore(`cheatcodes`): add more edge case tests on `expect*` cheatcodes (#5135)

* chore: add edge-cases

* chore: add edge case covering #4920 (comment)

* feat(`cheatcodes`): disallow usage of `expectRevert` with `expectCall` and `expectEmit`  (#5144)

* feat(cheatcodes): disallow usage of expectCall/Emit with expectRevert

* chore: add tests

* chore: fmt

* chore: fmt

* `foundryup`: v1 changes (#5158)

* feat(foundryup): look for v1 tag instead of nightly for normal foundryup

* feat(foundryup): add ability to download legacy nightly binary with -L flag

* feat: use latest release for figuring out the tag name

* chore(foundryup): slightly improve stable release detection

* chore: use proper repo

* make fns async

* chore: remove prb math from integration tests

* chore: forge fmt

* chore: fix some merge leftovers

* chore: last test fixes

* chore: forge fmt

* chore: uncomment etch test

* feat(docs): add `RELEASE_PROCESS.md` (#5269)

* feat(docs): add RELEASE_PROCESS.md

* chore: not include changelog changes in step

* chore: bump crates to 1.0.0 (#5346)
Evalir added a commit that referenced this pull request Jul 10, 2023
* feat(`cheatcodes`): `1.0` cheatcode changes (#5045)

* feat(`cheatcodes`): Make expectCall only work for the next call's subcalls (#5032)

* chore: make expect call only work for the next call

* chore: make expectCall actually check only the next call's subcalls

* chore: fmt

* chore: introduce checks at the main call level, not at the subcall level

* chore: handle dangling expected calls gracefully

* chore: fix tests

* chore: fmt

* chore: forge fmt

* chore: actually exclude depth the cheatcode was called from

* chore: tests

* chore: better docs

* chore: comment out impossible to check condition on expectCall

* chore: remove unused check

* fix(cheatcodes): Correct `expectRevert` behavior (#4945)

* chore: add repro test to pass

* chore: strictly check for the depth expectRevert was called in, instead of being able to peek at function end

* chore: tests

* chore: add more repro tests

* chore: fmt

* chore: clippy

* chore: fixup problematic tests, mark them as not working properly

* chore: forge fmt

* chore: forge fmt

* Update evm/src/executor/inspector/cheatcodes/mod.rs

* chore: add more info to changelog

* chore: fmt

* chore(tests): add more cases for `expectEmit` (#5076)

* chore(tests): add more extreme cases for expectEmit

* chore(tests): add next call fail case for expectEmit

* chore(`cheatcodes`): add more edge case tests on `expect*` cheatcodes (#5135)

* chore: add edge-cases

* chore: add edge case covering #4920 (comment)

* feat(`cheatcodes`): disallow usage of `expectRevert` with `expectCall` and `expectEmit`  (#5144)

* feat(cheatcodes): disallow usage of expectCall/Emit with expectRevert

* chore: add tests

* chore: fmt

* chore: fmt

* `foundryup`: v1 changes (#5158)

* feat(foundryup): look for v1 tag instead of nightly for normal foundryup

* feat(foundryup): add ability to download legacy nightly binary with -L flag

* feat: use latest release for figuring out the tag name

* chore(foundryup): slightly improve stable release detection

* chore: use proper repo

* make fns async

* chore: remove prb math from integration tests

* chore: forge fmt

* chore: fix some merge leftovers

* chore: last test fixes

* chore: forge fmt

* chore: uncomment etch test

* feat(docs): add `RELEASE_PROCESS.md` (#5269)

* feat(docs): add RELEASE_PROCESS.md

* chore: not include changelog changes in step

* chore: bump crates to 1.0.0 (#5346)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants