From 43bfc20c13273c5d7962fb8d1c60f7668059028f Mon Sep 17 00:00:00 2001 From: grandizzy <38490174+grandizzy@users.noreply.github.com> Date: Mon, 3 Feb 2025 11:49:41 +0200 Subject: [PATCH] Update book to reflect testFail removal (#1420) --- projects/cheatcodes/test/OwnerUpOnly.t.sol | 8 ----- projects/writing_tests/test/Basic.t.sol | 6 ++-- src/forge/cheatcodes.md | 29 +------------------ src/forge/gas-function-snapshots.md | 24 +++++++-------- src/forge/writing-tests.md | 6 +--- .../forge-test-cheatcodes-expectrevert | 9 +++--- src/output/nft_tutorial/forge-test | 16 +++++----- src/reference/ds-test.md | 4 +-- src/reference/forge-std/assertApproxEqAbs.md | 12 ++------ src/reference/forge-std/assertApproxEqRel.md | 12 ++------ src/reference/forge/forge-test.md | 6 ++-- src/tutorials/solmate-nft.md | 2 +- src/tutorials/testing-eip712.md | 6 ++-- 13 files changed, 47 insertions(+), 93 deletions(-) diff --git a/projects/cheatcodes/test/OwnerUpOnly.t.sol b/projects/cheatcodes/test/OwnerUpOnly.t.sol index 79cd3dbff..9520b2cd4 100644 --- a/projects/cheatcodes/test/OwnerUpOnly.t.sol +++ b/projects/cheatcodes/test/OwnerUpOnly.t.sol @@ -44,15 +44,7 @@ contract OwnerUpOnlyTest is Test { } // ANCHOR_END: simple_test - // ANCHOR: test_fail - function testFail_IncrementAsNotOwner() public { - vm.prank(address(0)); - upOnly.increment(); - } - // ANCHOR_END: test_fail - // ANCHOR: test_expectrevert - // Notice that we replaced `testFail` with `test` function test_RevertWhen_CallerIsNotOwner() public { vm.expectRevert(Unauthorized.selector); vm.prank(address(0)); diff --git a/projects/writing_tests/test/Basic.t.sol b/projects/writing_tests/test/Basic.t.sol index 837da365a..c3d4a2664 100644 --- a/projects/writing_tests/test/Basic.t.sol +++ b/projects/writing_tests/test/Basic.t.sol @@ -21,8 +21,10 @@ contract ContractBTest is Test { } // ANCHOR_END: testNumberIs42 - // ANCHOR: testFailSubtract43 - function testFail_Subtract43() public { + // ANCHOR: testRevert_Subtract43 + /// forge-config: default.allow_internal_expect_revert = true + function testRevert_Subtract43() public { + vm.expectRevert(); testNumber -= 43; } // ANCHOR_END: testFailSubtract43 diff --git a/src/forge/cheatcodes.md b/src/forge/cheatcodes.md index bf2b5d136..39cafbc79 100644 --- a/src/forge/cheatcodes.md +++ b/src/forge/cheatcodes.md @@ -26,34 +26,7 @@ $ forge test {{#include ../output/cheatcodes/forge-test-simple:output}} ``` -Let's make sure that someone who is definitely not the owner can't increment the count: - -```solidity -{{#include ../../projects/cheatcodes/test/OwnerUpOnly.t.sol:contract_prelude}} - - // ... - -{{#include ../../projects/cheatcodes/test/OwnerUpOnly.t.sol:test_fail}} -} -``` - -If we run `forge test` now, we will see that all the test pass. - -```ignore -$ forge test -{{#include ../output/cheatcodes/forge-test-cheatcodes:output}} -``` - -The test passed because the `prank` cheatcode changed our identity to the zero address for the next call (`upOnly.increment()`). The test case passed since we used the `testFail` prefix, however, using `testFail` is considered an anti-pattern since it does not tell us anything about *why* `upOnly.increment()` reverted. - -If we run the tests again with traces turned on, we can see that we reverted with the correct error message. - -```ignore -$ forge test -vvvv --match-test testFail_IncrementAsNotOwner -{{#include ../output/cheatcodes/forge-test-cheatcodes-tracing:output}} -``` - -To be sure in the future, let's make sure that we reverted because we are not the owner using the `expectRevert` cheatcode: +Let's make sure that someone who is definitely not the owner can't increment the count, by using the `expectRevert` cheatcode: ```solidity {{#include ../../projects/cheatcodes/test/OwnerUpOnly.t.sol:contract_prelude}} diff --git a/src/forge/gas-function-snapshots.md b/src/forge/gas-function-snapshots.md index 6d2faded6..04c5e5c3c 100644 --- a/src/forge/gas-function-snapshots.md +++ b/src/forge/gas-function-snapshots.md @@ -15,9 +15,9 @@ $ cat .gas-snapshot ERC20Test:testApprove() (gas: 31162) ERC20Test:testBurn() (gas: 59875) -ERC20Test:testFailTransferFromInsufficientAllowance() (gas: 81034) -ERC20Test:testFailTransferFromInsufficientBalance() (gas: 81662) -ERC20Test:testFailTransferInsufficientBalance() (gas: 52882) +ERC20Test:testRevertTransferFromInsufficientAllowance() (gas: 81034) +ERC20Test:testRevertTransferFromInsufficientBalance() (gas: 81662) +ERC20Test:testRevertTransferInsufficientBalance() (gas: 52882) ERC20Test:testInfiniteApproveTransferFrom() (gas: 90167) ERC20Test:testMetadata() (gas: 14606) ERC20Test:testMint() (gas: 53830) @@ -60,9 +60,9 @@ $ forge snapshot --diff .gas-snapshot2 Running 10 tests for src/test/ERC20.t.sol:ERC20Test [PASS] testApprove() (gas: 31162) [PASS] testBurn() (gas: 59875) -[PASS] testFailTransferFromInsufficientAllowance() (gas: 81034) -[PASS] testFailTransferFromInsufficientBalance() (gas: 81662) -[PASS] testFailTransferInsufficientBalance() (gas: 52882) +[PASS] testRevertTransferFromInsufficientAllowance() (gas: 81034) +[PASS] testRevertTransferFromInsufficientBalance() (gas: 81662) +[PASS] testRevertTransferInsufficientBalance() (gas: 52882) [PASS] testInfiniteApproveTransferFrom() (gas: 90167) [PASS] testMetadata() (gas: 14606) [PASS] testMint() (gas: 53830) @@ -70,9 +70,9 @@ Running 10 tests for src/test/ERC20.t.sol:ERC20Test [PASS] testTransferFrom() (gas: 84152) Test result: ok. 10 passed; 0 failed; finished in 2.86ms testBurn() (gas: 0 (0.000%)) -testFailTransferFromInsufficientAllowance() (gas: 0 (0.000%)) -testFailTransferFromInsufficientBalance() (gas: 0 (0.000%)) -testFailTransferInsufficientBalance() (gas: 0 (0.000%)) +testRevertTransferFromInsufficientAllowance() (gas: 0 (0.000%)) +testRevertTransferFromInsufficientBalance() (gas: 0 (0.000%)) +testRevertTransferInsufficientBalance() (gas: 0 (0.000%)) testInfiniteApproveTransferFrom() (gas: 0 (0.000%)) testMetadata() (gas: 0 (0.000%)) testMint() (gas: 0 (0.000%)) @@ -93,9 +93,9 @@ $ forge snapshot --check .gas-snapshot2 Running 10 tests for src/test/ERC20.t.sol:ERC20Test [PASS] testApprove() (gas: 31162) [PASS] testBurn() (gas: 59875) -[PASS] testFailTransferFromInsufficientAllowance() (gas: 81034) -[PASS] testFailTransferFromInsufficientBalance() (gas: 81662) -[PASS] testFailTransferInsufficientBalance() (gas: 52882) +[PASS] testRevertTransferFromInsufficientAllowance() (gas: 81034) +[PASS] testRevertTransferFromInsufficientBalance() (gas: 81662) +[PASS] testRevertTransferInsufficientBalance() (gas: 52882) [PASS] testInfiniteApproveTransferFrom() (gas: 90167) [PASS] testMetadata() (gas: 14606) [PASS] testMint() (gas: 53830) diff --git a/src/forge/writing-tests.md b/src/forge/writing-tests.md index ba772510d..3d28b6534 100644 --- a/src/forge/writing-tests.md +++ b/src/forge/writing-tests.md @@ -30,10 +30,6 @@ Forge uses the following keywords in tests: ```solidity {{#include ../../projects/writing_tests/test/Basic.t.sol:testNumberIs42}} ``` -- `testFail`: The inverse of the `test` prefix - if the function does not revert, the test fails. -```solidity -{{#include ../../projects/writing_tests/test/Basic.t.sol:testFailSubtract43}} -``` A good practice is to use the pattern `test_Revert[If|When]_Condition` in combination with the [`expectRevert`](../cheatcodes/expect-revert.md) cheatcode (cheatcodes are explained in greater detail in the following [section](./cheatcodes.md)). Also, other testing practices can be found in the [Tutorials section](../tutorials/best-practices.md). @@ -42,7 +38,7 @@ A good practice is to use the pattern `test_Revert[If|When]_Condition` in combin > import {stdError} from "forge-std/StdError.sol"; > ``` -Now, instead of using `testFail`, you know exactly what reverted and with which error: +In this way you know exactly what reverted and with which error: ```solidity {{#include ../../projects/writing_tests/test/Basic2.t.sol:testCannotSubtract43}} diff --git a/src/output/cheatcodes/forge-test-cheatcodes-expectrevert b/src/output/cheatcodes/forge-test-cheatcodes-expectrevert index 1fadfd73f..3ae807cc5 100644 --- a/src/output/cheatcodes/forge-test-cheatcodes-expectrevert +++ b/src/output/cheatcodes/forge-test-cheatcodes-expectrevert @@ -1,14 +1,15 @@ // ANCHOR: all // ANCHOR: command -$ forge test --match-test test_IncrementAsOwner|test_IncrementAsNotOwner --match-path test/OwnerUpOnly.t.sol +$ forge test --match-path test/OwnerUpOnly.t.sol // ANCHOR_END: command // ANCHOR: output No files changed, compilation skipped -Ran 1 test for test/OwnerUpOnly.t.sol:OwnerUpOnlyTest +Ran 2 tests for test/OwnerUpOnly.t.sol:OwnerUpOnlyTest [PASS] test_IncrementAsOwner() (gas: 29161) -Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 389.64µs (53.79µs CPU time) +[PASS] test_RevertWhen_CallerIsNotOwner() (gas: 8656) +Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 389.64µs (53.79µs CPU time) -Ran 1 test suite in 5.83ms (389.64µs CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests) +Ran 2 test suite in 5.83ms (389.64µs CPU time): 2 tests passed, 0 failed, 0 skipped (2 total tests) // ANCHOR_END: output // ANCHOR_END: all diff --git a/src/output/nft_tutorial/forge-test b/src/output/nft_tutorial/forge-test index 1ad260956..2dd1eef66 100644 --- a/src/output/nft_tutorial/forge-test +++ b/src/output/nft_tutorial/forge-test @@ -7,10 +7,10 @@ No files changed, compilation skipped Ran 8 tests for test/SolmateNft.sol:SolmateNftTests [PASS] testBalanceIncremented() (gas: 217400) -[PASS] testFailMaxSupplyReached() (gas: 134524) -[PASS] testFailMintToZeroAddress() (gas: 34521) -[PASS] testFailNoMintPricePaid() (gas: 5568) -[PASS] testFailUnSafeContractReceiver() (gas: 3524) +[PASS] testRevertMaxSupplyReached() (gas: 134524) +[PASS] testRevertMintToZeroAddress() (gas: 34521) +[PASS] testRevertNoMintPricePaid() (gas: 5568) +[PASS] testRevertUnSafeContractReceiver() (gas: 3524) [PASS] testMintPricePaid() (gas: 81321) [PASS] testNewMintOwnerRegistered() (gas: 190741) [PASS] testSafeContractReceiver() (gas: 272655) @@ -18,10 +18,10 @@ Suite result: ok. 8 passed; 0 failed; 0 skipped; finished in 1.51ms (1.34ms CPU Ran 8 tests for test/OpenZeppelinNft.t.sol:OpenZeppelinNftTests [PASS] testBalanceIncremented() (gas: 217829) -[PASS] testFailMaxSupplyReached() (gas: 134524) -[PASS] testFailMintToZeroAddress() (gas: 34577) -[PASS] testFailNoMintPricePaid() (gas: 5568) -[PASS] testFailUnSafeContractReceiver() (gas: 3524) +[PASS] testRevertMaxSupplyReached() (gas: 134524) +[PASS] testRevertMintToZeroAddress() (gas: 34577) +[PASS] testRevertNoMintPricePaid() (gas: 5568) +[PASS] testRevertUnSafeContractReceiver() (gas: 3524) [PASS] testMintPricePaid() (gas: 81554) [PASS] testNewMintOwnerRegistered() (gas: 190956) [PASS] testSafeContractReceiver() (gas: 273151) diff --git a/src/reference/ds-test.md b/src/reference/ds-test.md index 14c4a91bf..1283735b5 100644 --- a/src/reference/ds-test.md +++ b/src/reference/ds-test.md @@ -503,7 +503,7 @@ Asserts `a` is approximately equal to `b` with delta in absolute value. ##### Example ```solidity -function testFail () external { +function testRevert () external { uint256 a = 100; uint256 b = 200; @@ -528,7 +528,7 @@ Asserts `a` is approximately equal to `b` with delta in percentage, where `1e18` ##### Example ```solidity -function testFail () external { +function testRevert () external { uint256 a = 100; uint256 b = 200; assertApproxEqRel(a, b, 0.4e18); diff --git a/src/reference/forge-std/assertApproxEqAbs.md b/src/reference/forge-std/assertApproxEqAbs.md index 14c28d971..e9dd5980a 100644 --- a/src/reference/forge-std/assertApproxEqAbs.md +++ b/src/reference/forge-std/assertApproxEqAbs.md @@ -27,7 +27,7 @@ Optionally includes an error message in the revert string. ### Examples ```solidity -function testFail() external { +function testRevert() external { uint256 a = 100; uint256 b = 200; @@ -36,16 +36,10 @@ function testFail() external { ``` ```ignore -[PASS] testFail() (gas: 23169) -Logs: - Error: a ~= b not satisfied [uint] - Expected: 200 - Actual: 100 - Max Delta: 90 - Delta: 100 +[FAIL: assertion failed: 100 !~= 200 (max delta: 90, real delta: 100)] testRevert() (gas: 23884) ``` ### SEE ALSO - [`assertApproxEqAbsDecimal`](./assertApproxEqAbsDecimal.md) -- [`assertApproxEqRel`](./assertApproxEqRel.md) \ No newline at end of file +- [`assertApproxEqRel`](./assertApproxEqRel.md) diff --git a/src/reference/forge-std/assertApproxEqRel.md b/src/reference/forge-std/assertApproxEqRel.md index c74e58c51..e09be22eb 100644 --- a/src/reference/forge-std/assertApproxEqRel.md +++ b/src/reference/forge-std/assertApproxEqRel.md @@ -27,7 +27,7 @@ Optionally includes an error message in the revert string. ### Examples ```solidity -function testFail () external { +function testRevert() external { uint256 a = 100; uint256 b = 200; assertApproxEqRel(a, b, 0.4e18); @@ -35,16 +35,10 @@ function testFail () external { ``` ```ignore -[PASS] testFail() (gas: 23884) -Logs: - Error: a ~= b not satisfied [uint] - Expected: 200 - Actual: 100 - Max % Delta: 0.400000000000000000 - % Delta: 0.500000000000000000 +[FAIL: assertion failed: 100 !~= 200 (max delta: 40.0000000000000000%, real delta: 50.0000000000000000%)] testRevert() (gas: 23884) ``` ### SEE ALSO - [`assertApproxEqRelDecimal`](./assertApproxEqRelDecimal.md) -- [`assertApproxEqAbs`](./assertApproxEqAbs.md) \ No newline at end of file +- [`assertApproxEqAbs`](./assertApproxEqAbs.md) diff --git a/src/reference/forge/forge-test.md b/src/reference/forge/forge-test.md index 5d2ce79d6..81078e251 100644 --- a/src/reference/forge/forge-test.md +++ b/src/reference/forge/forge-test.md @@ -88,17 +88,17 @@ You can pass `--json` to make it easier for outside extensions to parse structur forge test --gas-report ``` -4. Only run tests in `test/Contract.t.sol` in the `BigTest` contract that start with `testFail`: +4. Only run tests in `test/Contract.t.sol` in the `BigTest` contract that start with `testRevert`: ```sh forge test --match-path test/Contract.t.sol --match-contract BigTest \ - --match-test "testFail*" + --match-test "testRevert*" ``` 5. List tests in desired format ```sh forge test --list forge test --list --json - forge test --list --json --match-test "testFail*" | tail -n 1 | json_pp + forge test --list --json --match-test "testRevert*" | tail -n 1 | json_pp ``` ### SEE ALSO diff --git a/src/tutorials/solmate-nft.md b/src/tutorials/solmate-nft.md index 43e3c9fb2..15b21fb41 100644 --- a/src/tutorials/solmate-nft.md +++ b/src/tutorials/solmate-nft.md @@ -335,7 +335,7 @@ The test suite is set up as a contract with a `setUp` method which runs before e As you can see, Forge offers a number of [cheatcodes](../cheatcodes/) to manipulate state to accommodate your testing scenario. -For example, our `testFailMaxSupplyReached` test checks that an attempt to mint fails when the max supply of NFT is reached. Thus, the `currentTokenId` of the NFT contract needs to be set to the max supply by using the store cheatcode which allows you to write data to your contracts storage slots. The storage slots you wish to write to can easily be found using the +For example, our `test_RevertMintMaxSupplyReached` test checks that an attempt to mint fails when the max supply of NFT is reached. Thus, the `currentTokenId` of the NFT contract needs to be set to the max supply by using the store cheatcode which allows you to write data to your contracts storage slots. The storage slots you wish to write to can easily be found using the [`forge-std`](https://github.com/foundry-rs/forge-std/) helper library. You can run the test with the following command: ```bash diff --git a/src/tutorials/testing-eip712.md b/src/tutorials/testing-eip712.md index d9393e498..da050f7f2 100644 --- a/src/tutorials/testing-eip712.md +++ b/src/tutorials/testing-eip712.md @@ -407,7 +407,7 @@ contract ERC20Test is Test { - Ensure failure for calls with an invalid allowance and invalid balance ```solidity - function testFail_InvalidAllowance() public { + function testRevert_InvalidAllowance() public { SigUtils.Permit memory permit = SigUtils.Permit({ owner: owner, spender: spender, @@ -431,10 +431,11 @@ contract ERC20Test is Test { ); vm.prank(spender); + vm.expectRevert(); token.transferFrom(owner, spender, 1e18); // attempt to transfer 1 token } - function testFail_InvalidBalance() public { + function testRevert_InvalidBalance() public { SigUtils.Permit memory permit = SigUtils.Permit({ owner: owner, spender: spender, @@ -458,6 +459,7 @@ contract ERC20Test is Test { ); vm.prank(spender); + vm.expectRevert(); token.transferFrom(owner, spender, 2e18); // attempt to transfer 2 tokens (owner only owns 1) } ```