test(e2e): adopt shared wait helpers - #24404
Merged
Merged
Conversation
Collaborator
Flakey Tests🤖 says: This CI run detected 2 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
spalladino
changed the title
Replace hand-rolled retryUntil/sleep/Promise.all(waitForTx) polls with the shared wait_helpers library (waitForBlockNumber, waitForProvenBlock, waitForNodeCheckpoint, waitForTxs, waitForL2ToL1Witness) and the context waitUntilCheckpointNumber method. Add new behavior-preserving helpers: waitForTxReceipt/waitForTxStatus, waitForPendingTxCount, and waitForSequencerState (with an after-action hook), and delete the duplicated waitForSequencerIdle copies in l2_to_l1/gas_estimation/reload_keystore.
…helper adoption Add RollupCheatCodes.waitForEpoch/waitForSlot and adopt them in the escape-hatch and gov-proposal sequencer tests. Reuse the existing MultiNodeTestContext.findSlotsWithProposers for the invalidate_block and ha_checkpoint_handoff slot searches. Adopt the build-window wait helper in high_tps, extend waitForBlockNumber with a compare option for the prune-backward poll in synching, consolidate the duplicated price-convergence polls in fee_asset_price_oracle, convert the quorum while-loop and the upload-completion poll to retryUntil/promiseWithResolvers.
spalladino
force-pushed
the
spl/e2e-helper-adoption-1
branch
from
June 30, 2026 20:58
479581e to
f0af2ab
Compare
PhilWindle
approved these changes
Jul 1, 2026
spalladino
added a commit
that referenced
this pull request
Jul 1, 2026
## Motivation A-1178 (#24281) measures each e2e test's wall-clock as four coarse buckets (`setupFnMs`, `beforeHooksMs`, `bodyMs`, `afterHooksMs`). That tells us a test is slow but not *where* it loses time. Most e2e tests spend their wall-clock in the same handful of operations — standing up the environment, waiting for a tx to be mined/checkpointed/proven, waiting for a committee to form or an offense to be detected, client-side proving of seed txs, and warp scans hunting for a proposer slot. This decomposes that time into **named spans** so a single aggregate query over a full CI run answers *"across the suite, how much total wall-clock goes into proving / spinning up nodes / waiting for checkpoints?"* — a ranked list of where to invest in speedups. It extends A-1178 and builds on the consolidation (#24201/#24310) and helpers (#24404) work that turned each repeated operation into one shared definition, so wrapping it once instruments every test with no per-test edits. This is **instrumentation / data-gathering only** — no test behavior or timing changes. ## Approach - A generic `testSpan(name, fn)` / `testSpanSync(name, fn)` wrapper (`fixtures/timing.ts`) records `{ owner, name, start, end }` into the shared collector installed by the timing environment. When `TEST_TIMING_FILE` is unset there is no collector, so `testSpan()` calls `fn()` directly — exactly zero-cost, with no clock reads. - At flush, the timing environment groups spans by owner then tag and computes four numbers per `(owner, tag)`: - `count` — multiplicity is itself a signal (waited for a checkpoint 14× points at a loop to batch). - `totalMs` — naive sum (correct for serial repeats). - `busyMs` — duration of the **union** of the spans' intervals; concurrency-correct, so a `Promise.all` of 12 concurrent 3s spans reads ~3s instead of ~36s. The `busyMs ≪ totalMs` signature flags work that is run serially where it could be parallel. - `maxMs` — longest single occurrence, to catch one pathological wait hiding in a cheap average. - The aggregates attach as an additive `spans` map on each `type:"test"` / `type:"suite"` JSONL line. `setupFnMs` / `teardownFnMs` are kept for back-compat, now derived from the `setup:env:<mode>` / `teardown:env` tags. - Tags follow a stable `category:label` taxonomy (`setup:`, `wait:`, `tx:`, `warp:`, `wallet:`, `deploy:`, `other:`), tagged **by concept, not by function** — e.g. every checkpoint waiter maps to `wait:checkpoint` — so a label is a forever aggregation key regardless of which helper a test happened to call. - **One clock.** All spans use `performance.now()`. I verified Node's `perf_hooks` clock is process-wide: a `performance.now()` taken inside a fresh `vm` context (the jest sandbox realm) shares the same monotonic origin as the host realm (delta ~0.002ms; a fresh `perf_hooks.performance` inside a separate context anchors to the same `timeOrigin`). Interval-merging for `busyMs` across the sandbox and host realms is therefore valid, so **`busyMs` is included** (no fallback to `totalMs`-only needed). - **Background loop.** `startMempoolFeeder` runs interleaved with arbitrary tests; without care its prove/send spans would smear onto whichever test was current when each round fired. Its production is run inside an `AsyncLocalStorage`-scoped owner override (`other:mempool-feeder`), which pins every span in its async call tree to a fixed owner — isolated from concurrent test-body spans. That owner matches no test/suite record, so the feeder's spans are cleanly excluded from the per-test view. ## Changes by phase - **Phase 0 — collector + `testSpan()` + flush aggregation.** New `fixtures/timing.ts` (`testSpan`/`testSpanSync`/`withTestSpanOwner`). `timing_env.mjs` collector generalized from `fnSpans` to a `spans` array; `finalizeAndFlush` groups by owner/tag, merges intervals for `busyMs`, attaches the `spans` map, and derives the back-compat fields. - **Phase 1 — shared waits + `setup:node`.** Wrapped the waiters in `fixtures/wait_helpers.ts` (`wait:proposed` / `wait:proven`, `wait:checkpoint` / `wait:proven-checkpoint`, `wait:tx-mined`, `wait:l2-to-l1-witness`, `wait:pending-tx`, `wait:sequencer-state`) and the wait methods on `SingleNodeTestContext` / `MultiNodeTestContext` (`wait:epoch`, `wait:slot`, `wait:proof-window`, `wait:node-sync`, `wait:proof-submitted`, `wait:offense`, the multi-node `wait:checkpoint`/`wait:proven-checkpoint`/`wait:block` convergence waiters). Wrapped `createNode`/`createProverNode` with `setup:node`. - **Phase 2 — decompose `setup:env`.** Cracked the opaque setup in `fixtures/setup.ts` into `setup:env:anvil`, `:l1-deploy`, `:sequencer-start`, `:prover-node`, `:pxe`, and `wallet:create`. The top-level `setup:env` is tagged with the prover mode (`setup:env:none` / `:fake` / `:real`) so the three factories are comparable. - **Phase 3 — tx leaf wraps.** `proveInteraction` → `tx:prove`, `ProvenTx.send` → `tx:send`; everything else (`proveTxs`/`proveAndSendTxs`/the submit helpers) aggregates through those leaves. `startMempoolFeeder` handled as above. - **Phase 4 — slashing/governance waiters.** Wrapped `awaitCommitteeExists` (`wait:committee`), `awaitOffenseDetected` (`wait:offense`), `awaitCommitteeKicked` (`wait:committee-kicked`), `awaitProposalExecution` (`wait:slash-execution`), and the `findUpcomingProposerSlot` / `advanceToEpochBeforeProposer` / `findSlotsWithProposers` scans (`warp:find-proposer`). - **Phase 5 — reporting.** The JSONL schema now carries the per-line `spans` map, and `TEST_TIMING_SPANS=1` emits one `type:"span"` line per occurrence (owner, name, ms) for deep dives. The repo now also ships a `track-e2e-times` skill (`yarn-project/.claude/skills/track-e2e-times/`) covering how to **collect and aggregate** the span timings locally: run the suite with `TEST_TIMING_FILE` set, find the per-worker JSONL, and print per-test sums plus the ranked span leaderboard (via the bundled `row.sh`). Only the step of publishing aggregate numbers to a running tracking log remains a separate out-of-band workflow. The leaderboard rollup is a small additive jq over the new `spans` maps: ``` jq -rs '[ .[] | select(.type=="test" or .type=="suite") | (.spans // {}) | to_entries[] ] | group_by(.key) | map({ tag: .[0].key, count: (map(.value.count) | add), busyMs: (map(.value.busyMs) | add), maxMs: (map(.value.maxMs) | max) }) | sort_by(-.busyMs) | .[] | "\(.busyMs)\t\(.count)\t\(.maxMs)\t\(.tag)"' ``` The existing `row.sh` is unaffected — it filters `type=="test"` and reads the unchanged `setupFnMs`/`bodyMs`/etc., ignoring the new `spans` key and `type:"span"` lines. ## Notes - `waitForEpoch`/`waitForSlot` (added to `@aztec/ethereum`'s `rollup_cheat_codes.ts` by #24404) are deliberately **not** instrumented in-package: that package should not depend on the e2e timing collector, and the `wait:epoch`/`wait:slot` concepts they cover are already captured at the e2e-context wrapper layer (`waitUntilEpochStarts`, the slot waiters). They have no e2e callers yet; a future call site picks them up via its context wrapper. - Spans do not partition `bodyMs` — a parent span includes its children, so `sum(spans) ≠ bodyMs`. Tagging is at the leaf wait/setup/tx level where additivity holds; the few intentional nests (e.g. `wait:committee-kicked` contains `wait:slash-execution`) carry distinct tags. Verified: `yarn build`, `yarn lint end-to-end`, `yarn format --check end-to-end` all pass. The flush aggregation (busyMs interval merge, back-compat derivation, feeder-owner exclusion, opt-in raw lines) is covered by unit tests in `end-to-end/src/shared/timing_env.test.ts`, which exercise the extracted `aggregateSpans` / `foldSpansInto` pure functions directly. Fixes A-1179 ## Example timing output Each e2e test run in CI emits one JSONL record per test and per suite, carrying a `spans` map keyed by `category:label` tag, where each value is `{count, totalMs, busyMs, maxMs}` (`busyMs` is the de-overlapped union duration, so concurrent occurrences of a span are counted once). An illustrative `type:"test"` line from CI run 28469687883 (commit de3635e): ```json { "suite": "optimistic.parallel", "type": "test", "name": "single-node/proving/optimistic happy path proves multiple epochs via checkpoint-driven flow", "status": "passed", "setupFnMs": 4707, "beforeHooksMs": 4717, "bodyMs": 327324, "teardownFnMs": 615, "afterHooksMs": 624, "totalMs": 332668, "startedAt": "2026-06-30T19:20:58.279Z", "spans": { "setup:env:anvil": { "count": 1, "totalMs": 82, "busyMs": 82, "maxMs": 82 }, "setup:env:l1-deploy": { "count": 1, "totalMs": 470, "busyMs": 470, "maxMs": 470 }, "setup:env:sequencer-start": { "count": 1, "totalMs": 3141, "busyMs": 3141, "maxMs": 3141 }, "setup:env:prover-node": { "count": 1, "totalMs": 139, "busyMs": 139, "maxMs": 139 }, "setup:env:pxe": { "count": 1, "totalMs": 391, "busyMs": 391, "maxMs": 391 }, "wallet:create": { "count": 1, "totalMs": 79, "busyMs": 79, "maxMs": 79 }, "setup:env:fake": { "count": 1, "totalMs": 4707, "busyMs": 4707, "maxMs": 4707 }, "wait:epoch": { "count": 5, "totalMs": 180210, "busyMs": 180210, "maxMs": 36067 }, "tx:prove": { "count": 4, "totalMs": 2123, "busyMs": 2123, "maxMs": 621 }, "tx:send": { "count": 4, "totalMs": 144305, "busyMs": 144305, "maxMs": 36124 }, "wait:proven-checkpoint": { "count": 4, "totalMs": 0, "busyMs": 0, "maxMs": 0 }, "wait:node-sync": { "count": 4, "totalMs": 401, "busyMs": 401, "maxMs": 101 }, "teardown:env": { "count": 1, "totalMs": 615, "busyMs": 615, "maxMs": 615 } }, "commit": "de3635e74eb89071b52939d1583d072ba2c3852c", "branch": "spl/a1179-track-common-spans", "runId": "28469687883" } ```
spalladino
added a commit
that referenced
this pull request
Jul 1, 2026
Finishes #24404's cleanup by resolving the 17 deferred // REFACTOR: wait-code markers in the e2e suite. Adopts a shared helper where one fits and extracts single-use polls into named local functions otherwise, preserving behavior. - ChainMonitor.waitForCheckpoint gains an opt-in guard that short-circuits on a fresh run() snapshot, avoiding the event-vs-poll already-satisfied race. - New RollupCheatCodes.waitForCheckpointBelow poll-only rollback waiter. - snapshot_sync adopts ChainMonitor.waitUntilCheckpoint. - FeesTest.waitForEpochProven encapsulates the advance-epoch + catch-up pair.
spalladino
added a commit
that referenced
this pull request
Jul 2, 2026
Finishes #24404's cleanup by resolving the 17 deferred // REFACTOR: wait-code markers in the e2e suite. Adopts a shared helper where one fits and extracts single-use polls into named local functions otherwise, preserving behavior. - ChainMonitor.waitForCheckpoint gains an opt-in guard that short-circuits on a fresh run() snapshot, avoiding the event-vs-poll already-satisfied race. - New RollupCheatCodes.waitForCheckpointBelow poll-only rollback waiter. - snapshot_sync adopts ChainMonitor.waitUntilCheckpoint. - FeesTest.waitForEpochProven encapsulates the advance-epoch + catch-up pair.
spalladino
added a commit
that referenced
this pull request
Jul 2, 2026
## Motivation Cleanup finishing #24404's work. That PR adopted shared wait helpers across the e2e suite but deliberately deferred 17 `// REFACTOR:` wait-code markers as harder or ambiguous cases. This resolves all of them, leaving zero `// REFACTOR:` markers under `end-to-end/src`. ## Approach - Adopt a shared helper where one genuinely fits. - Extract single-use / hard-to-adapt polls into descriptively-named local functions in the same file, then delete the marker. - Delete stale markers whose referenced code no longer exists. All changes are behavior-preserving refactors: comparator direction, timeouts (with seconds-vs-ms conversions where a helper's unit differs), the `retryUntil` falsy-means-keep-polling contract, and side effects (loops that send txs / advance blocks / rotate the oracle keep doing so every tick) are all retained. ## API changes New/changed test-only helpers (in `@aztec/ethereum` test utils and the e2e fees harness): - `ChainMonitor.waitForCheckpoint(match, opts)` gains an opt-in flag. When set, it takes a fresh `run()` snapshot and short-circuits if the current checkpoint already satisfies `match` before attaching the event listener. This closes an event-vs-poll already-satisfied race: a checkpoint could advance during a preceding wait and be missed by the purely event-driven path. The existing caller is unchanged. - New `RollupCheatCodes.waitForCheckpointBelow(checkpoint, opts)` — a poll-only waiter (mirroring `waitForEpoch`/`waitForSlot`) that reads the L1 rollup contract until the pending checkpoint drops below a baseline (rollback detection). No node dependency. - `snapshot_sync` now adopts the existing `ChainMonitor.waitUntilCheckpoint` (which has the already-satisfied guard) instead of a hand-rolled poll. - New `FeesTest.waitForEpochProven()` encapsulates the recurring `advanceToNextEpoch()` + `catchUpProvenChain()` pair (adopted in `failures` and `private_payments`).
rangozd
pushed a commit
to rangozd/aztec-packages
that referenced
this pull request
Aug 5, 2026
BEGIN_COMMIT_OVERRIDE refactor(e2e): relocate no-node straggler tests (AztecProtocol#24344) test(e2e): pin gas_estimation public-payment txs to one block to deflake fee comparison (AztecProtocol#24382) feat(archiver): event-trigger L2BlockStream sync from archiver updates (AztecProtocol#24317) test(e2e): speed up individual e2e tests (AztecProtocol#24345) chore(e2e): warm blob KZGs in parallel during setup (AztecProtocol#24383) chore: add perf as a valid PR title prefix (AztecProtocol#24412) test(e2e): adopt shared wait helpers (AztecProtocol#24404) test(e2e): run prover client.test.ts in CI (AztecProtocol#24399) fix(ethereum): mine empty L1 blocks without touching the mempool (AztecProtocol#24414) test(e2e): deflake empty block proving test (AztecProtocol#24411) test(e2e): allocate HA node p2p ports above the ephemeral range to deflake e2e_ha_full (AztecProtocol#24418) fix(sequencer): use evmMine in automine auto-settle to avoid dropping test L1 txs (AztecProtocol#24421) test(e2e): instrument common spans for wall-clock tracking (AztecProtocol#24407) test(e2e): remove redundant reqresp_no_handshake e2e test (AztecProtocol#24424) END_COMMIT_OVERRIDE
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
3 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.
Motivation
The e2e suite is littered with hand-rolled
retryUntil/sleep/Promise.all(...waitForTx)polls, each flagged with a// REFACTOR:marker. Most duplicate logic that already exists in the sharedfixtures/wait_helpers.tslibrary or on the test contexts. This PR resolves those markers by adopting the shared helpers (and adding a handful of small new ones), removing the duplicated, brittle wait code.Stacked on top of #24345 (
spl/e2e-speed-up-1), which shipped the warp/build-window helpers this PR adopts.Approach
Every change is behavior-preserving: the original loop's timeout, polling interval, and comparator/predicate are matched exactly. The mechanism is swapped, not what the test waits for or asserts. Where an existing helper didn't quite match the original semantics, the helper was extended with an option rather than the test's behavior being changed.
wait_helpers.tsor onSingleNodeTestContext/MultiNodeTestContext.wait_helpers.tsandRollupCheatCodes, each behavior-preserving and adopted in the same change.33 of 50
// REFACTOR:markers are resolved. The remaining 17 are deliberately deferred (see Follow-ups) - they are single-use, read from a data source no shared helper covers, or are the riskiest swaps that warrant their own focused change.Changes (by helper-family)
Adoption of existing
wait_helpers.tsfunctionswaitForBlockNumber/waitForProvenBlock:block_building,debug_trace,genesis_timestamp,l1_to_l2,fee_settings.waitForNodeCheckpoint:genesis_timestamp,fee_settings.waitForTxs:block_building(3 sites),multiple_blobs,gossip_network.waitForL2ToL1Witness:token_bridge_public,token_bridge_private.MultiNodeTestContext.waitUntilCheckpointNumber:proof_boundary.waitForP2PMeshConnectivity(replaces a fixedsleep(8000)):fee_asset_price_oracle_gossip.waitForBuildWindowForSlot:high_tps.EpochNotStablewarp to existingMultiNodeTestContext.findSlotsWithProposers:invalidate_block,ha_checkpoint_handoff.New helpers (added + adopted)
wait_helpers.ts:waitForTxReceipt/waitForTxStatus(tx-status transitions inblock_building),waitForPendingTxCount(attested_invalid_proposal),waitForSequencerStatewith anafter-action hook - and deleted the three duplicatedwaitForSequencerIdlecopies inl2_to_l1,gas_estimation, andreload_keystore.waitForBlockNumberextended with acompareoption for the prune-backward poll insynching.RollupCheatCodes.waitForEpoch/waitForSlot(poll-only, no warp), adopted inescape_hatch_vote_onlyandgov_proposal.Inline cleanups (no shared helper warranted)
fee_asset_price_oracle: two identical price-convergence polls consolidated into one local helper.upgrade_governance_proposer:while(true) + sleep(12000)quorum poll toretryUntil.upload_failed_proof: local-variable upload poll topromiseWithResolversresolved from the upload callback.synching: archiver-prunesleep(3000)toretryUntilon the archiver checkpoint number.Follow-ups (deferred markers)
These were left in place because adopting a helper would either change behavior or add a single-use abstraction:
proof_fails:109- rollback detection reads the L1 rollup contract (RollupContract.getCheckpointNumber), not the node; swapping to a node helper would change the data source. Needs a rollup-contract-based wait.snapshot_sync:89- readsChainMonitor.checkpointNumber(not the node) with>semantics; the test uses the legacyEndToEndContext, which has nowaitUntilCheckpointNumber.long_proving_time:74- the loop records peak proving-job parallelism on every iteration; that sampling is the behavior under test, so it can't be replaced by a plain wait.preferred_gossip_network:183- per-node exact peer counts in a heterogeneous topology; doesn't map ontowaitForP2PMeshConnectivity's uniform>=check.gov_proposal:245- ChainMonitor poll that returns a full snapshot (checkpoint + slot) consumed downstream; no helper returns that.failures:100- a two-calladvanceToNextEpoch+catchUpProvenChainsequence, not a hand-rolled poll.pruned_blocks:117,offchain_payment:215,snapshot_sync:100,l1_to_l2:130) - single-use, heterogeneous polls (error-message match, note-balance, readdir, advance-block-per-poll) that already useretryUntilrather than a sleep.state_vars:439- a chain-advance-by-sending-txs loop (side-effecting), not a poll.optimistic:115- a disposable sampler; single call-site.rediscovery:61- asleep(2500)guarding against port conflicts between node restarts, not connectivity.bridging_race.notest.ts:70/77- in a disabled.notest.tsfile.fee_settings:121- the L1-base-fee-spike + oracle-rotation loop returns a value and is fee-domain specific; warrants its own focusedRollupCheatCodeschange.Verification
yarn build,yarn lint, andyarn formatall pass on the touched packages (end-to-end,ethereum). The e2e tests themselves are validated by CI on this stacked PR.