integration-test-env: add start (brings up the stack only, no tests)#2091
integration-test-env: add start (brings up the stack only, no tests)#2091shrugs wants to merge 3 commits into
start (brings up the stack only, no tests)#2091Conversation
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 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 (6)
✨ 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 |
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>
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.
| 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: | ||
|
|
||
| ```sh | ||
| pnpm test:integration | ||
| ``` |
| "scripts": { | ||
| "start": "CI=1 tsx src/orchestrator.ts", | ||
| "start": "tsx src/start.ts", | ||
| "start:ci": "CI=1 tsx src/ci.ts", | ||
| "typecheck": "tsc --noEmit" | ||
| }, |
Greptile SummaryThis PR splits the integration test orchestrator into a shared
Confidence Score: 5/5Safe to merge — the CI path is functionally identical to the old orchestrator.ts, and the new manual start.ts is an additive convenience tool. The diff is almost entirely a mechanical extraction: bringUp() is the old main() body wrapped in if (should(svc)) guards, runIntegrationTests() is the last block of the old main(), and ci.ts re-assembles them in the same order. The one logic change (SIGINT handler exits 0 instead of 1) is intentional and documented. No new runtime paths are exercised by CI. No files require special attention; the most nuanced code is in lifecycle.ts, but it is a faithful extraction of the previously reviewed orchestrator logic. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["pnpm start"] --> S["start.ts\n(parse --only flag)"]
B["pnpm start:ci\n(CI=1)"] --> C["ci.ts"]
S --> BU["lifecycle.bringUp(only?)"]
C --> BU
BU --> P1["Phase 1: ENSDb + Devnet"]
P1 --> P2["Phase 2: Seed Devnet"]
P2 --> P3["Phase 3: ENSRainbow"]
P3 --> P4["Phase 4: ENSIndexer"]
P4 --> P5["Phase 5: Poll indexing"]
P5 --> P6["Phase 6: ENSApi"]
P6 -->|"start.ts"| BLOCK["Block forever"]
BLOCK -->|"SIGINT/SIGTERM"| HS["handleShutdown() exit 0"]
HS --> CL["cleanup()"]
P6 -->|"ci.ts"| T["runIntegrationTests()"]
T -->|"pass"| CL2["cleanup() exit 0"]
T -->|"fail"| ERR["catch cleanup() exit 1"]
Reviews (2): Last reviewed commit: "fix: allow --only" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * Bring up the integration test environment: ENSDb + Devnet, seed, ENSRainbow, ENSIndexer, | ||
| * wait for indexing to complete, ENSApi. Returns once every service is healthy. | ||
| * | ||
| * On failure, runs cleanup() and rethrows. | ||
| */ |
There was a problem hiding this comment.
bringUp() says "On failure, runs cleanup() and rethrows", but bringUp itself never calls cleanup() — it simply throws and leaves cleanup to the caller. Both ci.ts and start.ts do handle this correctly in their .catch() blocks, so runtime behaviour is fine, but the comment will mislead future readers about where cleanup responsibility sits.
| * Bring up the integration test environment: ENSDb + Devnet, seed, ENSRainbow, ENSIndexer, | |
| * wait for indexing to complete, ENSApi. Returns once every service is healthy. | |
| * | |
| * On failure, runs cleanup() and rethrows. | |
| */ | |
| * Bring up the integration test environment: ENSDb + Devnet, seed, ENSRainbow, ENSIndexer, | |
| * wait for indexing to complete, ENSApi. Returns once every service is healthy. | |
| * | |
| * On failure, throws — callers are responsible for calling cleanup(). | |
| */ |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| ``` | ||
|
|
||
| Works both in CI and locally — just make sure the required ports are available (8545, 8000, 3223, 42069, 4334). | ||
| 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: |
There was a problem hiding this comment.
| 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: |
README lists incorrect ports (includes non-existent 8000, omits PostgreSQL port 5433)
| * Note: `devnet` includes ensdb (coupled via docker-compose) and seeding. `ensindexer` includes | ||
| * waiting for indexing to reach following/completed. | ||
| * | ||
| * On failure, runs cleanup() and rethrows. |
There was a problem hiding this comment.
| * On failure, runs cleanup() and rethrows. | |
| * On failure, throws — callers are responsible for calling cleanup(). |
JSDoc for bringUp() incorrectly claims the function calls cleanup() on failure, but it doesn't—cleanup is the caller's responsibility
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