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

"InvariantTest" does not have a "vm" #282

Closed
PaulRBerg opened this issue Jan 25, 2023 · 10 comments · Fixed by #292
Closed

"InvariantTest" does not have a "vm" #282

PaulRBerg opened this issue Jan 25, 2023 · 10 comments · Fixed by #292

Comments

@PaulRBerg
Copy link
Contributor

PaulRBerg commented Jan 25, 2023

No cheatcodes can currently be used in a test contract that inherits just from InvariantTest.

I don't know if this is intended - but if it is, we should definitely mention what's the recommend way of adding the vm (in foundry-rs/book#760), e.g. inherit from Test, or TestBase, define a vm yourself, etc.

@mds1
Copy link
Collaborator

mds1 commented Jan 26, 2023

You would inherit from both, e.g. MyInvariantTest is Test, InvariantTest. The latter just adds some invariant helper methods to your contract. Now that I think of it, I wonder if the content of InvariantTest should just be part of Test by default, wdyt? cc @gakonst @lucas-manuel for thoughts too

@lucas-manuel
Copy link
Contributor

I think since invariants are a subset of overall testing it makes sense to do Test, InvariantTest. However if this is something that people will run into often I can see the argument for adding both the Test though.

Comes down to compilation time vs. ease of use I guess. What do you think is best here @mds1?

@PaulRBerg
Copy link
Contributor Author

Now that I think of it, I wonder if the content of InvariantTest should just be part of Test by default, wdyt?

Works for me.

Comes down to compilation time vs. ease of use

Compilation times should not be a concern in this case - compiling a dozen or so functions should be a super quick task on modern computers.

@PaulRBerg
Copy link
Contributor Author

Another benefit of merging InvariantTest with Test is that it would partially address this issue:

foundry-rs/foundry#4162

That is, invariant tests would no longer be considered something qualitatively different from fuzz tests - which makes sense, since inputs are fuzzed in invariants tests, too.

@gakonst
Copy link
Member

gakonst commented Jan 27, 2023

Let's do contract Test is InvariantTest then?

@mds1
Copy link
Collaborator

mds1 commented Jan 31, 2023

I'm good with that. This would be a breaking change, so we'll release a v1.4.0 after?

@gakonst
Copy link
Member

gakonst commented Jan 31, 2023

SGTM.

@Sabnock01
Copy link
Contributor

Sabnock01 commented Feb 2, 2023

Shouldn't the invariant testing utilities follow the same pattern as everything else?

i.e. have the file StdInvariant.sol inherited by Test.sol?

Very much in favor of keeping Test.sol as the superset as well.

@mds1
Copy link
Collaborator

mds1 commented Feb 2, 2023

i.e. have the file StdInvariant.sol inherited by Test.sol?

Yea this is what I was planning to do actually, will open a PR for it sometime this week

@mds1
Copy link
Collaborator

mds1 commented Feb 3, 2023

PR for this is ready here if anyone can give a quick review 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants