Chore/move tests to forge #318
Conversation
OT-kraftchain
changed the base branch from
develop
to
feat/initializable-message-bridge
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the test suite from Hardhat (TypeScript) to Forge (Solidity), completing the transition for both Bridge Implementation and Bridge Management test suites with 87 total tests migrated.
Key changes:
- All 50 Bridge Implementation tests successfully migrated to Forge with minor fixes for event emission ordering
- All 37 Bridge Management tests migrated with 1:1 mapping to original functionality
- Test helper contracts refactored to use production
MessageBridgeinstead of test version - Documentation emoji formatting standardized across deployment scripts
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test-equivalence.md | New comprehensive tracking document mapping all 87 migrated tests from Hardhat to Forge with migration notes |
| test/BridgeImplementationForge.t.sol | Complete Bridge Implementation test suite (50 tests) migrated to Solidity with helper functions and fixed event ordering |
| test/BridgeManagementForge.t.sol | Complete Bridge Management test suite (37 tests) migrated to Solidity with role and validator management tests |
| test/MessageBridgeTestHelper.sol | Updated to use production MessageBridge contract instead of TestMessageBridge |
| contracts/tests/TestMessageBridge.sol | Simplified to delegate initialization to parent MessageBridge class |
| contracts/messageBridge/MessageBridge.sol | Added initialize and __initialize functions to support both production and test initialization patterns |
| scripts/deploy/*.ts | Standardized console output by removing emoji characters from deployment logging |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
OT-kraftchain
force-pushed
the
chore/move-tests-to-forge
branch
from
a221f6e to
8823d5e
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| external | ||
| override | ||
| initializer |
There was a problem hiding this comment.
The override keyword is used here, but the parent MessageBridge.initialize function is marked as virtual, not as an interface function. Additionally, both this function and the parent use the initializer modifier. When overriding, only one initializer modifier should be used in the inheritance chain to avoid issues. The parent's initializer modifier will not be invoked when this override is called. Consider removing the initializer modifier from this override since __initialize uses onlyInitializing which works correctly in the override pattern.
| initializer |
Test Migration Status: Hardhat to Forge
Summary
Detailed Test Mappings
Bridge Implementation Tests
Deployment Block (2 tests)
test_BridgeShouldBeInitializedToCorrectVersiontest_BridgeManagementShouldBeInitializedToCorrectVersionParameter setters Block (6 tests)
test_SetWithdrawalFeetest_SetMinWithdrawalAmounttest_SetMaxWithdrawalAmounttest_SetInvalidMinAndMaxWithdrawalAmountstest_SetMaxDepositsPerDistributiontest_FailSettingMaxDepositsPerDistributionToZeroFunding the bridge Block (3 tests)
test_FundTheBridgeContracttest_NotFunderFailsToFundTheBridgeContracttest_CanFundAfterSetAsFunderDeposit Block (19 tests)
test_DepositTheFirstNoncetest_DepositWithSignaturesOutOfAnyOrdertest_DepositWithMultipleContinuousNoncetest_DepositWithMultipleTimesWithDifferentNonceArraytest_BridgeTwoDepositsWithInsufficientFundsForExecutingTheFirstDeposittest_BridgeThreeDepositsWithInsufficientFundsForExecutingTheSecondDeposittest_DepositWhenRecipientIsContracttest_DepositWhenDepositLengthIsEqualTo10test_DepositWhenRecipientAddressIsZeroAddresstest_ShouldRevertWithNoDepositstest_ShouldRevertWhenProvidingTooManyDepositsInSingleTransactiontest_ShouldRevertWithWrongFirstNoncetest_ShouldRevertWhenNonceIsNotSubsequenttest_ShouldRevertWhenSignatureLengthLessThan5test_ShouldRevertWhenSignatureLengthIs5ButWithTwoDuplicateSignaturetest_ShouldRevertWhenSignatureVerifyFailedtest_ShouldRevertWhenRootIsInvalidWithdraw Block (6 tests)
test_WithdrawOnlyOncetest_WithdrawMultipleTimestest_WithdrawWithAmountEdgeCasetest_WithdrawWithTheWrongAmounttest_WithdrawIsTooLowtest_WithdrawIsTooHightest_WithdrawWithDifferentFeesAndVerifyUnclaimedRewardsClaim Block (4 tests)
test_ClaimSuccessfulForEOAAccounttest_ClaimSuccessfulForPayableContracttest_FailToClaimDueToContractNotPayabletest_FailToClaimForNotExistedNonceInTheClaimTablePausing Block (6 tests)
test_PauseWithGovernorOrSecurityGuardAndUnpauseWithGovernortest_CanOnlyPauseIfGovernorOrSecurityGuardtest_CannotUnpauseContractIfItsAlreadyUnpausedtest_CannotPauseContractIfItsPausedAlreadytest_CannotDepositClaimOrWithdrawNativeCoinIfContractIsPausedWithdrawal Pausing Block (6 tests)
test_GovernorCanPauseWithdrawalstest_GovernorCanUnpauseWithdrawalstest_NonGovernorCannotPauseWithdrawalstest_NonGovernorCannotUnpauseWithdrawalstest_NativeCoinWithdrawalsAreRejectedWhileWithdrawalsArePausedtest_NativeCoinDepositsAreAllowedWhileWithdrawalsArePausedBridge Management Tests
Deployment Block (7 tests)
test_ShouldBeInitializedToTheCorrectVersiontest_ShouldHaveTheRightRelayertest_ShouldHaveTheRightValidatorstest_ShouldHaveTheRightOwnertest_ShouldHaveTheRightGovernortest_ShouldHaveTheRightSecurityGuardtest_ShouldHaveTheRightFunderBridge role setting Block (10 tests)
test_SetOwnertest_SetRelayertest_SetGovernortest_SetSecurityGuardtest_SetFundertest_NonOwnerFailToStartOwnershipTransfertest_NonOwnerFailToSetRelayertest_NonOwnerFailToSetGovernortest_NonOwnerFailToSetSecurityGuardtest_NonOwnerFailToSetFunderBridge validator changes Block (20 tests)
test_AddValidatorAndIncreaseThresholdtest_AddValidatorAndKeepCurrentThresholdtest_FailAddingValidatorWithoutAuthorizationtest_FailAddingValidatorThatIsAlreadyAValidatortest_FailAddingValidatorWithZeroAddresstest_RemoveValidatorAndDecreaseCurrentThresholdtest_RemoveValidatorAndKeepCurrentThresholdtest_FailRemovingValidatorWithoutAuthorizationtest_FailRemovingValidatorIfNotAValidatortest_FailRemovingValidatorAndKeepCurrentThresholdIfThresholdWasEqualToNumberOfValidatorstest_FailRemovingValidatorIfThereAreOnlyTwoValidatorstest_ReplaceValidatortest_FailReplacingValidatorWithoutAuthorizationtest_FailReplacingValidatorWithZeroAddresstest_FailReplacingAValidatorAddressThatIsNoValidatortest_FailReplacingValidatorWithAddressThatIsAlreadyAValidatortest_SetValidatorThresholdtest_FailSettingValidatorThresholdWithoutAuthorizationtest_FailSettingValidatorThresholdLowerThan2test_FailSettingValidatorThresholdHigherThanNumberOfValidatorsMigration Notes
Key Fixes Applied During Migration
Event Emission Order Issues:
test_DepositWhenRecipientAddressIsZeroAddress(): Fixed event order expectations -NativeDepositRootUpdateemitted beforeNativeDeposittest_DepositWithMultipleTimesWithDifferentNonceArray(): Fixed event order expectations - same as aboveNaming Conventions:
describeblocks withit()functions using human-readable stringsfunction test_*()naming with CamelCase conversion