-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Test function early returns after vm.expectRevert
#3437
Comments
@DaniPopes there's something off here, do you have the bandwidth to look into this? |
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 |
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. |
I think what's happening is that the |
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.
Hmm this seems odd though because my understanding is that |
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. |
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
|
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");
}
} |
So I looked into this. It is due to this line:
create_end ).
We evaluate based on // 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 |
@brockelmore the PR has been closed, so is this an intended behaviour now, or is there a different fix being discussed? |
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 |
The problem with using |
vm.expertRevert has surprising behavior when used before non-external calls: Anything after the call will not execute. See this GitHub issue and comment for more details: foundry-rs/foundry#3437 (comment) Expecting reverts for non-external calls is therefore only safe if the call is last in the test. This commit fixes two tests in TickAndBinTest which assumed that multiple reverts could be expected within a single test. The tests are split into one test per expected revert. The commit also fixes an error in one of the test cases which this change uncovered: The test assumed that it was possible to pass a too small `exp` to `TickLib.tickFromNormalizedRatio`. However, MAX_RATIO_EXP is zero and the exp parameter is uint, so it is not possible pass an exp value that is smaller than MAX_RATIO_EXP, since uint(MAX_RATIO_EXP - 1) = uint(-1) = type(uint)max.
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 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! |
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 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. |
Thank you! Makes sense |
I'm not sure what I'm describing is relevant to this open issue though. |
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!
}
}
|
@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 foundry/crates/cheatcodes/src/inspector.rs Line 1278 in eb04665
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). |
IMO "silently does not execute" is the reasonable behavior given how the EVM works, but I see how that's inconsistent with the way Deprecating
|
Hm, wouldn't the same confusion be happening with this one too? |
I think a good name for the cheatcode could help mitigate that confusion. I don't have any concrete suggestions though. |
Clarifying the docs for this cheatcode and having a new cheatcode |
Component
Forge
Have you ensured that all of these are up to date?
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
Running
forge test -vvv
passes this test and also doesn't print "Does not print" on console. In summary, aftervm.expectRevert()
the test just executes the call and does not run the remaining test.The text was updated successfully, but these errors were encountered: