diff --git a/.changeset/wild-months-carry.md b/.changeset/wild-months-carry.md new file mode 100644 index 0000000000000..f8d0e6074d524 --- /dev/null +++ b/.changeset/wild-months-carry.md @@ -0,0 +1,5 @@ +--- +'@eth-optimism/contracts-bedrock': patch +--- + +Document test function naming convention and create a script for checking. diff --git a/packages/contracts-bedrock/.gas-snapshot b/packages/contracts-bedrock/.gas-snapshot index 9c921054174a2..e06454d3eb89a 100644 --- a/packages/contracts-bedrock/.gas-snapshot +++ b/packages/contracts-bedrock/.gas-snapshot @@ -8,7 +8,6 @@ GasBenchMark_L1StandardBridge_Finalize:test_finalizeETHWithdrawal_benchmark() (g GasBenchMark_L2OutputOracle:test_proposeL2Output_benchmark() (gas: 88513) GasBenchMark_OptimismPortal:test_depositTransaction_benchmark() (gas: 74953) GasBenchMark_OptimismPortal:test_depositTransaction_benchmark_1() (gas: 36044) -CrossDomainMessenger_Test:testFuzz_baseGas(uint32) (runs: 256, μ: 20196, ~: 20196) CrossDomainMessenger_Test:test_baseGas() (gas: 20054) CrossDomainOwnableThroughPortal_Test:test_depositTransaction_crossDomainOwner() (gas: 61806) CrossDomainOwnable_Test:test_onlyOwner() (gas: 34861) @@ -19,7 +18,7 @@ CrossDomainOwnable2_Test:test_revertNotSetOnlyOwner2() (gas: 14565) CrossDomainOwnable2_Test:test_revertOnlyOwner() (gas: 61712) DeployerWhitelist_Test:test_owner() (gas: 7516) DeployerWhitelist_Test:test_storageSlots() (gas: 33395) -Encoding_Test:test_nonceVersioning(uint240,uint16) (runs: 256, μ: 658, ~: 658) +Encoding_Test:test_nonceVersioning_succeeds(uint240,uint16) (runs: 256, μ: 658, ~: 658) FeeVault_Test:test_constructor() (gas: 10623) FeeVault_Test:test_minWithdrawalAmount() (gas: 10689) GasPriceOracle_Test:test_baseFee() (gas: 8281) @@ -29,7 +28,7 @@ GasPriceOracle_Test:test_overhead() (gas: 10568) GasPriceOracle_Test:test_scalar() (gas: 10610) GasPriceOracle_Test:test_setGasPriceReverts() (gas: 5888) GasPriceOracle_Test:test_setL1BaseFeeReverts() (gas: 5909) -Hashing_Test:test_hashDepositSource() (gas: 628) +Hashing_Test:test_hashDepositSource_succeeds() (gas: 650) L1BlockTest:test_basefee() (gas: 7531) L1BlockTest:test_hash() (gas: 7553) L1BlockTest:test_number() (gas: 7608) @@ -122,9 +121,9 @@ L2StandardBridge_Test:test_receive() (gas: 131820) L2StandardBridge_Test:test_withdraw() (gas: 343951) L2StandardBridge_Test:test_withdrawTo() (gas: 344680) L2StandardBridge_Test:test_withdraw_onlyEOA() (gas: 251816) -L2ToL1MessagePasserTest:test_burn() (gas: 112530) -L2ToL1MessagePasserTest:test_initiateWithdrawal_fromContract() (gas: 70246) -L2ToL1MessagePasserTest:test_initiateWithdrawal_fromEOA() (gas: 75764) +L2ToL1MessagePasserTest:test_burn_succeeds() (gas: 112508) +L2ToL1MessagePasserTest:test_initiateWithdrawal_fromContract_succeeds() (gas: 70313) +L2ToL1MessagePasserTest:test_initiateWithdrawal_fromEOA_succeeds() (gas: 75764) LegacyERC20ETH_Test:test_approve() (gas: 10723) LegacyERC20ETH_Test:test_burn() (gas: 10615) LegacyERC20ETH_Test:test_crossDomain() (gas: 6339) @@ -149,40 +148,40 @@ OptimismMintableTokenFactory_Test:test_bridge() (gas: 7577) OptimismMintableTokenFactory_Test:test_createStandardL2Token() (gas: 1095274) OptimismMintableTokenFactory_Test:test_createStandardL2TokenSameTwice() (gas: 2176691) OptimismMintableTokenFactory_Test:test_createStandardL2TokenShouldRevertIfRemoteIsZero() (gas: 9413) -OptimismPortalUpgradeable_Test:test_initialize_cannotInitImpl_reverts() (gas: 10813) -OptimismPortalUpgradeable_Test:test_initialize_cannotInitProxy_reverts() (gas: 15789) -OptimismPortalUpgradeable_Test:test_params_initValuesOnProxy_success() (gas: 16033) -OptimismPortalUpgradeable_Test:test_upgradeToAndCall_upgrading_success() (gas: 180435) -OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputRootChanges_reverts() (gas: 199750) -OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputTimestampIsNotFinalized_reverts() (gas: 202003) +OptimismPortalUpgradeable_Test:test_initialize_cannotInitImpl_reverts() (gas: 10791) +OptimismPortalUpgradeable_Test:test_initialize_cannotInitProxy_reverts() (gas: 15833) +OptimismPortalUpgradeable_Test:test_params_initValuesOnProxy_succeeds() (gas: 16011) +OptimismPortalUpgradeable_Test:test_upgradeToAndCall_upgrading_succeeds() (gas: 180457) +OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputRootChanges_reverts() (gas: 199728) +OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputTimestampIsNotFinalized_reverts() (gas: 202025) OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalNotProven_reverts() (gas: 39656) OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalProofNotOldEnough_reverts() (gas: 197092) OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onInsufficientGas_reverts() (gas: 197814) -OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onRecentWithdrawal_reverts() (gas: 177829) -OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReentrancy_reverts() (gas: 236177) -OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 237785) -OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_provenWithdrawalHash_success() (gas: 229511) -OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails_fails() (gas: 332126) -OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_timestampLessThanL2OracleStart_reverts() (gas: 193772) -OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidOutputRootProof_reverts() (gas: 83498) +OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onRecentWithdrawal_reverts() (gas: 177806) +OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReentrancy_reverts() (gas: 236200) +OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 237764) +OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_provenWithdrawalHash_succeeds() (gas: 229467) +OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails_fails() (gas: 332148) +OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_timestampLessThanL2OracleStart_reverts() (gas: 193816) +OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidOutputRootProof_reverts() (gas: 83542) OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onSelfCall_reverts() (gas: 50732) OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_oninvalidWithdrawalProof_reverts() (gas: 136758) -OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_replayProveChangedOutputRoot_success() (gas: 277056) -OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_replayProve_reverts() (gas: 189117) -OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_validWithdrawalProof_success() (gas: 179324) -OptimismPortal_Test:test_OptimismPortalConstructor() (gas: 17276) -OptimismPortal_Test:test_OptimismPortalReceiveEth_success() (gas: 127480) -OptimismPortal_Test:test_depositTransaction_NoValueContract_success() (gas: 76681) -OptimismPortal_Test:test_depositTransaction_NoValueEOA_success() (gas: 77153) -OptimismPortal_Test:test_depositTransaction_contractCreation_reverts() (gas: 14245) -OptimismPortal_Test:test_depositTransaction_createWithZeroValueForContract_success() (gas: 76707) -OptimismPortal_Test:test_depositTransaction_createWithZeroValueForEOA_success() (gas: 77029) -OptimismPortal_Test:test_depositTransaction_withEthValueAndContractContractCreation_success() (gas: 83709) -OptimismPortal_Test:test_depositTransaction_withEthValueAndEOAContractCreation_success() (gas: 75872) -OptimismPortal_Test:test_depositTransaction_withEthValueFromContract_success() (gas: 83411) -OptimismPortal_Test:test_depositTransaction_withEthValueFromEOA_success() (gas: 84111) -OptimismPortal_Test:test_isOutputFinalized_success() (gas: 119473) -OptimismPortal_Test:test_simple_isOutputFinalized_success() (gas: 24209) +OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_replayProveChangedOutputRoot_succeeds() (gas: 277079) +OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_replayProve_reverts() (gas: 189072) +OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_validWithdrawalProof_succeeds() (gas: 179324) +OptimismPortal_Test:test_constructor_succeeds() (gas: 17277) +OptimismPortal_Test:test_depositTransaction_contractCreation_reverts() (gas: 14267) +OptimismPortal_Test:test_depositTransaction_createWithZeroValueForContract_succeeds() (gas: 76662) +OptimismPortal_Test:test_depositTransaction_createWithZeroValueForEOA_succeeds() (gas: 77029) +OptimismPortal_Test:test_depositTransaction_noValueContract_succeeds() (gas: 76681) +OptimismPortal_Test:test_depositTransaction_noValueEOA_succeeds() (gas: 77198) +OptimismPortal_Test:test_depositTransaction_withEthValueAndContractContractCreation_succeeds() (gas: 83664) +OptimismPortal_Test:test_depositTransaction_withEthValueAndEOAContractCreation_succeeds() (gas: 75862) +OptimismPortal_Test:test_depositTransaction_withEthValueFromContract_succeeds() (gas: 83389) +OptimismPortal_Test:test_depositTransaction_withEthValueFromEOA_succeeds() (gas: 84133) +OptimismPortal_Test:test_isOutputFinalized_succeeds() (gas: 119474) +OptimismPortal_Test:test_receive_succeeds() (gas: 127522) +OptimismPortal_Test:test_simple_isOutputFinalized_succeeds() (gas: 24165) Proxy_Test:test_clashingFunctionSignatures() (gas: 101347) Proxy_Test:test_implementationKey() (gas: 20887) Proxy_Test:test_implementationProxyCallIfNotAdmin() (gas: 29963) diff --git a/packages/contracts-bedrock/README.md b/packages/contracts-bedrock/README.md index ec9b9bc6ce2f2..adb00e2a22f6b 100644 --- a/packages/contracts-bedrock/README.md +++ b/packages/contracts-bedrock/README.md @@ -194,3 +194,48 @@ After the initial Bedrock upgrade, contracts MUST use the following versioning s #### Exceptions We have made an exception to the `Semver` rule for the `WETH` contract to avoid making changes to a well-known, simple, and recognizable contract. + +### Tests + +Tests are written using Foundry. + +All test contracts and functions should be organized and named according to the following guidelines. + +These guidelines are also encoded in a script which can be run with: + +``` +ts-node scripts/forge-test-names.ts +``` + +*Note: This is a work in progress, not all test files are compliant with these guidelines.* + +#### Organizing Principles + +- Solidity `contract`s are used to organize the test suite similar to how mocha uses describe. +- Every non-trivial state changing function should have a separate contract for happy and sad path + tests. This helps to make it very obvious where there are not yet sad path tests. +- Simpler functions like getters and setters are grouped together into test contracts. + +#### Test function naming convention + +Test function names are split by underscores, into 3 or 4 parts. An example function name is `test_onlyOwner_callerIsNotOwner_reverts()`. + +The parts are: `[method]_[FunctionName]_[reason]_[success]`, where: + +- `[method]` is either `test`, `testFuzz`, or `testDiff` +- `[FunctionName]` is the name of the function or higher level behavior being tested. +- `[reason]` is an optional description for the behavior being tested. +- `[status]` must be one of: + - `succeeds`: used for most happy path cases + - `reverts`: used for most sad path cases + - `works`: used for tests which include a mix of happy and sad assertions (these should be broken up if possible) + - `fails`: used for tests which 'fail' in some way other than reverting + - `benchmark`: used for tests intended to establish gas costs + +#### Contract Naming Conventions + +Test contracts should be named one of the following according to their use: + +- `TargetContract_Init` for contracts that perform basic setup to be reused in other test contracts. +- `TargetContract_Function_Test` for contracts containing happy path tests for a given function. +- `TargetContract_Function_TestFail` for contracts containing sad path tests for a given function. diff --git a/packages/contracts-bedrock/contracts/test/AddressAliasHelper.t.sol b/packages/contracts-bedrock/contracts/test/AddressAliasHelper.t.sol index d15a192efa6e1..406736fc3915e 100644 --- a/packages/contracts-bedrock/contracts/test/AddressAliasHelper.t.sol +++ b/packages/contracts-bedrock/contracts/test/AddressAliasHelper.t.sol @@ -5,7 +5,7 @@ import { Test } from "forge-std/Test.sol"; import { AddressAliasHelper } from "../vendor/AddressAliasHelper.sol"; contract AddressAliasHelper_Test is Test { - function test_fuzz_roundtrip(address _address) external { + function testFuzz_applyAndUndo_succeeds(address _address) external { address aliased = AddressAliasHelper.applyL1ToL2Alias(_address); address unaliased = AddressAliasHelper.undoL1ToL2Alias(aliased); assertEq(_address, unaliased); diff --git a/packages/contracts-bedrock/contracts/test/Encoding.t.sol b/packages/contracts-bedrock/contracts/test/Encoding.t.sol index c20a02d7f58a3..2da7779ed4897 100644 --- a/packages/contracts-bedrock/contracts/test/Encoding.t.sol +++ b/packages/contracts-bedrock/contracts/test/Encoding.t.sol @@ -10,7 +10,7 @@ contract Encoding_Test is CommonTest { _setUp(); } - function test_nonceVersioning(uint240 _nonce, uint16 _version) external { + function test_nonceVersioning_succeeds(uint240 _nonce, uint16 _version) external { (uint240 nonce, uint16 version) = Encoding.decodeVersionedNonce( Encoding.encodeVersionedNonce(_nonce, _version) ); @@ -18,7 +18,7 @@ contract Encoding_Test is CommonTest { assertEq(nonce, _nonce); } - function test_decodeVersionedNonce_differential(uint240 _nonce, uint16 _version) external { + function testDiff_decodeVersionedNonce_succeeds(uint240 _nonce, uint16 _version) external { uint256 nonce = uint256(Encoding.encodeVersionedNonce(_nonce, _version)); (uint256 decodedNonce, uint256 decodedVersion) = ffi.decodeVersionedNonce(nonce); @@ -27,7 +27,7 @@ contract Encoding_Test is CommonTest { assertEq(_nonce, uint240(decodedNonce)); } - function test_encodeCrossDomainMessage_differential( + function testDiff_encodeCrossDomainMessage_succeeds( uint240 _nonce, uint8 _version, address _sender, @@ -60,7 +60,7 @@ contract Encoding_Test is CommonTest { assertEq(encoding, _encoding); } - function test_encodeDepositTransaction_differential( + function testDiff_encodeDepositTransaction_succeeds( address _from, address _to, uint256 _mint, diff --git a/packages/contracts-bedrock/contracts/test/Hashing.t.sol b/packages/contracts-bedrock/contracts/test/Hashing.t.sol index d1b5ef78ff215..0c679f36738d0 100644 --- a/packages/contracts-bedrock/contracts/test/Hashing.t.sol +++ b/packages/contracts-bedrock/contracts/test/Hashing.t.sol @@ -11,7 +11,7 @@ contract Hashing_Test is CommonTest { _setUp(); } - function test_hashDepositSource() external { + function test_hashDepositSource_succeeds() external { bytes32 sourceHash = Hashing.hashDepositSource( 0xd25df7858efc1778118fb133ac561b138845361626dfb976699c5287ed0f4959, 0x1 @@ -20,7 +20,7 @@ contract Hashing_Test is CommonTest { assertEq(sourceHash, 0xf923fb07134d7d287cb52c770cc619e17e82606c21a875c92f4c63b65280a5cc); } - function test_hashCrossDomainMessage_differential( + function testDiff_hashCrossDomainMessage_succeeds( uint240 _nonce, uint16 _version, address _sender, @@ -54,7 +54,7 @@ contract Hashing_Test is CommonTest { assertEq(hash, _hash); } - function test_hashWithdrawal_differential( + function testDiff_hashWithdrawal_succeeds( uint256 _nonce, address _sender, address _target, @@ -71,7 +71,7 @@ contract Hashing_Test is CommonTest { assertEq(hash, _hash); } - function test_hashOutputRootProof_differential( + function testDiff_hashOutputRootProof_succeeds( bytes32 _version, bytes32 _stateRoot, bytes32 _messagePasserStorageRoot, @@ -98,7 +98,7 @@ contract Hashing_Test is CommonTest { // TODO(tynes): foundry bug cannot serialize // bytes32 as strings with vm.toString - function test_hashDepositTransaction_differential( + function testDiff_hashDepositTransaction_succeeds( address _from, address _to, uint256 _mint, diff --git a/packages/contracts-bedrock/contracts/test/L2ToL1MessagePasser.t.sol b/packages/contracts-bedrock/contracts/test/L2ToL1MessagePasser.t.sol index 81d52c1a45073..e6a787dffc7f5 100644 --- a/packages/contracts-bedrock/contracts/test/L2ToL1MessagePasser.t.sol +++ b/packages/contracts-bedrock/contracts/test/L2ToL1MessagePasser.t.sol @@ -25,7 +25,7 @@ contract L2ToL1MessagePasserTest is CommonTest { messagePasser = new L2ToL1MessagePasser(); } - function test_fuzz_initiateWithdrawal( + function testFuzz_initiateWithdrawal_succeeds( address _sender, address _target, uint256 _value, @@ -60,7 +60,7 @@ contract L2ToL1MessagePasserTest is CommonTest { } // Test: initiateWithdrawal should emit the correct log when called by a contract - function test_initiateWithdrawal_fromContract() external { + function test_initiateWithdrawal_fromContract_succeeds() external { bytes32 withdrawalHash = Hashing.hashWithdrawal( Types.WithdrawalTransaction( messagePasser.nonce(), @@ -88,7 +88,7 @@ contract L2ToL1MessagePasserTest is CommonTest { } // Test: initiateWithdrawal should emit the correct log when called by an EOA - function test_initiateWithdrawal_fromEOA() external { + function test_initiateWithdrawal_fromEOA_succeeds() external { uint256 gasLimit = 64000; address target = address(4); uint256 value = 100; @@ -114,7 +114,7 @@ contract L2ToL1MessagePasserTest is CommonTest { } // Test: burn should destroy the ETH held in the contract - function test_burn() external { + function test_burn_succeeds() external { messagePasser.initiateWithdrawal{ value: NON_ZERO_VALUE }( NON_ZERO_ADDRESS, NON_ZERO_GASLIMIT, diff --git a/packages/contracts-bedrock/contracts/test/OptimismPortal.t.sol b/packages/contracts-bedrock/contracts/test/OptimismPortal.t.sol index 06a7f3ab2362d..6633df2eca287 100644 --- a/packages/contracts-bedrock/contracts/test/OptimismPortal.t.sol +++ b/packages/contracts-bedrock/contracts/test/OptimismPortal.t.sol @@ -11,13 +11,13 @@ import { Hashing } from "../libraries/Hashing.sol"; import { Proxy } from "../universal/Proxy.sol"; contract OptimismPortal_Test is Portal_Initializer { - function test_OptimismPortalConstructor() external { + function test_constructor_succeeds() external { assertEq(op.FINALIZATION_PERIOD_SECONDS(), 7 days); assertEq(address(op.L2_ORACLE()), address(oracle)); assertEq(op.l2Sender(), 0x000000000000000000000000000000000000dEaD); } - function test_OptimismPortalReceiveEth_success() external { + function test_receive_succeeds() external { vm.expectEmit(true, true, false, true); emitTransactionDeposited(alice, alice, 100, 100, 100_000, false, hex""); @@ -38,7 +38,7 @@ contract OptimismPortal_Test is Portal_Initializer { } // Test: depositTransaction should emit the correct log when an EOA deposits a tx with 0 value - function test_depositTransaction_NoValueEOA_success() external { + function test_depositTransaction_noValueEOA_succeeds() external { // EOA emulation vm.prank(address(this), address(this)); vm.expectEmit(true, true, false, true); @@ -62,7 +62,7 @@ contract OptimismPortal_Test is Portal_Initializer { } // Test: depositTransaction should emit the correct log when a contract deposits a tx with 0 value - function test_depositTransaction_NoValueContract_success() external { + function test_depositTransaction_noValueContract_succeeds() external { vm.expectEmit(true, true, false, true); emitTransactionDeposited( AddressAliasHelper.applyL1ToL2Alias(address(this)), @@ -84,7 +84,7 @@ contract OptimismPortal_Test is Portal_Initializer { } // Test: depositTransaction should emit the correct log when an EOA deposits a contract creation with 0 value - function test_depositTransaction_createWithZeroValueForEOA_success() external { + function test_depositTransaction_createWithZeroValueForEOA_succeeds() external { // EOA emulation vm.prank(address(this), address(this)); @@ -103,7 +103,7 @@ contract OptimismPortal_Test is Portal_Initializer { } // Test: depositTransaction should emit the correct log when a contract deposits a contract creation with 0 value - function test_depositTransaction_createWithZeroValueForContract_success() external { + function test_depositTransaction_createWithZeroValueForContract_succeeds() external { vm.expectEmit(true, true, false, true); emitTransactionDeposited( AddressAliasHelper.applyL1ToL2Alias(address(this)), @@ -119,7 +119,7 @@ contract OptimismPortal_Test is Portal_Initializer { } // Test: depositTransaction should increase its eth balance when an EOA deposits a transaction with ETH - function test_depositTransaction_withEthValueFromEOA_success() external { + function test_depositTransaction_withEthValueFromEOA_succeeds() external { // EOA emulation vm.prank(address(this), address(this)); @@ -145,7 +145,7 @@ contract OptimismPortal_Test is Portal_Initializer { } // Test: depositTransaction should increase its eth balance when a contract deposits a transaction with ETH - function test_depositTransaction_withEthValueFromContract_success() external { + function test_depositTransaction_withEthValueFromContract_succeeds() external { vm.expectEmit(true, true, false, true); emitTransactionDeposited( AddressAliasHelper.applyL1ToL2Alias(address(this)), @@ -167,7 +167,7 @@ contract OptimismPortal_Test is Portal_Initializer { } // Test: depositTransaction should increase its eth balance when an EOA deposits a contract creation with ETH - function test_depositTransaction_withEthValueAndEOAContractCreation_success() external { + function test_depositTransaction_withEthValueAndEOAContractCreation_succeeds() external { // EOA emulation vm.prank(address(this), address(this)); @@ -193,7 +193,7 @@ contract OptimismPortal_Test is Portal_Initializer { } // Test: depositTransaction should increase its eth balance when a contract deposits a contract creation with ETH - function test_depositTransaction_withEthValueAndContractContractCreation_success() external { + function test_depositTransaction_withEthValueAndContractContractCreation_succeeds() external { vm.expectEmit(true, true, false, true); emitTransactionDeposited( AddressAliasHelper.applyL1ToL2Alias(address(this)), @@ -215,7 +215,7 @@ contract OptimismPortal_Test is Portal_Initializer { assertEq(address(op).balance, NON_ZERO_VALUE); } - function test_simple_isOutputFinalized_success() external { + function test_simple_isOutputFinalized_succeeds() external { uint256 ts = block.timestamp; vm.mockCall( address(op.L2_ORACLE()), @@ -234,7 +234,7 @@ contract OptimismPortal_Test is Portal_Initializer { assertEq(op.isOutputFinalized(0), true); } - function test_isOutputFinalized_success() external { + function test_isOutputFinalized_succeeds() external { uint256 checkpoint = oracle.nextBlockNumber(); uint256 nextOutputIndex = oracle.nextOutputIndex(); vm.roll(checkpoint); @@ -397,7 +397,7 @@ contract OptimismPortal_FinalizeWithdrawal_Test is Portal_Initializer { // Test: proveWithdrawalTransaction succeeds if the passed transaction's withdrawalHash has // already been proven AND the output root has changed AND the l2BlockNumber stays the same. - function test_proveWithdrawalTransaction_replayProveChangedOutputRoot_success() external { + function test_proveWithdrawalTransaction_replayProveChangedOutputRoot_succeeds() external { vm.expectEmit(true, true, true, true); emit WithdrawalProven(_withdrawalHash, alice, bob); op.proveWithdrawalTransaction( @@ -440,7 +440,7 @@ contract OptimismPortal_FinalizeWithdrawal_Test is Portal_Initializer { } // Test: proveWithdrawalTransaction succeeds and emits the WithdrawalProven event. - function test_proveWithdrawalTransaction_validWithdrawalProof_success() external { + function test_proveWithdrawalTransaction_validWithdrawalProof_succeeds() external { vm.expectEmit(true, true, true, true); emit WithdrawalProven(_withdrawalHash, alice, bob); op.proveWithdrawalTransaction( @@ -452,7 +452,7 @@ contract OptimismPortal_FinalizeWithdrawal_Test is Portal_Initializer { } // Test: finalizeWithdrawalTransaction succeeds and emits the WithdrawalFinalized event. - function test_finalizeWithdrawalTransaction_provenWithdrawalHash_success() external { + function test_finalizeWithdrawalTransaction_provenWithdrawalHash_succeeds() external { uint256 bobBalanceBefore = address(bob).balance; vm.expectEmit(true, true, true, true); @@ -802,7 +802,7 @@ contract OptimismPortal_FinalizeWithdrawal_Test is Portal_Initializer { assert(address(bob).balance == bobBalanceBefore); } - function test_finalizeWithdrawalTransaction_differential_success( + function testDiff_finalizeWithdrawalTransaction_succeeds( address _sender, address _target, uint256 _value, @@ -878,7 +878,7 @@ contract OptimismPortalUpgradeable_Test is Portal_Initializer { proxy = Proxy(payable(address(op))); } - function test_params_initValuesOnProxy_success() external { + function test_params_initValuesOnProxy_succeeds() external { (uint128 prevBaseFee, uint64 prevBoughtGas, uint64 prevBlockNum) = OptimismPortal( payable(address(proxy)) ).params(); @@ -897,7 +897,7 @@ contract OptimismPortalUpgradeable_Test is Portal_Initializer { OptimismPortal(opImpl).initialize(); } - function test_upgradeToAndCall_upgrading_success() external { + function test_upgradeToAndCall_upgrading_succeeds() external { // Check an unused slot before upgrading. bytes32 slot21Before = vm.load(address(op), bytes32(uint256(21))); assertEq(bytes32(0), slot21Before); diff --git a/packages/contracts-bedrock/package.json b/packages/contracts-bedrock/package.json index 4d8e40be13196..2e1a52c28efc6 100644 --- a/packages/contracts-bedrock/package.json +++ b/packages/contracts-bedrock/package.json @@ -25,7 +25,7 @@ "test": "yarn build:differential && forge test", "coverage": "yarn build:differential && forge coverage", "coverage:lcov": "yarn build:differential && forge coverage --report lcov", - "gas-snapshot": "yarn build:differential && forge snapshot --no-match-test 'differential|fuzz'", + "gas-snapshot": "yarn build:differential && forge snapshot --no-match-test 'testDiff|testFuzz'", "storage-snapshot": "./scripts/storage-snapshot.sh", "validate-spacers": "hardhat validate-spacers", "slither": "./scripts/slither.sh", diff --git a/packages/contracts-bedrock/scripts/forge-test-names.ts b/packages/contracts-bedrock/scripts/forge-test-names.ts new file mode 100644 index 0000000000000..6a002e8459db0 --- /dev/null +++ b/packages/contracts-bedrock/scripts/forge-test-names.ts @@ -0,0 +1,81 @@ +import fs from 'fs' +import path from 'path' + +const testPath = './contracts/test' +const testFiles = fs.readdirSync(testPath) + +// Given a test function name, ensures it matches the expected format +const handleFunctionName = (name: string) => { + if (!name.startsWith('test')) { + return + } + const parts = name.split('_') + parts.forEach((part) => { + // Good enough approximation for camelCase + if (part[0] !== part[0].toLowerCase()) { + throw new Error( + `Invalid test name: ${name}.\n Test name parts should be in camelCase` + ) + } + }) + if (parts.length < 3 || parts.length > 4) { + throw new Error( + `Invalid test name: ${name}.\n Test names should have either 3 or 4 parts, each separated by underscores` + ) + } + if (!['test', 'testFuzz', 'testDiff'].includes(parts[0])) { + throw new Error( + `Invalid test name: ${name}.\n Names should begin with either "test" or "testFuzz"` + ) + } + if ( + !['succeeds', 'reverts', 'fails', 'benchmark', 'works'].includes( + parts[parts.length - 1] + ) && + parts[parts.length - 2] !== 'benchmark' + ) { + throw new Error( + `Invalid test name: ${name}.\n Test names should end with either "succeeds", "reverts", "fails", "differential" or "benchmark[_num]"` + ) + } + if ( + ['reverts', 'fails'].includes(parts[parts.length - 1]) && + parts.length < 4 + ) { + throw new Error( + `Invalid test name: ${name}.\n Failure tests should have 4 parts. The third part should indicate the reason for failure.` + ) + } +} + +// Todo: define this function for validating contract names +// Given a test contract name, ensures it matches the expected format +const handleContractName = (name: string) => { + name +} + +for (const testFile of testFiles) { + const filePath = path.join(testPath, testFile) + const lines = fs + .readFileSync(filePath, 'utf-8') + .split('\n') + .map((l) => l.trim()) + let currentContract: string + for (const line of lines) { + if (line.startsWith('contract')) { + currentContract = line.split(' ')[1] + handleContractName(line) + continue + } else if (line.startsWith('function')) { + const funcName = line.split(' ')[1].split('(')[0] + try { + handleFunctionName(funcName) + } catch (error) { + throw new Error( + `In ${filePath}::${currentContract}:\n ${error.message}` + ) + } + continue + } + } +}