test(integration): drive the integration specs against the live stack - #717
Open
0xisk wants to merge 13 commits into
Open
test(integration): drive the integration specs against the live stack#7170xisk wants to merge 13 commits into
0xisk wants to merge 13 commits into
Conversation
The `compile` task declared only `dependsOn`, so turbo fell back to hashing every tracked file in the contracts package. Editing a spec or a vitest config was therefore a cache miss on a task that recompiles all 70 contracts with real keygen, which defeats the intent that TS-only edits skip recompilation. The six per-category tasks were cache hits while the top-level task rebuilt everything anyway. Declare what the script actually reads, mirroring the per-category tasks. Measured with `turbo run compile --dry=json`, the resolved input set drops from 214 files to 71, and excludes `src/archive` to match the script's own `--exclude`. Also declare MIDNIGHT_LIVE_KEEP_ENV as a global passthrough: the live orchestrator reads it to skip stack teardown, the same way it already reads GITHUB_ACTIONS and GITHUB_STEP_SUMMARY. Note that declaring SKIP_ZK means `compile` now honours an ambient value where strict env mode previously discarded it. The orchestrator clears it for both compiles so a live run cannot be handed keyless artifacts.
`emptyKeyArtifacts` took a single optional source root and skipped any artifact directory with no matching `.compact` under it, so stale orphan directories could not false-positive. That filter also skipped anything compiled from outside `src`. The integration mocks compile into the same `artifacts/` tree but live under `test/integration/_mocks`, so a `src`-only scan never looked at them. A truncated key in a mock would therefore go undetected, the live deploy would fail in `beforeAll`, and vitest would turn that into a silent whole-suite skip: exactly the failure this check exists to prevent. Take variadic source roots and union the contract names across them. One call site, one pass over `artifacts/`, and the stale-orphan filter still works. The standalone CLI now passes both roots too.
Every live project derives its wallets from `walletSeedsFor(VITEST_POOL_ID)`, so worker 1 of `unit-live` and worker 1 of `integration-live` resolve to the same genesis deployer seed. Two pools would balance transactions against one deployer's UTXO snapshot and reintroduce the stale-UTXO race that `WalletPool.ensureReady`'s serial build exists to prevent, and both would write `logs/live-harness-w1.log`. The run lock cannot catch this. It is deliberately reentrant for our own pid, precisely so one process can run several live globalSetups. So claim the process separately. The guard lives in globalSetup rather than in `live.setup`: vitest gives each project its own worker processes, so a per-worker guard never sees the other project, while globalSetup runs once per project in the single main process. The claim is parked on `globalThis` because each project loads the file through its own module runner. It runs before the lock and before the freshness scan, so a mis-invocation fails in milliseconds without touching the node. `assertSoleLiveProject` is exported as a pure function, matching the convention already used for `lockHolderState` and `countCoinEvents`.
`test-live.ts` had grown to 478 lines carrying eleven responsibilities: paths, process spawning, console output, category discovery, target resolution, vitest invocation, report parsing, the run lock, compile verification, verdict reporting, and the two-round loop. Split one service per concern under `scripts/live/`, moving each rationale comment to sit with the code it explains. The entry point is now a 99-line main that resolves a plan, wires the services, and runs under the lock. Behaviour is preserved: every console string, all exit codes, and the order of operations are unchanged. Two behavioural additions come with it. The orchestrator now owns the whole stack lifecycle. `env-up` was already automatic and itself resets, so a leaked stack never corrupted the next run, but containers and the `docker compose logs -f` streamers each bring-up backgrounds outlived the process. Teardown runs on every exit path, before the lock release so no other run can start against a half-stopped stack, and from a signal handler because Node runs no `finally` on a signal. MIDNIGHT_LIVE_KEEP_ENV=1 opts out. `run` is asynchronous. With spawnSync the event loop was blocked for a child's whole lifetime, and a compile phase is one long synchronous chain, so a queued SIGINT handler could not run until the phase finished. Ctrl-C during a compile was ignored, the interrupted compile looked successful because turbo exits 0 on its own graceful shutdown, and the truncated keys the interruption had just created were then read as a poisoned cache: draining it and starting a pointless serial recompile. Awaiting the child keeps the loop live. Teardown stays synchronous, since the handler exits as soon as it returns. The integration target is added as a first-class target here: it routes to `--project integration-live`, compiles the mocks with real keys, and is emitted by `--list` so CI can spawn a job for it. It is matched before the category branch because it is not a `src/` directory and would otherwise fall through to the unscoped path, silently running every live-ready unit category instead.
A skipped test never runs `beforeEach`, so no worker stamps its metadata and every skipped line printed `[w?]`. Fall back to the last worker seen for that test's module: the module was loaded and run by that worker even where an individual test was skipped. `?` now means only that nothing in the module ever reported a worker.
The compiler names each artifact directory after the source basename, and `ConfidentialFungibleTokenPublicSupply.compact` existed both under `src/token/extensions` and under `test/integration/_mocks`. Two different contracts wrote one `artifacts/<name>/` directory, so whichever compiled last won. That is not theoretical. A background `yarn types` pulled in a src compile, replaced the directory, and every dry functional test then failed with "Contract state constructor: expected 1 argument, received 4". Nothing else catches it: the wrong artifact's keys are well-formed, just wrong, so the truncated-key scan cannot see it, and it survives lint and typecheck to surface at runtime as a confusing wrong-contract arity error. Prefixing the mock gives the two contracts separate directories and removes the whole class rather than ordering around it. The mock's own header records why the prefix exists so a future rename does not reintroduce it.
Let the existing live backend drive the `integration` project the same way it already drives `unit-live`. The backend is already backend-generic, so this is additive wiring rather than new harness logic: the new project reuses the same globalSetup (freshness, run lock) and setup (wallet pool, backend register), with its own scheduler group because `unit-live` holds group 1 with a different `maxWorkers`, which vitest rejects. One glob feeds both projects. Which blocks run under each backend is decided at runtime by `skipIf`/`runIf(isLiveBackend())` in the spec, so a spec's live/dry split lives next to the code it guards rather than in a naming convention or a central allowlist. The deliverable is the capability plus a block-limit canary, not green functional live coverage. The composed contract cannot deploy live: the base token already bundles four k=16 circuits' IR into one deploy tx and overruns the per-tx block byte budget, and this composition is strictly larger. Rather than skip and hide that, the canary asserts the rejection, so an unexpected success fails loudly the day a staged deploy or a smaller composition lets it through. It asserts the node's exact wording, since a loose match could be satisfied by an unrelated deploy failure and report a false green. The functional suite stays dry on two counts: the deploy above is rejected, and every flow mutates private state mid-test via `switchIdentity`/`cachePlaintext`, which the live backend throws on. `initStateIsolation` is dry-only by nature, constructing simulators directly and never deploying. `compile:integration` drops its hardcoded SKIP_ZK so one script serves both the dry and the full-key path, cached separately by turbo. The dry path is unchanged because the root `test:integration` still exports it. Each live project also publishes its own worker total, so the per-worker banner reports that project's `maxWorkers` instead of `unit-live`'s.
The orchestrator had no test coverage at all. Add a `scripts` vitest project and cover the two paths where a regression would be silent rather than loud. Target resolution is the first: ask for `integration` and a broken guard would run every live-ready unit category instead, with nothing in the output to say so. The cases pin that guard, including the one where the target is deliberately absent from the category list. Flake classification is the second, and the higher-consequence one: a regression there flips a real failure to FLAKY and the build goes green over a genuine bug. `classify` is extracted as a pure function so it can be tested without a stack. Report naming is covered too, since round 2 cannot classify an integration file if its `.spec.ts` extension is not stripped. The spawn wrappers are deliberately not tested. Asserting the arguments we pass to spawnSync restates the implementation and locks in call shapes that should stay free to change.
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR modularizes live integration testing with target planning, verified artifact compilation, stack and lock management, two-round retries, flaky-test reporting, and dedicated Vitest projects. Integration fixtures and suites now use the composed artifact with backend-specific execution rules. ChangesLive verification workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
scripts/live/test/live.test.ts (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering
listTargetshere too.It's pure and is the single source of truth for the CI matrix, so a regression (e.g. dropping
integration) would only surface as a silently missing CI job.🧪 Suggested addition
describe('listTargets', () => { it('lists live-ready categories plus the integration target', () => { expect(listTargets(CATEGORIES)).toStrictEqual(['multisig', 'integration']); }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/live/test/live.test.ts` at line 15, Add coverage for the pure listTargets function in the live test suite, asserting that listTargets(CATEGORIES) returns the expected live-ready category and integration target in order. Import or reuse the existing listTargets symbol without changing CATEGORIES or its implementation.scripts/live/targets.ts (1)
88-128: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnscoped runs silently swallow a mistyped category as a file filter.
resolvePlan([], …)andresolvePlan(['multsig'], …)differ only in that the typo becomes a positional vitest filter across every live-ready target — which, with--passWithNoTestsinVitestRunner.run, exits 0 having run nothing. Consider warning when the first arg looks like a target name (e.g. matches no file filter semantics / is a near-miss of a known category) so a typo doesn't produce a green run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/live/targets.ts` around lines 88 - 128, Update resolvePlan to detect unscoped first arguments that resemble a category or near-match a known target, rather than treating them as fileFilters across all live-ready targets. Return a failed PlanResolution with a clear invalid-target message for such typos, while preserving valid file-filter arguments and existing scoped, integration, and empty-argument behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/package.json`:
- Line 35: Update the compile:integration script in package.json to set
SKIP_ZK=true when invoking compact-compiler, preserving the existing integration
mock source while ensuring Turbo uses the intended dry-integration cache key.
In `@scripts/live/RunLock.ts`:
- Around line 55-63: Update the stale-lock branch in the lock acquisition method
around `#read` and writeFileSync: remove the stale lock before retrying creation,
then recreate it using exclusive wx semantics so concurrent reclaimers cannot
both succeed. Preserve the existing live-PID error behavior and ensure a failed
exclusive retry does not leave the caller believing it owns the lock.
In `@scripts/live/VitestRunner.ts`:
- Around line 60-66: Update fileStatuses to guard JSON.parse and report reading
so malformed or partially written reports return undefined, matching the
documented missing-report behavior. Preserve the existing Map construction for
valid JsonReport data so LiveOrchestrator.#round1 and `#round2` can handle
failures through their graceful INFRA_ABORT path.
In `@scripts/test-live.ts`:
- Around line 94-98: Update the promise completion and rejection handlers in
main() to assign the resulting status to process.exitCode instead of calling
process.exit(), while preserving the existing error message and INFRA_ABORT
fallback so buffered stdout can drain before termination.
---
Nitpick comments:
In `@scripts/live/targets.ts`:
- Around line 88-128: Update resolvePlan to detect unscoped first arguments that
resemble a category or near-match a known target, rather than treating them as
fileFilters across all live-ready targets. Return a failed PlanResolution with a
clear invalid-target message for such typos, while preserving valid file-filter
arguments and existing scoped, integration, and empty-argument behavior.
In `@scripts/live/test/live.test.ts`:
- Line 15: Add coverage for the pure listTargets function in the live test
suite, asserting that listTargets(CATEGORIES) returns the expected live-ready
category and integration target in order. Import or reuse the existing
listTargets symbol without changing CATEGORIES or its implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36195c9e-24ba-4403-931d-d95875d5ffe7
📒 Files selected for processing (22)
contracts/package.jsoncontracts/test-utils/harness/live.globalSetup.tscontracts/test-utils/harness/liveProgressReporter.tscontracts/test/integration/_mocks/ComposedConfidentialFungibleTokenPublicSupply.compactcontracts/test/integration/fixtures/confidentialFungibleTokenPublicSupply.tscontracts/test/integration/specs/confidentialFungibleToken.spec.tscontracts/test/integration/specs/initStateIsolation.spec.tscontracts/vitest.config.tspackage.jsonscripts/keyIntegrity.tsscripts/live/ArtifactCompiler.tsscripts/live/LiveOrchestrator.tsscripts/live/LiveStack.tsscripts/live/Reporter.tsscripts/live/RunLock.tsscripts/live/VitestRunner.tsscripts/live/paths.tsscripts/live/shell.tsscripts/live/targets.tsscripts/live/test/live.test.tsscripts/test-live.tsturbo.json
Reclaiming a lock left behind by a killed run overwrote it with a plain write, so two runs that both found the same dead pid could both "win" it. The last writer owned the file while the other ran on believing it held the lock — its `release()` sees a foreign pid and silently no-ops — and two orchestrators then drove the one shared node, which is exactly what the lock exists to prevent. Every contended step is now a single atomic filesystem call. A stale lock is claimed by renaming it aside (POSIX moves the inode once, so the second reclaimer gets ENOENT), then re-created under `wx`, so a third run that started in between still wins or loses cleanly. Both losing paths report the same "already in progress" rejection, unchanged in wording for the live-pid case. Adds the lock's first tests: fresh acquire, live-pid rejection, stale reclaim, no leftover stale copy, and release only unlinking a lock this run owns. Refs #717
`fileStatuses` documents `undefined` for a run that produced no result, and both rounds rely on it to abort gracefully with INFRA_ABORT. But a killed vitest can leave a partially-written report that still passes `existsSync`, and `JSON.parse` then threw straight through the callers, crashing the orchestrator instead — after the stack was already up, and with no verdict. Parsing now falls into the same no-result path and names the unreadable file, since the caller's "produced no results file" message would otherwise misdescribe it. Adds the first tests for the report reader: statuses mapped, the `--passWithNoTests` empty report, a missing report, and a truncated one. Refs #717
The orchestrator ended on `process.exit(code)`. Under CI its stdout is a pipe (`tee`, a log collector), where writes are asynchronous and `process.exit` drops whatever is still queued — and what a run prints last is the verdict block, so that is exactly what a piped run could lose. Setting `process.exitCode` instead lets the queued writes flush. Nothing holds the loop open once `main` resolves: every child is awaited to `close`, and Node unrefs signal listeners. The handler in `shell.ts` keeps `process.exit`, where leaving immediately after synchronous cleanup is the point. Verified piped: `--list` still exits 0 and a non-live-ready category still exits 2, both with their output intact. Refs #717
`resolvePlan` cannot tell a mistyped target from a file filter, so `yarn test:live multsig` becomes a positional filter across every live-ready target. With `--passWithNoTests` vitest then exits 0 and writes a report with no results, and the run finished as PASSED having executed nothing — the one verdict a live run must never produce by accident. Round 1 now counts the files each target reported and aborts with INFRA_ABORT when the whole run matched none, naming the filter and pointing at `--list`. A single target contributing zero files is still a pass: in an unscoped run a name filter may only exist under some targets, which is why `--passWithNoTests` is there. Prefers this over near-miss detection in `resolvePlan`, which would have to guess which unrecognised arguments are typos, and would still miss a mistyped file filter. Also adds the orchestrator's first round tests, over stand-in collaborators: this abort, the no-report abort, and the green path. Refs #717
`listTargets` is the single source of truth for the CI matrix, so dropping an entry would surface only as a live target nobody runs — a silently missing job rather than a failure. It was the one pure function in `targets.ts` with no test. Covers the live-ready categories plus `integration`, and that `integration` survives an empty category list, since it is not a `src/` category and so does not depend on LIVE_READY. Refs #717
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.
Types of changes
What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an
xin the boxes that applyFixes #718
Parent: #665 (live-backend test harness umbrella). Sibling of the per-category
children #668–#672 and #693, for the
test/integration/specstree.Refs #716, which tracks the follow-ups deliberately left out.
Lets the existing live backend drive the
integrationvitest project, the same wayit already drives
unit-live. The backend is already backend-generic, so this isadditive wiring rather than new harness logic.
The deliverable is the capability plus a block-limit canary, not green functional
live coverage. The composed contract cannot deploy live: the base token already
bundles four k=16 circuits' IR into one deploy tx and overruns the per-tx block byte
budget, and this composition is strictly larger. Rather than skip and hide that, the
canary asserts the rejection, so it fails loudly the day a staged deploy or a smaller
composition lets it through.
What's here
integration-livevitest project +test:live integrationtargetunit-live's globalSetup and setup verbatimconfidentialFungibleToken.spec.tsscripts/live/— orchestrator split into 8 servicestest-live.tshad reached 478 lines and eleven responsibilitiescompileturbo inputs scoped.tsedit forced a full 70-contract keygenscriptsvitest project + testsDry behaviour is unchanged:
--project integrationproduces the same pass set asmain, plus the canary skipped.Verified
unit(CI-gated)harnessscripts(new)integration(dry)integration-liveVERDICT: PASSEDLive-verified on a local stack: the canary, the exact node rejection, the
one-live-project guard, teardown on both the normal and signal paths, and the mock
rename resolving through
artifactNamefor a real deploy.The canary asserts the node's exact wording, confirmed on-stack:
A loose
/block limits/match could be satisfied by an unrelated deploy failure(funding, proving, a submission bounce) and report a false green, which would
silently void the scope claim the canary exists to prove.
PR Checklist
Further comments
The
integration-livejob will not run in CI until #681 merges. That PR adds theworkflow which reads
test:live --list; this branch only makes--listemitintegration. So green CI here has not exercised the live path. It was verifiedlocally instead.
Two fixes here predate this branch and are self-contained commits, splittable if
you'd rather keep the PR narrow:
02533780— thecompileturbo task declared onlydependsOn, so turbo hashedevery tracked file in the package. The six per-category tasks were cache hits while
the top-level task rebuilt all 70 contracts anyway. Resolved input set drops from
214 files to 71, measured with
--dry=json.f453342d—ConfidentialFungibleTokenPublicSupply.compactexisted under bothsrc/token/extensionsandtest/integration/_mocks, and the compiler names eachartifact directory after the source basename. It surfaced as a wrong-contract arity
error, invisible to lint, typecheck, and the truncated-key scan.
Why the orchestrator was split rather than extended. Adding the integration target
to a 478-line file carrying eleven concerns would have made it worse. The split is
behaviour-preserving: every console string, all exit codes, and the order of
operations are unchanged. It also made the two silent-failure paths testable, which is
where the new tests went. The spawn wrappers are deliberately untested, since
asserting the arguments passed to
spawnSyncrestates the implementation.One interrupt bug worth flagging. Ctrl-C during a compile used to be ignored:
spawnSyncblocked the event loop for a whole compile phase, so the SIGINT handlercould not run, the interrupted compile looked successful because turbo exits 0 on its
own graceful shutdown, and the truncated keys the interruption had just created were
then read as a poisoned cache — draining it and starting a pointless serial recompile.
runis now asynchronous, so the handler pre-empts that.Known limitation.
unit-livewas not re-run on this branch. Two changes touch it(the per-project worker total, and the reporter's worker tag for skipped tests), and
yarn test:live multisigis a long run. Worth one before merge if you want that pathcovered.