search

foundry-rs / foundry

Foundry is a blazing fast, portable and modular toolkit for Ethereum application development written in Rust.
https://getfoundry.sh
Apache License 2.0
8.33k stars 1.76k forks source link

testFail tests do not report the revert reason #3586

Closed norswap closed 4 months ago

norswap commented 2 years ago

Component

Forge

Have you ensured that all of these are up to date?

What version of Foundry are you on?

forge 0.2.0 (0e1d52d 2022-10-31T00:04:54.74416Z)

What command(s) is the bug in?

forge test -vvvv

Operating System

macOS (Apple Silicon)

Describe the bug

When running a test that is supposed to fail (testFailXXX), the reason for the revert is not given, appearing only as "EvmError: Revert".

Meanwhile, on test that are supposed to succeed, the exact same revert will list the reason if available (e.g. "ERC20: insufficient allowance").

It's pretty useful to see the actual revert reason as a sanity check that the test fails for the expected reason. It seems like it should be possible to make the behaviour symmetric between testXXX and testFailXXX.

mds1 commented 2 years ago

Just curious, why do you use testFail? IMO this is an antipattern, mainly supported for backwards compatibility with dapptools which did not have vm.expectRevert. My suggestion would be to never use testFail, since it will cause the test to pass on any failure, not just a specific revert reason. Per the WIP best practices (https://github.com/foundry-rs/book/pull/686), you can use a naming convention like test_RevertIf_Description to make it easier to identify and filter on revert tests

norswap commented 2 years ago

In this particular case I considered it, but I was testing that a function reverted if the contract wasn't initialized. Since the initialization set some addresses, it meant that the function would fail anyway by making a call to address 0 and so an explicit require is not needed.

The revert in this case has no message, so I could use vm.expectRevert(""). I guess it is better than testFail because at least the test won't pass if failing for something that has a message. I will make this change!

Could be a good addition to the best practices also!

zerosnacks commented 4 months ago

Marking as stale / wontfix as testFail is an antipattern maintained for backwards compatibility