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

Is using mock contracts for testing necessary? #603

Closed
milosdjurica opened this issue Sep 17, 2024 · 1 comment
Closed

Is using mock contracts for testing necessary? #603

milosdjurica opened this issue Sep 17, 2024 · 1 comment

Comments

@milosdjurica
Copy link

In test cases when revert is expected, tests are following this pattern:

  1. Deploy MockContract
  2. Call method on that MockContract that is supposed to revert.

As far as I saw, mocks are used only for revert cases.

Quick example :

function test_RevertIf_ChainNotFound() public {
// We deploy a mock to properly test the revert.
StdChainsMock stdChainsMock = new StdChainsMock();
vm.expectRevert("StdChains getChain(string): Chain with alias \"does_not_exist\" not found.");
stdChainsMock.exposed_getChain("does_not_exist");
}

Why do we need to use mocks to properly test the revert?

Can't we just test revert like this? ->

    function test_RevertIf_ChainNotFound() public {
        vm.expectRevert("StdChains getChain(string): Chain with alias \"does_not_exist\" not found.");
        getChain("does_not_exist");
    }

I have tried changing tests to 2nd approach and tests are passing. I am not sure if I am missing something.
I will open the PR with changes on the StdChains.t.sol so you can get a closer look on what do I mean. Please let me know your thoughts on this :)

@mds1
Copy link
Collaborator

mds1 commented Sep 18, 2024

We intentionally use mocks because vm.expectRevert is intended to be used on the next call (or contract creation). If you remove the mocks and directly call the internal functions, you JUMP to that function. This was accidentally supported by foundry, but not a lot of people rely on it so it's not easy to remove. It's a big footgun because if you have two consecutive expectRevert calls with jumps, the test stops executing. So:

// This one is ok
vm.expectRevert("StdChains getChain(string): Chain with alias \"does_not_exist\" not found.");
getChain("does_not_exist");

// All code below here never executes! This is dangerous
vm.expectRevert("StdChains getChain(string): Chain with alias \"does_not_exist\" not found.");
getChain("does_not_exist");

I checked the forge book quickly and didn't see this documented, cc @klkvr @grandizzy @Evalir for more info

@mds1 mds1 closed this as not planned Won't fix, can't repro, duplicate, stale Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants