Function Modifier or an Opcode to mark a test as skipped conditionally

[vminkov]

Component

Forge

Describe the feature you would like

It would be nice to have a Forge provided modifier that lets one mark a test as skipped [SKIP] depending on some custom condition. The specific use case we need it for is to mark which tests are run on different mainnet forks

The code that we currently use looks like this (below) but it marks the skipped tests as passed [PASS]

  uint256 BSC_MAINNET = 56;
  uint256 ETH_KOVAN = 42;
  uint256 ETH_RINKEBY = 4;
  uint256 EVMOS_TESTNET = 9000;

  /// custom modifier for running tests conditionally
  modifier shouldRun(bool run) {
    if (run) {
      _;
    }
  }

 // filter function for a specific chain
  function forChains(uint256 id0) public view returns (bool) {
    return block.chainid == id0;
  }

 // filter function for multiple chains
  function forChains(uint256 id0, uint256 id1) public view returns (bool) {
    return block.chainid == id0 || block.chainid == id1;
  }


  // TEST CASES


  function testSomething() public {
    // this test will be run on all chains
  }
 
  function testSomethingElse() public shouldRun(forChains(BSC, ETH_KOVAN)) {
    // this test will be skipped on EVMOS_TESTNET and ETH_RINKEBY
  }

I would imagine that we're provided by the forge env either a modifier like shouldRun(forChains(...)) or some assembly opcode to return from the test marking it as skipped [SKIP] and then we can craft the modifier in the same way

  modifier shouldRun(bool run) {
    if (run) {
      _;
    }
    else {
      assembly {
        some_op_code_to_mark_test_as_skipped()
      }
    }
  }

Additional context

[onbjerg]

Note on implementation: This could be a cheatcode (vm.skip(bool)) that simply reverts with some magic bytes. When a test reverts, we then check if the revert is the magic bytes, and if it is, we mark the test as skipped instead of failed

Others could then implement their custom modifiers, and forge-std could also have one, something like:

modifier shouldRun(bool pred) {
  vm.skip(!pred);
  _;
}

// Or
modifier shouldRunForChains(uint256 id0, uint256 id1) public view returns (bool) {
  vm.skip(block.chainid != id0 && block.chainid != id1);
  _;
}

The bulk of the work is not the cheatcode itself, but adjusting all of the test-related code to move away from success (true/false) to an enum

Marked it as low priority since it can already be solved using custom solutions, even if the outcome is not 100% desireable

[0x00000002]

    /**
     * @notice this modifier will skip the testFail*** ONLY
     */
    modifier skipFail(bool isSkipped) {
        if (!isSkipped) {
            require(0 == 1); 
            _;
        }
    }```

[onbjerg]

I wonder if this is needed anymore since we now have fork-related cheatcodes to choose what fork to use on a per-test level? Wdyt @vminkov? I can't really think of a use case beyond that.

[mds1]

I still think this feature is useful in some cases. For example with hardhat, I'd sometimes scaffold out a bunch of unimplemented tests and mark them as skipped to signal that they're not implemented. Having them pass/fail didn't feel correct since it might be a while until all tests are implemented.

Instead of a cheatcode, another idea is a special test name prefix, like function skipTest*() instead of the usual function test*(). (Either cheatcode or function name is fine with me, no preference here)

I don't feel too strongly about this feature and am ok marking this closed, pending @vminkov's thoughts

[vminkov]

For our use case the fork-related cheatcodes are good enough alternative. It is still useful to mark a test case as skipped when the reasons for its failure are not related to the code that is tested, but to the fork RPC URL being invalid, for example.

[mattsse]

Instead of a cheatcode, another idea is a special test name prefix, like function skipTest*() instead of the usual function test*(). (Either cheatcode or function name is fine with me, no preference here)

I think this would be the best fix here, since we identify tests via naming convention, simply adding skip, or ignore feels more appropriate than adding a vm.skip() call.
this prefix will ignore it automatically, we jus need to include hem in the output as "ignored"

[gakonst]

I thought the OP wanted to skip tests on certain conditions like the chain id, which the skiptest idea I think does not address

[mds1]

Probably should have left this comment in this issue instead, so linking to it here #2606 (comment)

Ah good point, it would be nice to support contract-level skips too, but contract SkipMyContract would be a new naming approach + seems more likely to clash with a real contract name than a test prefix. I'd be ok with it, but could also be an argument to go the cheatcode route instead.

The cheatcode approach does give more flexibility, since you can have vm.skip(bool) and put some condition in there, so I think I've convinced myself that it's the better approach

[alcueca]

I've got another use case for this feature, which I originally described in #3845, and that now I'm moving here.

When coding a test harness for live contracts, it is not possible to know in advance the state the the smart contracts will be in. Therefore, some tests will make sense, and others will not.

For example, we might have a set of tests that make sense on a given uninitialized contract, but once the contract is initialized it would make more sense to skip those tests and move on to the next ones in the same file.

The workaround described above still works, but the PASS in the output might still lead to confusion as to what is the current state of my contracts.

I would very much prefer if the test would show as SKIP, and maybe even in the summary at the end ("67 passed, 32 skipped, 0 failed", for example). vm.skip(bool) would work just fine.

Additional context
In this test file, this test only makes sense in the contract is in an EJECTED state after the setup function.

[mds1]

Copying some nice from #4918 here, cc @shazow

• Two new cheats: vm.skip(bool skip) and vm.skip(bool skip, string calldata reason), where the latter prints a reason string
• Ensure the skip call can be made at the test level or setUp level

cc @Evalir, might be an easy one and this is often requested