feat: add modifier to run afterTest in tests that inherit StdAfterTest
#616
Conversation
yash-atreya
force-pushed
the
yash/after-test-modifier
branch
from
57375a8
to
49b07a4
Compare
--------- Co-authored-by: grandizzy <grandizzy.the.egg@gmail.com>
yash-atreya
force-pushed
the
yash/after-test-modifier
branch
from
49b07a4
to
b5a84f5
Compare
|
makes sense to me, would be great to have @mds1 input too |
| pragma solidity >=0.6.2 <0.9.0; | ||
|
|
||
| // Inheritted by test contracts where afterTest() is needed | ||
| abstract contract StdAfterTest { |
There was a problem hiding this comment.
We will need to inherit from this here:
Line 30 in 4e5f46a
| // This is needs to be overriden by the Test contract that inherits this | ||
| function afterTest() public virtual; |
There was a problem hiding this comment.
Can we add some comments to document how this works exactly and what it's intended for? Does it run only after successful tests, or also failed ones? If both, I can see a use case like cleaning up tmp files. What's the use case for only running after successful test cases?
There was a problem hiding this comment.
afterTest() won't run after tests that revert. Runs in other cases such as successful tests, tests that fail due to assertions.
There was a problem hiding this comment.
Oh I see this is just a pure solidity implementation with no foundry cheat/hook, so actually doesn't it only run after successful tests?
Given that, since we want to reduce the amount of code/functionality in forge-std, this seems very simple so I would suggest closing this PR and recommending users to implement this functionality themselves as needed
There was a problem hiding this comment.
Makes sense.
Closing this and adding an example impl to the linked issue.
Ref: #617
Implementing this directly in foundry would add unnecessary complexity to
ContractRunner. See foundry-rs/foundry#8985 (comment)This approach is cleaner and easier.
Implemented the modifier and
afterTest()fn that needs to be overridden in a separate abstract contractStdAfterTest; and not in existingStdCheatsSafeas this would break existing tests. Hence, making this feature opt-in.