From 688d284c4f53b2b9188f9065584e54626215232d Mon Sep 17 00:00:00 2001 From: Adam Gall Date: Sun, 26 Oct 2025 23:38:33 -0400 Subject: [PATCH] test: remove gas tests and standardize fuzz test naming Remove all explicit gas tests from the codebase as they are an anti-pattern that creates maintenance burden without providing real value. Gas costs vary between compiler versions, optimization settings, and coverage runs, making exact assertions fragile and requiring constant updates. Changes: - Remove 14 gas tests across Owner and OwnerTwoSteps test files - Standardize fuzz test naming from `test_Fuzz_*` to `testFuzz_*` convention - Clean up test files by removing gas measurement code (gasleft, gasUsed, etc.) Gas tests removed from: - test/access/Owner/LibOwner.t.sol (3 tests) - test/access/Owner/OwnerFacet.t.sol (2 tests) - test/access/OwnerTwoSteps/LibOwnerTwoSteps.t.sol (4 tests) - test/access/OwnerTwoSteps/OwnerTwoSteps.t.sol (5 tests) Fuzz test naming fixed in: - test/token/ERC20/ERC20/ERC20Facet.t.sol - test/token/ERC20/ERC20/LibERC20.t.sol --- test/access/Owner/LibOwner.t.sol | 41 ++--------- test/access/Owner/OwnerFacet.t.sol | 32 ++------- .../OwnerTwoSteps/LibOwnerTwoSteps.t.sol | 55 ++------------- test/access/OwnerTwoSteps/OwnerTwoSteps.t.sol | 69 ++----------------- test/token/ERC20/ERC20/ERC20Facet.t.sol | 12 ++-- test/token/ERC20/ERC20/LibERC20.t.sol | 10 +-- 6 files changed, 31 insertions(+), 188 deletions(-) diff --git a/test/access/Owner/LibOwner.t.sol b/test/access/Owner/LibOwner.t.sol index 68e2ca14..92b9d0f2 100644 --- a/test/access/Owner/LibOwner.t.sol +++ b/test/access/Owner/LibOwner.t.sol @@ -183,13 +183,13 @@ contract LibOwnerTest is Test { // Fuzz Tests // ============================================ - function test_Fuzz_TransferOwnership(address newOwner) public { + function testFuzz_TransferOwnership(address newOwner) public { vm.prank(INITIAL_OWNER); harness.transferOwnership(newOwner); assertEq(harness.owner(), newOwner); } - function test_Fuzz_MultipleTransfers(address owner1, address owner2, address owner3) public { + function testFuzz_MultipleTransfers(address owner1, address owner2, address owner3) public { vm.assume(owner1 != address(0)); vm.assume(owner2 != address(0)); @@ -206,7 +206,7 @@ contract LibOwnerTest is Test { assertEq(harness.owner(), owner3); } - function test_Fuzz_RevertWhen_RenouncedOwnerTransfers(address target) public { + function testFuzz_RevertWhen_RenouncedOwnerTransfers(address target) public { vm.assume(target != address(0)); // Renounce @@ -252,7 +252,7 @@ contract LibOwnerTest is Test { harness.requireOwner(); } - function test_Fuzz_RequireOwner(address caller) public { + function testFuzz_RequireOwner(address caller) public { if (caller == INITIAL_OWNER) { // Should not revert for owner vm.prank(caller); @@ -264,37 +264,4 @@ contract LibOwnerTest is Test { harness.requireOwner(); } } - - // ============================================ - // Gas Tests - // ============================================ - - function test_Gas_Owner() public view { - uint256 gasBefore = gasleft(); - harness.owner(); - uint256 gasUsed = gasBefore - gasleft(); - - console2.log("Gas used for LibOwner.owner():", gasUsed); - assertTrue(gasUsed < 10000, "Owner getter uses too much gas"); - } - - function test_Gas_TransferOwnership() public { - uint256 gasBefore = gasleft(); - vm.prank(INITIAL_OWNER); - harness.transferOwnership(NEW_OWNER); - uint256 gasUsed = gasBefore - gasleft(); - - console2.log("Gas used for LibOwner.transferOwnership():", gasUsed); - assertTrue(gasUsed < 50000, "Transfer ownership uses too much gas"); - } - - function test_Gas_TransferOwnership_Renounce() public { - uint256 gasBefore = gasleft(); - vm.prank(INITIAL_OWNER); - harness.transferOwnership(ZERO_ADDRESS); - uint256 gasUsed = gasBefore - gasleft(); - - console2.log("Gas used for LibOwner renounce:", gasUsed); - assertTrue(gasUsed < 50000, "Renounce uses too much gas"); - } } diff --git a/test/access/Owner/OwnerFacet.t.sol b/test/access/Owner/OwnerFacet.t.sol index 43169555..e3170a1c 100644 --- a/test/access/Owner/OwnerFacet.t.sol +++ b/test/access/Owner/OwnerFacet.t.sol @@ -165,14 +165,14 @@ contract OwnerFacetTest is Test { // Fuzz Tests // ============================================ - function test_Fuzz_TransferOwnership(address newOwner) public { + function testFuzz_TransferOwnership(address newOwner) public { vm.prank(INITIAL_OWNER); ownerFacet.transferOwnership(newOwner); assertEq(ownerFacet.owner(), newOwner); } - function test_Fuzz_SequentialTransfers(address owner1, address owner2, address owner3) public { + function testFuzz_SequentialTransfers(address owner1, address owner2, address owner3) public { vm.assume(owner1 != address(0)); vm.assume(owner2 != address(0)); vm.assume(owner3 != address(0)); @@ -190,7 +190,7 @@ contract OwnerFacetTest is Test { assertEq(ownerFacet.owner(), owner3); } - function test_Fuzz_RevertWhen_UnauthorizedCaller(address caller, address target) public { + function testFuzz_RevertWhen_UnauthorizedCaller(address caller, address target) public { vm.assume(caller != INITIAL_OWNER); vm.prank(caller); @@ -198,7 +198,7 @@ contract OwnerFacetTest is Test { ownerFacet.transferOwnership(target); } - function test_Fuzz_RenouncePreventsAllTransfers(address caller, address target) public { + function testFuzz_RenouncePreventsAllTransfers(address caller, address target) public { vm.assume(caller != address(0)); // Renounce ownership @@ -212,28 +212,4 @@ contract OwnerFacetTest is Test { } // ============================================ - // Gas Tests - // ============================================ - - function test_Gas_TransferOwnership() public { - uint256 gasBefore = gasleft(); - vm.prank(INITIAL_OWNER); - ownerFacet.transferOwnership(NEW_OWNER); - uint256 gasUsed = gasBefore - gasleft(); - - // Log gas usage for optimization tracking - console2.log("Gas used for transferOwnership:", gasUsed); - // Should be relatively low since it's just storage updates - assertTrue(gasUsed < 50000, "Transfer ownership uses too much gas"); - } - - function test_Gas_OwnerGetter() public view { - uint256 gasBefore = gasleft(); - ownerFacet.owner(); - uint256 gasUsed = gasBefore - gasleft(); - - console2.log("Gas used for owner():", gasUsed); - // Should be very low since it's just a storage read - assertTrue(gasUsed < 10000, "Owner getter uses too much gas"); - } } diff --git a/test/access/OwnerTwoSteps/LibOwnerTwoSteps.t.sol b/test/access/OwnerTwoSteps/LibOwnerTwoSteps.t.sol index f3e720c6..c18ed1d5 100644 --- a/test/access/OwnerTwoSteps/LibOwnerTwoSteps.t.sol +++ b/test/access/OwnerTwoSteps/LibOwnerTwoSteps.t.sol @@ -364,7 +364,7 @@ contract LibOwnerTwoStepsTest is Test { // Fuzz Tests // ============================================ - function test_Fuzz_TransferOwnership(address newOwner) public { + function testFuzz_TransferOwnership(address newOwner) public { vm.prank(INITIAL_OWNER); harness.transferOwnership(newOwner); @@ -372,7 +372,7 @@ contract LibOwnerTwoStepsTest is Test { assertEq(harness.pendingOwner(), newOwner); } - function test_Fuzz_AcceptOwnership(address newOwner) public { + function testFuzz_AcceptOwnership(address newOwner) public { vm.assume(newOwner != address(0)); vm.prank(INITIAL_OWNER); @@ -385,7 +385,7 @@ contract LibOwnerTwoStepsTest is Test { assertEq(harness.pendingOwner(), ZERO_ADDRESS); } - function test_Fuzz_TransferOwnership_AnyCaller(address caller, address target) public { + function testFuzz_TransferOwnership_AnyCaller(address caller, address target) public { // Library allows any caller vm.prank(caller); harness.transferOwnership(target); @@ -395,7 +395,7 @@ contract LibOwnerTwoStepsTest is Test { assertEq(harness.owner(), INITIAL_OWNER); // Owner unchanged until acceptance } - function test_Fuzz_AcceptOwnership_AnyCaller(address caller) public { + function testFuzz_AcceptOwnership_AnyCaller(address caller) public { vm.prank(INITIAL_OWNER); harness.transferOwnership(NEW_OWNER); @@ -408,7 +408,7 @@ contract LibOwnerTwoStepsTest is Test { assertEq(harness.pendingOwner(), ZERO_ADDRESS); } - function test_Fuzz_SequentialTransfers(address owner1, address owner2, address owner3) public { + function testFuzz_SequentialTransfers(address owner1, address owner2, address owner3) public { vm.assume(owner1 != address(0)); vm.assume(owner2 != address(0)); vm.assume(owner3 != address(0)); @@ -434,49 +434,4 @@ contract LibOwnerTwoStepsTest is Test { harness.acceptOwnership(); assertEq(harness.owner(), owner3); } - - // ============================================ - // Gas Tests - // ============================================ - - function test_Gas_TransferOwnership() public { - uint256 gasBefore = gasleft(); - vm.prank(INITIAL_OWNER); - harness.transferOwnership(NEW_OWNER); - uint256 gasUsed = gasBefore - gasleft(); - - console2.log("Gas used for LibOwnerTwoSteps.transferOwnership():", gasUsed); - assertTrue(gasUsed < 70000, "Transfer ownership uses too much gas"); - } - - function test_Gas_AcceptOwnership() public { - vm.prank(INITIAL_OWNER); - harness.transferOwnership(NEW_OWNER); - - uint256 gasBefore = gasleft(); - vm.prank(NEW_OWNER); - harness.acceptOwnership(); - uint256 gasUsed = gasBefore - gasleft(); - - console2.log("Gas used for LibOwnerTwoSteps.acceptOwnership():", gasUsed); - assertTrue(gasUsed < 35000, "Accept ownership uses too much gas"); - } - - function test_Gas_OwnerGetter() public view { - uint256 gasBefore = gasleft(); - harness.owner(); - uint256 gasUsed = gasBefore - gasleft(); - - console2.log("Gas used for LibOwnerTwoSteps.owner():", gasUsed); - assertTrue(gasUsed < 10000, "Owner getter uses too much gas"); - } - - function test_Gas_PendingOwnerGetter() public view { - uint256 gasBefore = gasleft(); - harness.pendingOwner(); - uint256 gasUsed = gasBefore - gasleft(); - - console2.log("Gas used for LibOwnerTwoSteps.pendingOwner():", gasUsed); - assertTrue(gasUsed < 10000, "Pending owner getter uses too much gas"); - } } diff --git a/test/access/OwnerTwoSteps/OwnerTwoSteps.t.sol b/test/access/OwnerTwoSteps/OwnerTwoSteps.t.sol index 590ebc54..afdebbe2 100644 --- a/test/access/OwnerTwoSteps/OwnerTwoSteps.t.sol +++ b/test/access/OwnerTwoSteps/OwnerTwoSteps.t.sol @@ -254,7 +254,7 @@ contract OwnerTwoStepsFacetTest is Test { // Fuzz Tests // ============================================ - function test_Fuzz_TransferOwnership(address newOwner) public { + function testFuzz_TransferOwnership(address newOwner) public { vm.prank(INITIAL_OWNER); ownerTwoSteps.transferOwnership(newOwner); @@ -262,7 +262,7 @@ contract OwnerTwoStepsFacetTest is Test { assertEq(ownerTwoSteps.pendingOwner(), newOwner); } - function test_Fuzz_AcceptOwnership(address newOwner) public { + function testFuzz_AcceptOwnership(address newOwner) public { vm.assume(newOwner != address(0)); // Zero address can't execute transactions vm.prank(INITIAL_OWNER); @@ -275,7 +275,7 @@ contract OwnerTwoStepsFacetTest is Test { assertEq(ownerTwoSteps.pendingOwner(), ZERO_ADDRESS); } - function test_Fuzz_RevertWhen_UnauthorizedTransfer(address caller, address target) public { + function testFuzz_RevertWhen_UnauthorizedTransfer(address caller, address target) public { vm.assume(caller != INITIAL_OWNER); vm.prank(caller); @@ -283,7 +283,7 @@ contract OwnerTwoStepsFacetTest is Test { ownerTwoSteps.transferOwnership(target); } - function test_Fuzz_RevertWhen_UnauthorizedAccept(address caller) public { + function testFuzz_RevertWhen_UnauthorizedAccept(address caller) public { vm.assume(caller != NEW_OWNER); vm.prank(INITIAL_OWNER); @@ -471,66 +471,11 @@ contract OwnerTwoStepsFacetTest is Test { assertEq(ownerTwoSteps.pendingOwner(), ZERO_ADDRESS); } - // ============================================ - // Gas Benchmarks - // ============================================ - - function test_Gas_Owner() public view { - uint256 gasBefore = gasleft(); - ownerTwoSteps.owner(); - uint256 gasUsed = gasBefore - gasleft(); - - console2.log("Gas used for owner():", gasUsed); - assertTrue(gasUsed < 10000, "Owner getter uses too much gas"); - } - - function test_Gas_PendingOwner() public view { - uint256 gasBefore = gasleft(); - ownerTwoSteps.pendingOwner(); - uint256 gasUsed = gasBefore - gasleft(); - - console2.log("Gas used for pendingOwner():", gasUsed); - assertTrue(gasUsed < 10000, "Pending owner getter uses too much gas"); - } - - function test_Gas_TransferOwnership() public { - uint256 gasBefore = gasleft(); - vm.prank(INITIAL_OWNER); - ownerTwoSteps.transferOwnership(NEW_OWNER); - uint256 gasUsed = gasBefore - gasleft(); - - console2.log("Gas used for transferOwnership():", gasUsed); - assertTrue(gasUsed < 50000, "Transfer ownership uses too much gas"); - } - - function test_Gas_AcceptOwnership() public { - vm.prank(INITIAL_OWNER); - ownerTwoSteps.transferOwnership(NEW_OWNER); - - uint256 gasBefore = gasleft(); - vm.prank(NEW_OWNER); - ownerTwoSteps.acceptOwnership(); - uint256 gasUsed = gasBefore - gasleft(); - - console2.log("Gas used for acceptOwnership():", gasUsed); - assertTrue(gasUsed < 35000, "Accept ownership uses too much gas"); - } - - function test_Gas_RenounceOwnership() public { - uint256 gasBefore = gasleft(); - vm.prank(INITIAL_OWNER); - ownerTwoSteps.renounceOwnership(); - uint256 gasUsed = gasBefore - gasleft(); - - console2.log("Gas used for renounceOwnership():", gasUsed); - assertTrue(gasUsed < 30000, "Renounce ownership uses too much gas"); - } - // ============================================ // Additional Fuzz Tests // ============================================ - function test_Fuzz_RenounceOwnership_OnlyOwner(address caller) public { + function testFuzz_RenounceOwnership_OnlyOwner(address caller) public { if (caller == INITIAL_OWNER) { vm.prank(caller); ownerTwoSteps.renounceOwnership(); @@ -543,7 +488,7 @@ contract OwnerTwoStepsFacetTest is Test { } } - function test_Fuzz_StateAfterRenounce(address caller, address target) public { + function testFuzz_StateAfterRenounce(address caller, address target) public { // Renounce ownership vm.prank(INITIAL_OWNER); ownerTwoSteps.renounceOwnership(); @@ -567,7 +512,7 @@ contract OwnerTwoStepsFacetTest is Test { } } - function test_Fuzz_RenounceWithPendingOwner(address pendingOwner) public { + function testFuzz_RenounceWithPendingOwner(address pendingOwner) public { vm.assume(pendingOwner != address(0)); // Set pending owner diff --git a/test/token/ERC20/ERC20/ERC20Facet.t.sol b/test/token/ERC20/ERC20/ERC20Facet.t.sol index 38bcf41b..88a2308f 100644 --- a/test/token/ERC20/ERC20/ERC20Facet.t.sol +++ b/test/token/ERC20/ERC20/ERC20Facet.t.sol @@ -106,7 +106,7 @@ contract ERC20FacetTest is Test { assertEq(token.balanceOf(bob), INITIAL_SUPPLY); } - function test_Fuzz_Transfer(address to, uint256 amount) public { + function testFuzz_Transfer(address to, uint256 amount) public { vm.assume(to != address(0)); vm.assume(amount <= INITIAL_SUPPLY); @@ -198,7 +198,7 @@ contract ERC20FacetTest is Test { assertEq(token.allowance(alice, bob), 0); } - function test_Fuzz_Approve(address spender, uint256 amount) public { + function testFuzz_Approve(address spender, uint256 amount) public { vm.assume(spender != address(0)); vm.prank(alice); @@ -261,7 +261,7 @@ contract ERC20FacetTest is Test { assertEq(token.allowance(alice, bob), allowanceAmount - transferAmount); } - function test_Fuzz_TransferFrom(uint256 approval, uint256 amount) public { + function testFuzz_TransferFrom(uint256 approval, uint256 amount) public { vm.assume(approval <= INITIAL_SUPPLY); vm.assume(amount <= approval); @@ -397,7 +397,7 @@ contract ERC20FacetTest is Test { assertEq(token.totalSupply(), 0); } - function test_Fuzz_Burn(uint256 amount) public { + function testFuzz_Burn(uint256 amount) public { vm.assume(amount <= INITIAL_SUPPLY); vm.prank(alice); @@ -458,7 +458,7 @@ contract ERC20FacetTest is Test { assertEq(token.totalSupply(), INITIAL_SUPPLY - burnAmount); } - function test_Fuzz_BurnFrom(uint256 approval, uint256 amount) public { + function testFuzz_BurnFrom(uint256 approval, uint256 amount) public { vm.assume(approval <= INITIAL_SUPPLY); vm.assume(amount <= approval); @@ -1001,7 +1001,7 @@ contract ERC20FacetTest is Test { token.permit(owner, bob, value, deadline, v, r, bytes32(0)); } - function test_Fuzz_Permit(uint256 ownerKey, address spender, uint256 value, uint256 deadline) public { + function testFuzz_Permit(uint256 ownerKey, address spender, uint256 value, uint256 deadline) public { vm.assume(ownerKey != 0 && ownerKey < type(uint256).max / 2); vm.assume(spender != address(0)); vm.assume(deadline > block.timestamp); diff --git a/test/token/ERC20/ERC20/LibERC20.t.sol b/test/token/ERC20/ERC20/LibERC20.t.sol index 9bc8840e..de6d2860 100644 --- a/test/token/ERC20/ERC20/LibERC20.t.sol +++ b/test/token/ERC20/ERC20/LibERC20.t.sol @@ -73,7 +73,7 @@ contract LibERC20Test is Test { assertEq(harness.totalSupply(), 350e18); } - function test_Fuzz_Mint(address to, uint256 amount) public { + function testFuzz_Mint(address to, uint256 amount) public { vm.assume(to != address(0)); vm.assume(amount < type(uint256).max / 2); @@ -117,7 +117,7 @@ contract LibERC20Test is Test { assertEq(harness.totalSupply(), 0); } - function test_Fuzz_Burn(address account, uint256 mintAmount, uint256 burnAmount) public { + function testFuzz_Burn(address account, uint256 mintAmount, uint256 burnAmount) public { vm.assume(account != address(0)); vm.assume(mintAmount < type(uint256).max / 2); vm.assume(burnAmount <= mintAmount); @@ -185,7 +185,7 @@ contract LibERC20Test is Test { assertEq(harness.balanceOf(bob), 0); } - function test_Fuzz_Transfer(uint256 balance, uint256 amount) public { + function testFuzz_Transfer(uint256 balance, uint256 amount) public { vm.assume(balance < type(uint256).max / 2); vm.assume(amount <= balance); @@ -277,7 +277,7 @@ contract LibERC20Test is Test { assertEq(harness.allowance(alice, bob), 0); } - function test_Fuzz_Approve(address spender, uint256 amount) public { + function testFuzz_Approve(address spender, uint256 amount) public { vm.assume(spender != address(0)); vm.prank(alice); @@ -331,7 +331,7 @@ contract LibERC20Test is Test { assertEq(harness.allowance(alice, bob), allowanceAmount - transferAmount); } - function test_Fuzz_TransferFrom(uint256 balance, uint256 approval, uint256 amount) public { + function testFuzz_TransferFrom(uint256 balance, uint256 approval, uint256 amount) public { vm.assume(balance < type(uint256).max / 2); vm.assume(approval <= balance); vm.assume(amount <= approval);