Improve performance by sharing client object within test suites#609
Merged
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
The pull request correctly refactors most test suites to share a client object, improving performance. However, packages/shared/src/accounts.ts contains a couple of issues related to the new test setup logic that could lead to bugs and resource leaks.
Suggestions that couldn't be attached to a specific line
packages/shared/src/accounts.ts:1179, 1354, 1461, 1564, 1743
Several test functions (e.g., forceTransferKillTest, forceTransferBelowExistentialDepositTest) call setupNetworks internally. This is incorrect within the new test structure as it registers additional beforeEach and afterAll hooks, leading to resource leaks and unpredictable test execution. These tests need to be refactored to use the suite-level client or manage clients without using setupNetworks inside the test body. For example, using createNetworks and manually tearing down the clients in a finally block within the test, or restructuring the test suite to handle the conditional client setup at the suite level.
The conditional 1- to 3-chain network creation in upgrade tests cannot be lifted to a suite-level beforeAll because the same-vs-distinct chain decision is per-test. Replaced setupNetworks (which registers describe-level beforeEach/afterAll hooks at runtime, leaking state across tests) with createNetworks plus a try/finally that disconnects every distinct client at the end of each test.
…e rejections After a tx hits isInBlock/isFinalized, sendTransaction's deferred is resolved but the polkadot.js subscription stays active and keeps firing. A subsequent isError callback (e.g. a tx-pool rejection that arrives after the block was already produced) calls deferred.reject on the already-settled promise — a no-op for that promise, but the underlying signed.send subscription's own error path can still produce unhandled rejections. Two changes: explicitly unsubscribe after either terminal status, and attach a no-op catch to the deferred promise so any late settle is silenced rather than escaping as an unhandled rejection.
Collaborator
Author
|
@xlc this needs AcalaNetwork/chopsticks#1028 (which is still WIP) so that the |
xlc
approved these changes
May 8, 2026
Replaces the hand-crafted patches with full rebuilds from AcalaNetwork/chopsticks#pet-perf-stack (commit e267237). Adds chopsticks-db patch covering the new PagedKeys and RpcCall entities (§1 of #606). Updates chopsticks-core and chopsticks-utils patches to include all remaining commits: getKeysPaged guard refinement, getKeysPaged disk cache, cache-hit extension fix, in-flight dedup, setHead subscriber fix, and dryRunExtrinsicsAmortized. Drop when chopsticks#1028 is published.
Replaces the sendTransaction + newBlock per-proxy-action loop with a single dryRunExtrinsicsAmortized batch. Pre-signs every action with the proxy account's current nonce (storage layer pops between extrinsics revert nonce, so sequential nonces would be rejected). Adds eventsFromAmortizedDryRunResult helper that decodes the system events Vec from a per-extrinsic storageDiff and returns a { events: Promise<Codec[]> } shape compatible with checkEvents. Verified locally on Polkadot proxy filtering tests (16 of 16 pass).
Each XCM-based test in the accounts suite was spinning up its own ephemeral relay+parachain pair via createNetworks inside the test body, then tearing them down with a per-test teardownExtras closure. Move that to the suite-level beforeAll: accountsE2ETests now takes an optional relayChain parameter, creates the connected pair once in beforeAll, restores both via captureSnapshot in beforeEach, and tears both down in afterAll. The 13 XCM test functions now take an already-connected relayClient instead of a relayChain plus the boilerplate to bootstrap one. Drops the relayChain field from AccountsTestConfig (it is no longer test-local data) and the TInitStoragesRelay generic from the test fns. Verified locally: peoplePolkadot.accounts (55/55 pass, uses relayClient path) and assetHubPolkadot.accounts (46/46 pass, no-relay path).
addRegistrarViaRelayAsRoot was creating its own ephemeral relay+people pair via createNetworks on every invocation, ignoring the peopleClient already provisioned by basePeopleChainE2ETests's beforeAll. Move relay creation to suite-level beforeAll, restore both via captureSnapshot(peopleClient, relayClient) in beforeEach, and tear both down in afterAll. The function now takes the suite-shared (relayClient, peopleClient) directly. Verified: peoplePolkadot.e2e (5/5 pass).
rockbmb
commented
May 9, 2026
Collaborator
Author
rockbmb
left a comment
rockbmb
left a comment
There was a problem hiding this comment.
Bot's flagged tests use createNetworks + manual teardown, not setupNetworks — no hook leak.
naijauser
pushed a commit
to naijauser/polkadot-ecosystem-tests
that referenced
this pull request
May 14, 2026
…-web3-stack#609) * Add beforeEach to DescribeNode * Snapshot restore truncates storage layers * Fix network teardown * Share network across accounts suite * Share network across scheduler suite * Share network across bounties suite * Share network across childBounties suite * Share network across vesting suite * Share network across governance suite * Share network across multisig suite * Share network across multisig.proxy suite * Share network across nomination-pools suite * Share network across staking suite * Share network across proxy suite * Share network across remoteProxy suite * Share network across bounties suite * Share network across childBounties suite * Share network across governance suite * Share network across multisig suite * Share network across multisig.proxy suite * Share network across preimage suite * Share network across proxy suite * Share network across remoteProxy suite * Share network across staking suite * Patch chopsticks-core to expose resetStorageLayers on Block * Update yarn.lock for chopsticks-core patch * Switch CI to forks pool with 5 workers The shared-client refactor is incompatible with vitest's threads pool: multiple workers contend for the same chopsticks WS server, causing RPC timeouts and retry loops that see leaked state from earlier attempts. Forks pool gives each worker its own process and avoids the contention entirely. * Fix accounts suite teardown and await dev.setHead Two issues flagged by automated review on PR open-web3-stack#609: 1. The accounts suite's beforeEach was missing an await on baseClient.dev.setHead(blockNumber). The setHead RPC is async and the unawaited promise leaves a pending request that can reject after the test completes. 2. The relay-fallback paths in the force_transfer/force_unreserve/ force_set_balance/force_adjust_total_issuance tests called setupNetworks from inside the test body. setupNetworks registers beforeEach and afterAll hooks at the describe level, conflicting with the suite-level lifecycle and producing resource leaks. The chains that take this path (e.g. bridgeHubPolkadot, which lacks the scheduler pallet) hit assertion failures and timeouts. Replaced with createNetworks plus a per-function teardownExtras closure that disconnects and tears down both the relay and re-created base clients at the end of the test. * Await setupBalances calls in preimage tests Nine call sites kicked off the storage seed without awaiting it. The underlying setStorage RPC can reject asynchronously on chains with strict transaction-pool validation (basilisk, karura), and the unawaited rejection escaped as an unhandled rejection after the test had moved on. * Bump block numbers * Fix lint errors after shared-client refactor - postAhmFiltering: PostAhmTest.testFn was typed (chain: Chain<>) but the leaf functions it points to all take Client<>; updated the type. - preimage, proxy, scheduler: drop unused setupNetworks imports left over from the refactor. - governance: drop the dead 'chain' parameter from injectDecisionPeriodEnd (unused since the function was refactored to take a Client). * Share network across people suite * Share networks across treasury suite * Share networks across collectives suite * Share network across configuration suite * Use createNetworks with explicit teardown in upgrade suite The conditional 1- to 3-chain network creation in upgrade tests cannot be lifted to a suite-level beforeAll because the same-vs-distinct chain decision is per-test. Replaced setupNetworks (which registers describe-level beforeEach/afterAll hooks at runtime, leaking state across tests) with createNetworks plus a try/finally that disconnects every distinct client at the end of each test. * Share networks across system suite * Share network across recovery suite * Patch chopsticks-utils.sendTransaction to unsubscribe and swallow late rejections After a tx hits isInBlock/isFinalized, sendTransaction's deferred is resolved but the polkadot.js subscription stays active and keeps firing. A subsequent isError callback (e.g. a tx-pool rejection that arrives after the block was already produced) calls deferred.reject on the already-settled promise — a no-op for that promise, but the underlying signed.send subscription's own error path can still produce unhandled rejections. Two changes: explicitly unsubscribe after either terminal status, and attach a no-op catch to the deferred promise so any late settle is silenced rather than escaping as an unhandled rejection. * Update Bifrost Kusama <-> KAH XCM snap * Skip failing CoretimeK <-> KAH XCM test * Bump block numbers * Skip failing KAH <-> PeopleKusama XCM tests * Regenerate chopsticks patches from pet-perf-stack branch tip Replaces the hand-crafted patches with full rebuilds from AcalaNetwork/chopsticks#pet-perf-stack (commit e267237). Adds chopsticks-db patch covering the new PagedKeys and RpcCall entities (§1 of open-web3-stack#606). Updates chopsticks-core and chopsticks-utils patches to include all remaining commits: getKeysPaged guard refinement, getKeysPaged disk cache, cache-hit extension fix, in-flight dedup, setHead subscriber fix, and dryRunExtrinsicsAmortized. Drop when chopsticks#1028 is published. * Use dryRunExtrinsicsAmortized for proxy call-filtering loop Replaces the sendTransaction + newBlock per-proxy-action loop with a single dryRunExtrinsicsAmortized batch. Pre-signs every action with the proxy account's current nonce (storage layer pops between extrinsics revert nonce, so sequential nonces would be rejected). Adds eventsFromAmortizedDryRunResult helper that decodes the system events Vec from a per-extrinsic storageDiff and returns a { events: Promise<Codec[]> } shape compatible with checkEvents. Verified locally on Polkadot proxy filtering tests (16 of 16 pass). * Hoist accounts relay client to suite scope Each XCM-based test in the accounts suite was spinning up its own ephemeral relay+parachain pair via createNetworks inside the test body, then tearing them down with a per-test teardownExtras closure. Move that to the suite-level beforeAll: accountsE2ETests now takes an optional relayChain parameter, creates the connected pair once in beforeAll, restores both via captureSnapshot in beforeEach, and tears both down in afterAll. The 13 XCM test functions now take an already-connected relayClient instead of a relayChain plus the boilerplate to bootstrap one. Drops the relayChain field from AccountsTestConfig (it is no longer test-local data) and the TInitStoragesRelay generic from the test fns. Verified locally: peoplePolkadot.accounts (55/55 pass, uses relayClient path) and assetHubPolkadot.accounts (46/46 pass, no-relay path). * Hoist people relay client to suite scope addRegistrarViaRelayAsRoot was creating its own ephemeral relay+people pair via createNetworks on every invocation, ignoring the peopleClient already provisioned by basePeopleChainE2ETests's beforeAll. Move relay creation to suite-level beforeAll, restore both via captureSnapshot(peopleClient, relayClient) in beforeEach, and tear both down in afterAll. The function now takes the suite-shared (relayClient, peopleClient) directly. Verified: peoplePolkadot.e2e (5/5 pass). * Use more vitest forks in CI workflow
rockbmb
added a commit
to rockbmb/runtimes
that referenced
this pull request
May 18, 2026
Brings the PET ecosystem-tests job in this repo's CI into line with the perf improvements landed in https://github.com/open-web3-stack/polkadot-ecosystem-tests over the last few weeks: - Shard the matrix 3 ways per network via Vitest's `--shard`, so the two ecosystem-test jobs (polkadot, kusama) become six running in parallel on separate self-hosted runners. - Switch from `--pool=threads --maxWorkers=3` to `--pool=forks --maxWorkers=8`. The shared-client refactor (open-web3-stack/polkadot-ecosystem-tests#609) showed retry-induced state leaks under the threads pool; forks isolate each test file in its own child process and tolerate `maxWorkers` up to the file count without contention. - `--retry=3` -> `--retry=2`; endpoint flakiness was cut by the ranking work in open-web3-stack/polkadot-ecosystem-tests#616, so the extra retry just inflates the worst-case wall time on a bad day. - `timeout-minutes` 120 -> 60 to match the new envelope.
1 task
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
2 participants
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.
Refactors test suites onto a shared chopsticks client per suite, with snapshot restore between tests.
Closes #607, which further details the reasoning behind these changes.
The refactor depends on two chopsticks-internal changes
Block.resetStorageLayers/storageLayerCountandHeadState.setHeadfix to notify storage subscribers for keys reverted by a backwardsetHeadwithout which snapshot restore leaks state across tests and polkadot.js queries route to the wrong block.
Both are filed alongside the @ggwpez performance changes from #606 in the chopsticks issue at AcalaNetwork/chopsticks#1027. While that issue is open and unreleased, those two changes are delivered to this PR via
yarn patchagainst@acala-network/chopsticks-core@1.3.1(see.yarn/patches/). The patch is removed once chopsticks publishes a release containing the fix.