bug: snapshot/revertTo causes tests to pass unexpectedly

[mds1]

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 (48d5d79 2022-09-01T00:10:22.933552Z)

What command(s) is the bug in?

forge test

Operating System

No response

Describe the bug

If you run this test, it passes. If you comment out the vm.revertTo line it fails. Failure is expected in both cases since the assertion should fail. It seems like the cause is that reverting the state also reverts the failed flag: https://github.com/dapphub/ds-test/blob/9310e879db8ba3ea6d5c6489a579118fd264a3f5/src/test.sol#L47-L63

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.15;
import "forge-std/Test.sol";

contract MyTest is Test {
  function test_snapshot() external {
    uint256 snapId = vm.snapshot();
    assertEq(uint256(0), uint256(1));
    vm.revertTo(snapId);
  }
}

[mattsse]

It seems like the cause is that reverting the state also reverts the failed flag:

that sounds right, need to make a check for failed() here

[mattsse]

with #3087 this now results in

~/git/rust/foundry/target/debug/forge test                                                                                                           ! master 
[⠢] Compiling...
No files changed, compilation skipped

Running 1 test for test/Counter.t.sol:MyTest
[FAIL. Reason: Assertion failed.] test_snapshot() (gas: 15534)
Test result: FAILED. 0 passed; 1 failed; finished in 2.71ms

Failing tests:
Encountered 1 failing test in test/Counter.t.sol:MyTest
[FAIL. Reason: Assertion failed.] test_snapshot() (gas: 15534)

Encountered a total of 1 failing tests, 0 tests succeeded

[graykode]

This still doesn't work. But after modifying a few lines in forge-std, it works. foundry-rs/forge-std#241 @mattsse

[mds1]

@mattsse I was able to reproduce the issue from @graykode, any idea on the cause?

The forge-std fix in foundry-rs/forge-std#241 does resolve the issue, but I'm not sure why. Regardless though, seems this should be fixed upstream here in foundry instead?

[mattsse]

looks strange, I have to take a closer look

[ZeroEkkusu]

@mattsse, see foundry-rs/forge-std#241 (review). The underlying issue needs to be identified.

[xenoliss]

Hey ! I'm still facing this issue on forge std c7d30303fc34b7b2a680c86ddc1cd186bec23614. Here is a PoC code:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.17;

import "forge-std/Test.sol";

contract TestMe is Test {
    function testMe() public {
        uint256 snapshot = vm.snapshot();
        assertTrue(false);

        vm.revertTo(snapshot);
        assertTrue(true);
    }
}

This outputs the following:

$ forge test --match-test testMe -vv
[⠒] Compiling...
No files changed, compilation skipped

Running 1 test for test/TestMe.t.sol:TestMe
[PASS] testMe() (gas: 11436)
Logs:
  Error: Assertion Failed

The only was to view the failing test is using a more verbose mode like -vv, else this is totally invisible.
Also when nesting snapshots, the logs are simply vanished and -vv does not log the errors.

[OliverNChalk]

For those facing this issue, you may wish to use the following as a temp workaround:

    function _revertTo(uint256 aSnapshot) private {
        require(!failed(), "Fail flag triggered, aborting revert");

        vm.revertTo(aSnapshot);
    }

[mattsse]

hmm, I'm not quite sure what the issue is here, because both examples fail.

or is the problem that the first assertion doesn't stop execution?

this behavior has ds-test legacy reasons but I'm skeptical that this is any good...

[OliverNChalk]

Interesting, I can get logged test failures without the test failing:

I am on a fork while using vm.revertTo. My forge version is forge 0.2.0 (0f1dea4 2023-03-31T00:07:29.356304995Z). Let me try get you a repro a bit later

[OliverNChalk]

@mattsse It appears that fuzz testing breaks whatever code was introduced to persist the failure state. That said, I vaguely remember seeing this fail prior to fuzzing. However, perhaps they share the same root cause.

This minimal repro should make it easier to track down:
https://github.com/OliverNChalk/revert-to-repro

[mattsse]

thanks, will check!