-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(forge
): remove testFail*
#9574
Conversation
forge
): deprecate testFail*
forge
): remove testFail*
Moved to ExpectRevertFailures.t.sol in cli tests
60e0887
to
1eda9a5
Compare
forge
): remove testFail*
forge
): remove testFail*
This reverts commit a9e7568.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
ExtTester::new("transmissions11", "solmate", "c892309933b25c03d32b1b0d674df7ae292ba925"); | ||
// <https://github.com/transmissions11/solmate> | ||
// #[test] | ||
// fn solmate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this going to be replaced with something else or should we remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed a903ee0
.args(["--chain-id", "99", "--sender", "0x00a329c0648769A73afAc7F9381E08FB43dBEA72"]) | ||
fn snekmate() { | ||
ExtTester::new("pcaversaccio", "snekmate", "df226f4a45e86c8f8c3ff1f9fa3443d260002050") | ||
.args(["--nmc", "ERC4626VaultTest"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, this is not in snekmate master anymore, we'll update once a stable 0.4.1 vyper version to pin in CI
#[test] | ||
fn forge_std() { | ||
ExtTester::new("foundry-rs", "forge-std", "2b59872eee0b8088ddcade39fe8c041e17bb79c0") | ||
ExtTester::new("foundry-rs", "forge-std", "08d6af5d6c8a9a60e308b689cd19751876d321e0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for future reference, this commit is from this PR: foundry-rs/forge-std#643
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, maybe we could have additional reviewers for that forge-std PR, bump @mds1 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
In preparation to deprecate `testFail*`: foundry-rs/foundry#9574 - Removes `deployMockERC721` and `deployMockERC20` from `StdUtils` as their tests includes large amount of `testFail*` tests and is generally outdated. I haven't been able to find the `deploy*` methods being used in any public project on Github. - Implements workaround for other cases Alternatively we could upstream Solady's mocks and tests (which have been updated to not use `testFail*` but this feels out of place and would be incomplete (ERC4626, ERC6909, etc.. https://github.com/Vectorized/solady/tree/main/test/utils/mocks). I think it is most common for people to rely on the mocks their library comes with (be it Solmate, Solady or OpenZeppelin) or write their own wrapper around the token implementation.
Motivation
Closes #4437
Solution
testFail*
.testFail*
, to cli tests in order to assert internal tooling failure. See failure_assertions.rs