integration-test-env: add start (brings up the stack only, no tests)#2091
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRefactors the integration-test orchestrator into an exported lifecycle module with bringUp/runIntegrationTests/cleanup APIs, adds CI ( ChangesLifecycle Refactoring and Entrypoint Split
Sequence DiagramsequenceDiagram
participant CI as CI Entrypoint (ci.ts)
participant Lifecycle as lifecycle.ts
participant Services as Services (DB/Devnet/ENSRainbow/ENSIndexer/ENSApi)
participant Tests as pnpm test:integration
participant Cleanup as cleanup()
CI->>Lifecycle: bringUp()
Lifecycle->>Services: start phases (DB/devnet seed, ENSRainbow, ENSIndexer, ENSApi)
Lifecycle->>Tests: runIntegrationTests() (ENSNODE_URL=endpoints.ensapi)
Tests->>Cleanup: signal completion
CI->>Cleanup: cleanup()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors the integration test environment orchestrator into a shared lifecycle module so the stack bring-up (including devnet seeding) can be reused by both CI and a new manual “bring up and wait” workflow.
Changes:
- Extracts stack bring-up/cleanup/test execution logic into
src/lifecycle.tsshared by both entrypoints. - Adds a new
startentrypoint that brings up the full stack and blocks until Ctrl+C, and renames the prior behavior tostart:ci. - Updates root
test:integration:cito call the newstart:ciscript to keep CI behavior consistent.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/integration-test-env/src/start.ts | New manual entrypoint to bring up the stack and wait (no tests). |
| packages/integration-test-env/src/orchestrator.ts | Simplified CI entrypoint now delegating to lifecycle.ts. |
| packages/integration-test-env/src/lifecycle.ts | New shared bring-up/cleanup/signal-handling and integration test runner. |
| packages/integration-test-env/README.md | Documents the split between manual start and CI start:ci flows. |
| packages/integration-test-env/package.json | Renames start behavior and adds start:ci script. |
| package.json | Updates test:integration:ci to call start:ci. |
Comments suppressed due to low confidence (2)
packages/integration-test-env/src/lifecycle.ts:245
- The JSDoc for bringUp() says it "runs cleanup() and rethrows" on failure, but bringUp() doesn't wrap its phases in a try/catch/finally and doesn't call cleanup() itself (callers do). Please update the comment to match the actual behavior, or add error-handling inside bringUp() if that's the intended contract.
packages/integration-test-env/src/lifecycle.ts:117 - handleShutdown() always exits with code 1 after SIGINT/SIGTERM. For the new manual
pnpm startflow (Ctrl+C as a normal teardown), this will report failure to the shell/CI wrappers. Consider exiting 0 for SIGINT (or when CI is not set), and reserve exit code 1 for actual failures/forced termination.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThis PR splits the monolithic
Confidence Score: 5/5This is a mechanical extraction and renaming with no new runtime logic — the CI path through ci.ts is functionally identical to the old orchestrator.ts main(). The bring-up phases, cleanup ordering, abort-flag pattern, and health-check logic are all unchanged from the original orchestrator. The new start.ts correctly delegates to the same lifecycle module. The only substantive change — the signal handler exit code moving from 1 to 0 — was already flagged in a prior review cycle. No new correctness issues were found. lifecycle.ts is the highest-leverage file; a regression there would affect both entrypoints. The signal-handler exit-code behaviour already has an open comment from the previous review round. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([pnpm start]) --> S[start.ts]
B([pnpm start:ci]) --> C[ci.ts]
S --> parseArgs[parseCliArgs\n--only flag]
parseArgs --> bringUp
C --> bringUp
subgraph lifecycle.ts
bringUp{bringUp\nonly?}
bringUp -->|should devnet| D1[Start ENSDb + Devnet\ndocker-compose]
D1 --> D2[Seed devnet]
bringUp -->|should ensrainbow| D3[Start ENSRainbow\npnpm entrypoint]
D3 --> D4[waitForHealth /ready]
bringUp -->|should ensindexer| D5[Start ENSIndexer\npnpm start]
D5 --> D6[waitForHealth /health]
D6 --> D7[pollIndexingStatus]
bringUp -->|should ensapi| D8[Start ENSApi\npnpm start]
D8 --> D9[waitForHealth /health]
end
bringUp --> S2[Block on\nnever-resolving Promise]
bringUp --> C2[runIntegrationTests\nexecaSync pnpm test:integration]
C2 --> C3[cleanup]
C3 --> exit0([exit 0])
SIG([SIGINT / SIGTERM]) --> handler[handleShutdown\nprocess.exit 0]
handler --> cleanup2[cleanup]
S2 -->|Ctrl+C| SIG
Reviews (5): Last reviewed commit: "fix: bot notes (README ports + --only do..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Splits the integration-test-env orchestrator into a reusable bring-up phase (`lifecycle.ts`) shared by two entrypoints: - `pnpm start` — bring up the full stack (incl. seed) and block until Ctrl+C, so `pnpm test:integration` can run from another terminal. - `pnpm start:ci` — renamed from `start`; bring up + run tests + tear down. CI flow is unchanged via the `test:integration:ci` root alias. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The file no longer orchestrates — lifecycle.ts does. ci.ts is now the CI entrypoint that wires bringUp + runIntegrationTests + cleanup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
shrugs
force-pushed
the
fix/integration-test-env-up-down
branch
from
b7eb383 to
8f0fd1f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
packages/integration-test-env/src/lifecycle.ts:140
- Because lifecycle.ts is shared by start:ci, this handler now exits 0 for SIGTERM/SIGINT during the CI test flow too. A termination before the integration tests finish (for example a timeout or service-manager SIGTERM) can therefore be reported as a successful
test:integration:cirun; keep the zero exit behavior scoped to the manual start entrypoint or distinguish SIGINT from CI termination.
packages/integration-test-env/src/lifecycle.ts:275 - This exported function's documentation says it runs cleanup on failure, but the implementation does not catch errors or call cleanup; cleanup currently happens only in the entrypoint catch blocks. This mismatch can mislead future callers into leaking started containers/processes if they rely on the documented contract.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/integration-test-env/src/lifecycle.ts (1)
134-144:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't report every
SIGTERMshutdown as success.
SIGTERMis also what CI runners and supervisors use for cancellation/timeouts. Exiting0here can make an abortedstart:cirun look green. If you want local Ctrl+C to be clean, keepSIGINTat0but preserve a non-zero or signal-derived exit forSIGTERM.Suggested fix
-async function handleShutdown() { +async function handleShutdown(signal: NodeJS.Signals) { if (cleanupInProgress) return; cleanupInProgress = true; log("Shutting down..."); await cleanup(); - // SIGINT/SIGTERM is a user-initiated shutdown, not an error — exit 0. - process.exit(0); + process.exit(signal === "SIGINT" ? 0 : 128 + 15); } -process.on("SIGINT", handleShutdown); -process.on("SIGTERM", handleShutdown); +process.on("SIGINT", () => void handleShutdown("SIGINT")); +process.on("SIGTERM", () => void handleShutdown("SIGTERM"));🤖 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 `@packages/integration-test-env/src/lifecycle.ts` around lines 134 - 144, The current handleShutdown always calls process.exit(0) which hides CI/supervisor cancellations; change the shutdown wiring so SIGINT still exits 0 but SIGTERM yields a non-zero/signal-derived exit code: update handleShutdown to accept an optional signal name (e.g., function handleShutdown(signal?: string)) and after await cleanup() call process.exit(0) when signal === "SIGINT" but process.exit(1) or process.exit(128 + <signalNumber>) for other signals (e.g., "SIGTERM"); register handlers with process.on("SIGINT", () => handleShutdown("SIGINT")) and process.on("SIGTERM", () => handleShutdown("SIGTERM")) while preserving cleanupInProgress and the cleanup() call.
♻️ Duplicate comments (2)
packages/integration-test-env/README.md (1)
39-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the incorrect port list.
This line still lists incorrect ports. Port 8000 does not exist in the stack, and PostgreSQL port 5433 is missing. The correct required ports are: 8545, 3223, 42069, 4334, 5433.
📝 Proposed fix
-Brings up the full stack and blocks until Ctrl+C. The required ports must be available (8545, 8000, 3223, 42069, 4334). Once it's up, run integration tests from another terminal: +Brings up the full stack and blocks until Ctrl+C. The required ports must be available (8545, 3223, 42069, 4334, 5433). Once it's up, run integration tests from another terminal:🤖 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 `@packages/integration-test-env/README.md` at line 39, Update the port list in the README line that starts "Brings up the full stack and blocks until Ctrl+C." to show the correct required ports: 8545, 3223, 42069, 4334, 5433 (remove 8000 and add 5433). Edit that sentence in packages/integration-test-env/README.md so the parentheses contain the corrected port set.packages/integration-test-env/src/lifecycle.ts (1)
275-383:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
bringUp()still leaks partial startup on failure.After phase 1/3/4 acquires resources, any later throw here rejects without tearing them down. That leaves detached processes and compose services running on startup errors, and Line 275 currently promises the opposite. Wrap the phase body in
try/catch, callcleanup(), then rethrow so the exported lifecycle contract matches its behavior.🤖 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 `@packages/integration-test-env/src/lifecycle.ts` around lines 275 - 383, bringUp() can leak started resources if a later phase throws; wrap the main phase sequence inside a try/catch in the bringUp function, and in the catch call await cleanup() (which will stop composeEnvironment and spawned services like those started via spawnService and any started in phases "devnet"/"ensrainbow"/"ensindexer"/"ensapi"), then rethrow the original error so the function's contract holds; ensure you reference and use the existing cleanup() helper and do not swallow the error when rethrowing.
🤖 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 `@packages/integration-test-env/package.json`:
- Line 10: The package.json "start:ci" script exports CI=1 but that env var is
never used; either remove "CI=1" from the "start:ci" entry in package.json, or
update the code to consume it (e.g., check process.env.CI in src/ci.ts or
src/lifecycle.ts and gate CI-specific behavior such as tighter timeouts, quieter
logging, or alternate resource allocation). Ensure any new behavior is clearly
named and documented in the script and that process.env.CI is parsed as a
boolean where used.
In `@packages/integration-test-env/README.md`:
- Around line 35-43: Add documentation for the new --only flag in the section
that explains how to run the stack with pnpm start: state that you can pass
--only with a comma-separated list of services (valid values: devnet,
ensrainbow, ensindexer, ensapi), show a brief usage example like pnpm start
--only devnet,ensrainbow, and mention that this starts only the listed services
instead of the full stack so ports for the omitted services are not required.
In `@packages/integration-test-env/src/ci.ts`:
- Line 24: runIntegrationTests() is being invoked without awaiting, causing
cleanup() and process.exit(0) to run before tests finish; change the invocation
to await runIntegrationTests() and ensure it's inside an async context (or use
.then/.catch) so the process only calls cleanup() and process.exit(0) after
runIntegrationTests() resolves; refer to runIntegrationTests(), cleanup(), and
process.exit to locate and update the call site and surrounding control flow
(use try/finally or promise chaining to guarantee cleanup runs after the awaited
test run).
In `@packages/integration-test-env/src/lifecycle.ts`:
- Around line 59-77: Add zod to packages/integration-test-env package.json
dependencies, then replace the manual validation in parseOnly with a zod schema:
create a Zod schema that accepts a comma-separated string,
splits/trims/filter(Boolean) and validates each item is one of the allowed
values from ALL_SERVICES, then call zod.parse(...) to validate the incoming
value; use the parsed array to construct and return a Set<Service> (removing the
unchecked "as Service[]" cast) and ensure any validation errors bubble up as zod
errors from parseOnly.
---
Outside diff comments:
In `@packages/integration-test-env/src/lifecycle.ts`:
- Around line 134-144: The current handleShutdown always calls process.exit(0)
which hides CI/supervisor cancellations; change the shutdown wiring so SIGINT
still exits 0 but SIGTERM yields a non-zero/signal-derived exit code: update
handleShutdown to accept an optional signal name (e.g., function
handleShutdown(signal?: string)) and after await cleanup() call process.exit(0)
when signal === "SIGINT" but process.exit(1) or process.exit(128 +
<signalNumber>) for other signals (e.g., "SIGTERM"); register handlers with
process.on("SIGINT", () => handleShutdown("SIGINT")) and process.on("SIGTERM",
() => handleShutdown("SIGTERM")) while preserving cleanupInProgress and the
cleanup() call.
---
Duplicate comments:
In `@packages/integration-test-env/README.md`:
- Line 39: Update the port list in the README line that starts "Brings up the
full stack and blocks until Ctrl+C." to show the correct required ports: 8545,
3223, 42069, 4334, 5433 (remove 8000 and add 5433). Edit that sentence in
packages/integration-test-env/README.md so the parentheses contain the corrected
port set.
In `@packages/integration-test-env/src/lifecycle.ts`:
- Around line 275-383: bringUp() can leak started resources if a later phase
throws; wrap the main phase sequence inside a try/catch in the bringUp function,
and in the catch call await cleanup() (which will stop composeEnvironment and
spawned services like those started via spawnService and any started in phases
"devnet"/"ensrainbow"/"ensindexer"/"ensapi"), then rethrow the original error so
the function's contract holds; ensure you reference and use the existing
cleanup() helper and do not swallow the error when rethrowing.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 39dbe864-f7b1-49df-b27d-6acdfbf99110
📒 Files selected for processing (6)
package.jsonpackages/integration-test-env/README.mdpackages/integration-test-env/package.jsonpackages/integration-test-env/src/ci.tspackages/integration-test-env/src/lifecycle.tspackages/integration-test-env/src/start.ts
…env-up-down # Conflicts: # packages/integration-test-env/src/lifecycle.ts
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@greptile review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
packages/integration-test-env/src/lifecycle.ts:141
handleShutdownnow exits with code 0 for both SIGINT and SIGTERM. That means the CI entrypoint (start:ci) will also exit 0 on SIGTERM, which can mask unexpected terminations (timeouts, supervisor restarts, etc.) as success. Consider exiting non-zero for SIGTERM (or making the exit code configurable per entrypoint) while keeping SIGINT exit 0 for the interactivestartflow.
packages/integration-test-env/src/lifecycle.ts:284--onlycurrently allows starting services without their required dependencies (e.g.ensapiwithoutensindexer/devnet, orensindexerwithoutensrainbow/devnet). In these casesbringUp()still passes localhost URLs (ENSDb/ENSRainbow) and will typically fail in confusing ways. Consider validating the selected set upfront (enforce dependency closure or throw a clear error explaining what else must be selected / already running).
Summary
lifecycle.ts) shared by two entrypointspnpm -F @ensnode/integration-test-env start— bring up the full stack (incl. seed) and block until Ctrl+C, sopnpm test:integrationcan run from another terminalstartscript is renamed tostart:ci(bring up + run tests + tear down); the roottest:integration:cialias is updated to match so CI behavior is unchanged--onlyflag:pnpm start --only devnet,ensrainbowrestricts the subset of services brought up, so you can iterate on ensindexer/ensapi locally and have the rest auto-managed + auto-cleaned. valid services:devnet(incl. ensdb + seed),ensrainbow,ensindexer(incl. wait-for-indexing),ensapiLOG_LEVEL=errorso its info/warn chatter doesn't drown out the rest of the stack outputstartexits 0 (user-initiated shutdown is not an error)Why
Running the integration test env manually (
docker compose up devnet ensrainbow+ standalone ensapi) skips the orchestrator'sseedDevnetphase, so resolver/primary-name records aren't on chain and 6 resolution integration tests fail. This adds a one-shot manual path that runs the same lifecycle CI does (seed included), without coupling it to a test run.--onlyexists so when iterating on ensindexer or ensapi (running them yourself in dev mode), the supporting services come up and tear down with one command instead of separatedocker composeinvocations.Testing
pnpm -F @ensnode/integration-test-env typecheck✓pnpm lint✓pnpm start/ Ctrl+C teardown not exercised locally (existing CI flow is unchanged becausestart:ciis identical to the oldstartmodulo a one-line extracted call). Worth a manual spot-check before merge.Notes for Reviewer (Optional)
lifecycle.tsis a mechanical extraction of phases 1–6 fromorchestrator.ts— diff is large only because of the move; the only logic change is oneawait waitForHealth(...)URL now usesendpoints.ensapiinstead of an inline string.lifecycle.tsso both entrypoints share them automatically.--onlygates each phase via ashould(service)check insidebringUp. ci.ts passes no options → full stack as before.Checklist
🤖 Generated with Claude Code