test: migrate Foundry unit tests to integration-style#2132
Open
valera-grinenko-ai wants to merge 20 commits intomatter-labs:draft-v31from
Open
test: migrate Foundry unit tests to integration-style#2132valera-grinenko-ai wants to merge 20 commits intomatter-labs:draft-v31from
valera-grinenko-ai wants to merge 20 commits intomatter-labs:draft-v31from
Conversation
Migrate 218 of 254 Foundry unit test files to run against a fully deployed L1 ecosystem via MigrationTestBase, replacing isolated mock-heavy setups with real contract interactions. Key changes: - Add _SharedMigrationBase.t.sol deploying full L1 ecosystem + ZK chain + UtilsFacet - Extend UtilsFacet with 20 new methods for Getters test compatibility (72 -> 92 selectors) - Rewrite 15 shared test bases to inherit from MigrationTestBase - Remove ~10 unnecessary vm.mockCall where real deployed contracts suffice - Fix bug in BridgehubRequestL2Transaction where coincidental makeAddr masked wrong prank target - Fix ProvingL2LogsInclusion recursive proofs (prank consumption, state corruption, v31 placeholder) - 36 pure library tests intentionally kept on Test (solc OOM with full ecosystem, zero coverage benefit) Validation: 2517 passed, 1 failed (pre-existing UpgradeTestv31_Local - missing system-contracts zkout) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
valera-grinenko-ai
requested review from
StanislavBreadless,
kelemeno and
vladbochok
as code owners
Unit tests now deploy a full L1 ecosystem via MigrationTestBase, making them very slow under forge coverage instrumentation. Since the same production code paths are covered by integration tests in l1/integration/, exclude unit tests from the coverage run to keep CI times manageable. The test:foundry step (which runs before coverage) still validates all unit tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This reverts commit dd5d5ad.
MigrationTestBase.setUp() was deploying the full L1 ecosystem (~50 contracts) for every test contract. Under forge coverage, this made coverage runs ~6x slower (36+ min vs 6 min baseline) since 124 test contracts each triggered a full ecosystem deployment with instrumentation. Fix: setUp() is now a no-op. Only the 5 shared bases that actually USE the ecosystem (Admin, Getters, Base, Migrator, Mailbox) call _deployIntegrationBase() explicitly. The other ~119 test contracts inherit MigrationTestBase for its helper methods (UtilsCallMockerTest, etc.) without paying for deployment. Also reverts the CI exclusion commit (coverage now runs unit tests normally). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tests
- _Admin_Shared: Remove DummyBridgehub (UtilsFacet setters have no access
control, so pranking as bridgehub before calling them was unnecessary)
- AdminExtended: Use real CTM from integration deployment instead of
overriding CTM to address(this) via DummyBridgehub prank pattern
- UpgradeChainFromVersion: Use real CTM address for upgradeCutHash mocks
instead of makeAddr("chainTypeManager"), remove util_setChainTypeManager
overrides that broke integration state
- EraMultisigValidator: Deploy with real bridgehub and use real chain from
_deployIntegrationBase() instead of DummyBridgehub + mocked IGetters
- MultisigCommitter: Same as EraMultisigValidator - use real bridgehub and
real chain, removing DummyBridgehub and IGetters.getAdmin/getChainId mocks
- Remove agent_review_report.md (not production code)
Forwarded-call mocks on ValidatorTimelock (commit/prove/execute/revert) are
retained as valid test isolation: these tests verify multisig/commit logic,
not batch processing.
Validation: 2497 passed, 1 failed (pre-existing UpgradeTestv31_Local)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…r tests
Replace makeAddr("bridgehub") and makeAddr("assetRouter") with real deployed
contracts from _deployIntegrationBase(). The GatewayTransactionFilterer is now
constructed with the real L1Bridgehub and L1AssetRouter addresses.
- _GatewayTransactionFilterer_Shared: convert constructor-based setup to
setUp() pattern, call _deployIntegrationBase(), use real addresses
- GatewayTransactionFiltererAdditional: same migration, use real addresses
Retained mocks: ctmAssetIdToAddress on real bridgehub (test-specific control
of CTM lookup results for valid/invalid scenarios).
Validation: 23 GatewayTransactionFilterer tests passed
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace makeAddr("assetRouter") with real L1AssetRouter from
_deployIntegrationBase(). Convert constructor-based setup to setUp().
Validation: 12 PrividiumTransactionFilterer tests passed
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace DummyBridgehub and makeAddr("zkSync") with real deployed contracts
from _deployIntegrationBase(). ValidatorTimelock is now constructed with the
real L1Bridgehub, uses the real chain address for getAdmin/getChainId queries,
and the real chain's admin for access control.
- Remove DummyBridgehub, DummyChainTypeManagerForValidatorTimelock
- Remove vm.mockCall for IGetters.getAdmin() and IGetters.getChainId()
- Use real chain from integration deployment
- For fakeChain test (NotAZKChain): mock getZKChain on real bridgehub
- Add missing executeBatchesSharedBridge forwarded-call mock that was
previously hidden by makeAddr (no-code address silently accepted calls)
Forwarded-call mocks (commit/prove/execute/revert) on real chain address
retained as valid test isolation for timelock logic.
Validation: 32 ValidatorTimelock tests passed
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DataAvailabilityDeployedAddresses was restructured: rollupDAManager and l1RollupDAValidator moved into nested daContracts (DAContracts). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The PR's yarn.lock unified @types/node@* and @types/node@^17.0.34 into a single resolution at v17.0.45, which is too old for Node 20+ APIs (Dirent.path, readdirSync recursive) used by calculate-hashes.ts. Restore the base branch yarn.lock which correctly resolves @types/node@* to v20.12.6 separately from @types/node@^17.0.34. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tions These mocks called vm.mockCall on the real CTM to return the current protocol version — the same value the real CTM already returns. The integration base deploys both CTM and chain with matching versions, making the mocks unnecessary. Kept: the test-specific mock that returns a DIFFERENT version to test the OutdatedProtocolVersion error path. Validation: 36 Migrator tests passed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…dant mocks ChainTypeManager shared base: - Add setUp() that calls _deployIntegrationBase(), activating the integration path in deploy() that was previously dead code (all 15 child tests were hitting the fallback standalone-ecosystem path) - Remove mockDiamondInitInteropCenterCallsWithAddress from createNewChain() since real bridgehub provides real assetRouter/NTV/assetTracker - Simplify _mockMigrationPausedFromBridgehub to mock on real chainAssetHandler instead of creating a fake one PermanentRestriction keeps working via its own setUp() that calls deploy() directly without _deployIntegrationBase(). ForwardedBridgeFunctions: - Remove 6 redundant protocolVersion() mocks that returned the same value the real CTM already returns Validation: 2512 tests passed, 0 failed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace makeAddr("bridgehub") and makeAddr("messageRoot") with real
deployed contracts from _deployIntegrationBase(). Fresh L1AssetRouter,
L1Nullifier, L1NativeTokenVault are still created as contracts-under-test
but now use real bridgehub/messageRoot as constructor dependencies.
Removed mocks:
- bridgehub.chainAssetHandler() (real bridgehub provides)
- bridgehub.v31UpgradeChainBatchNumber() (real messageRoot provides)
- l1NullifierAddress.getTransientSettlementLayer() (dead mock on address(0))
Uses real chain IDs (testChainId, zkChainIds[0]) and real eraDiamondProxy.
Kept mocks:
- requestL2TransactionDirect (legacy deposit path)
- baseTokenAssetId/settlementLayer/baseToken (blanket mocks for
legacy routing with fresh contract's internal resolution)
- getProofData (real messageRoot proof format differs)
- migrationNumber on fresh L1AssetTracker's uninitialized chainAssetHandler
- NTV tokenAddress (fresh NTV token mapping)
Validation: 74 L1SharedBridge tests passed, 2512 total passed
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace makeAddr("bridgeHub") with real bridgehub from integration.
Fresh L1MessageRoot is still created (contract under test) but uses
real bridgehub as dependency. Removed 7 setUp mocks; kept 1 genesis
construction mock. Test-specific mocks remain for controlled scenarios.
Validation: 9 MessageRoot tests passed, 2512 total passed
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
MessageRoot_Extended: replace fresh L1Bridgehub with real one, remove 6 setUp mocks (L1_CHAIN_ID, chainAssetHandler, owner, chainTypeManager, settlementLayer, getAllZKChainChainIDs). Keep 1 genesis construction mock. L1MessageRoot_V31Upgrade: replace makeAddr bridgehub with real, remove 1 setUp mock (chainAssetHandler). Keep genesis construction mock. Test-specific mocks for getZKChain, settlementLayer, getTotalBatchesExecuted remain (control specific error path scenarios). L1MessageRoot_PlaceholderRegressionTest: replace makeAddr bridgehub with real, remove 1 setUp mock (chainAssetHandler). Test function mocks remain (control V31 upgrade scenarios with specific chain registration state). Validation: 120 Bridgehub tests passed, 2512 total passed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ayBase
ChainTypeManager: replace makeAddr("baseToken") with real ERC20 from
integration deployment (tokens[0]). Remove 6 IERC20Metadata mocks
(name/symbol/decimals × 2 helpers). Falls back to makeAddr when
integration is not deployed (PermanentRestriction fallback path).
L1GatewayBase: replace makeAddr bridgehub/assetRouter/NTV with real
deployed contracts. Remove 2 mocks (assetRouter, nativeTokenVault).
Keep 3 mocks for test-specific data (unregistered chainId 123,
fake baseTokenAssetId).
Validation: 2512 tests passed, 0 failed
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…Batches Real bridgehub provides assetRouter/NTV/assetTracker — the 6-mock interop center helper is unnecessary when using integration base. Validation: 2512 tests passed, 0 failed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
migrationNumber() is a Solidity mapping that defaults to 0 for all unmapped keys. Mocking it to return 0 on the real chainAssetHandler is redundant — the real contract already returns 0. Removed from: AssetRouterTest, L1ChainAssetHandlerTest This establishes the correct baseline: integration tests should not mock values that the real deployed contracts already provide. Validation: 2512 tests passed, 0 failed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
L1SharedBridge shared base (6 mocks removed): - Deploy L1AssetTracker via proxy so initialize()+setAddresses() works. This sets chainAssetHandler from real bridgehub, eliminating the migrationNumber mock on address(0). - Remove blanket baseTokenAssetId/settlementLayer mocks — real bridgehub already returns correct values for registered chains. - Remove baseToken(chainId) mock — real bridgehub has this for testChainId. - Remove 2 NTV tokenAddress mocks — registerToken()/registerEthToken() already set the tokenAddress mapping correctly. Integration tests (2 mocks removed): - Remove migrationNumber → 0 mocks from AssetRouterTest and L1ChainAssetHandlerTest — Solidity mapping default is already 0. Remaining L1SharedBridge setUp mocks (3): - requestL2TransactionDirect (legacy deposit path) - getProofData (no real proof data — same as integration tests) - _setBaseTokenAssetId helper (child tests override base token) Validation: 2512 tests passed, 0 failed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MigrationTestBase, replacing isolated mock-heavy setups with real contract interactionsUtilsFacetwith 20 new methods for Getters test compatibility (72 → 92 selectors)BridgehubRequestL2Transactionwhere coincidentalmakeAddrmasked wrong prank targetProvingL2LogsInclusionrecursive proofs (prank consumption by argument evaluation, integration state corruption, missing v31 placeholder for settlement layer)Test(solc OOM with full ecosystem overhead, zero coverage benefit for pure/view functions)Mock reduction: 392 → 366
Structural changes (real ecosystem wiring):
_deployIntegrationBase()path that was previously dead code. All 15 CTM child tests now use real bridgehub. RemovedmockDiamondInitInteropCenterCallsWithAddress. Use real ERC20 forbaseToken(eliminates IERC20Metadata mocks).DummyBridgehub. Use real CTM from integration.DummyBridgehub+IGetters.getAdmin()/getChainId()mocks.L1BridgehubandL1AssetRouter.makeAddr). Removed chainAssetHandler, v31UpgradeChainBatchNumber, getTransientSettlementLayer mocks.protocolVersion()mocks.Other fixes:
@types/noderesolution.agent_review_report.md.Remaining 366 mocks — classification
upgradeCutHashwith wrong hash,getTotalBatchesExecuted→ 0,settlementLayer→ wrong chain,migrationPaused→ true, V31 upgrade scenarios with specific chain states, legacy ERA deposit pathsThe remaining test-specific mocks cannot be removed by wiring to real contracts — they exist to control specific return values that trigger specific error paths (wrong hashes, zero batch counts, unregistered chain IDs). Removing them would require rewriting the tests to trigger the same error conditions through real contract state manipulation, which is a fundamentally different testing approach.
Test plan
forge test --threads 1 --ffi→ 2512 passed, 0 failedyarn lint:sol --fix --noPromptpasses cleanyarn prettier:fixpasses clean🤖 Generated with Claude Code