Open
Conversation
|
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes introduce a new integration test environment package that orchestrates multiple services (Postgres, devnet, ENSRainbow, ENSIndexer, ENSApi) for testing, along with CI workflow configuration updates and test assertions for v1 ETH domain support. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test Environment
participant Postgres as Postgres DB
participant Devnet as Devnet
participant Rainbow as ENSRainbow
participant Indexer as ENSIndexer
participant API as ENSApi
participant Runner as Test Runner
Test->>Postgres: Start service
activate Postgres
Test->>Devnet: Start service
activate Devnet
Test->>Rainbow: Download & extract data
Test->>Rainbow: Start service
activate Rainbow
Test->>Rainbow: Health check
Rainbow-->>Test: Healthy
Test->>Indexer: Start with env config
activate Indexer
Test->>Indexer: Poll indexing status
Indexer-->>Test: Status check (Following/Completed)
Test->>API: Start service
activate API
Test->>API: Health check
API-->>Test: Healthy
Test->>Runner: Execute integration tests
activate Runner
Runner-->>Test: Tests complete
deactivate Runner
Test->>Test: Cleanup & shutdown
deactivate API
deactivate Indexer
deactivate Rainbow
deactivate Devnet
deactivate Postgres
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR wires up end-to-end integration tests for ENSApi into CI by adding a new Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant CI as GitHub Actions
participant Script as ci.ts
participant PG as PostgreSQL (testcontainer)
participant Dev as Devnet (testcontainer)
participant ER as ENSRainbow (child process)
participant EI as ENSIndexer (child process)
participant EA as ENSApi (child process)
participant Tests as vitest (pnpm test:integration)
CI->>Script: tsx src/test/integration/ci.ts
Script->>PG: start PostgreSqlContainer
PG-->>Script: DATABASE_URL
Script->>Dev: start GenericContainer (devnet)
Dev-->>Script: healthy (Wait.forHttp /health)
Script->>Script: download & extract ENSRainbow DB
Script->>ER: spawn pnpm serve
Script->>Script: waitForHealth (ENSRainbow /health, 120s)
Script->>EI: spawn pnpm start
Script->>Script: waitForHealth (ENSIndexer /health, 120s)
Script->>Script: pollIndexingStatus (300s)
EI-->>Script: omnichain-following / omnichain-completed
Script->>EA: spawn pnpm start
Script->>Script: waitForHealth (ENSApi /health, 60s)
Script->>Tests: execSync pnpm test:integration
Tests-->>Script: pass / fail
Script->>ER: SIGTERM
Script->>EI: SIGTERM
Script->>EA: SIGTERM
Script->>Dev: container.stop()
Script->>PG: container.stop()
Script-->>CI: exit 0 / exit 1
Last reviewed commit: 9dd74a0 |
![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}
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test_ci.yml:
- Around line 106-111: Add a Docker preflight step before the "Run integration
tests" step so the CI fails fast when Docker is unavailable; insert a step named
like "Docker preflight check" that runs a simple command (e.g., docker info or
docker ps) in the same job/runner and returns nonzero on failure, placed
immediately before the step that runs pnpm test:integration:ci which executes
apps/ensapi/src/test/integration/ci.ts, ensuring the workflow errors with an
explicit message when Docker is not available.
- Around line 103-111: The new GitHub Actions job "integration-tests" lacks an
explicit permissions block and currently inherits repo defaults; add a job-level
permissions entry for the integration-tests job to restrict GITHUB_TOKEN to
least-privilege read-only access (e.g., read-only for contents and any other
required scopes like packages or actions if needed) so the job no longer uses
broader inherited permissions; update the "integration-tests" job definition
(the job with name "Integration Tests (ens-test-env)" that runs pnpm
test:integration:ci) to include this permissions block.
In `@apps/ensapi/src/test/integration/ci.ts`:
- Line 155: The current log call uses DATABASE_URL directly (log(`Postgres is
ready (${DATABASE_URL})`)), which exposes credentials; change it to avoid
printing the full connection URI by either logging a redacted string or derived
safe fields (e.g., host/port/db name) instead. In the block where log(...) is
called, replace usage of DATABASE_URL with a redaction routine that strips
userinfo (username:password) or build a safe message like "Postgres is ready
(host=..., db=...)" by parsing DATABASE_URL before logging; ensure the symbol
log and the environment variable DATABASE_URL are the only touched identifiers.
- Around line 181-188: Replace the shell-interpolated execSync calls with Node's
execFileSync and pass the command and an argv array instead of a single template
string: call execFileSync('bash',
[`${ENSRAINBOW_DIR}/scripts/download-prebuilt-database.sh`, DB_SCHEMA_VERSION,
LABEL_SET_ID, LABEL_SET_VERSION], { cwd: ENSRAINBOW_DIR, stdio: 'inherit', env:
{ ...process.env, OUT_DIR: downloadTempDir } }) (and do the same for the other
execSync invocation around lines 197-199), so you avoid shell interpolation and
properly pass arguments; locate the usages of execSync in this file and replace
them with execFileSync with the described args and same options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 834ee0c9-7f20-4bf3-abf8-ac02b006eda2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
.github/workflows/test_ci.ymlapps/ensapi/package.jsonapps/ensapi/src/graphql-api/schema/query.integration.test.tsapps/ensapi/src/test/integration/ci.tsapps/ensrainbow/scripts/download-prebuilt-database.sh
- use execFileSync instead of execSync to avoid shell injection - add abort flag to detect spawned service crashes during polling - log non-ok HTTP responses in waitForHealth - don't log DATABASE_URL credentials, log port only - mkdirSync before tar extraction - remove try/catch around test execution (let errors propagate) - add timeout-minutes and permissions to CI workflow Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test_ci.yml:
- Around line 109-115: The CI integration job timeout is too short
(timeout-minutes: 10) and causes false failures during long startup/indexing
waits; update the workflow step for the "Run integration tests" job (the block
containing timeout-minutes and the step named "Run integration tests" with
working-directory: apps/ensapi) to increase timeout-minutes to a larger value
(e.g., 30) so the orchestration script has enough time for
devnet/ENSRainbow/ENSIndexer/indexing/ENSApi startup before tests run.
In `@apps/ensapi/src/test/integration/ci.ts`:
- Around line 115-121: The exit handler for child processes only treats non-zero
exit codes as aborts; update the child.on("exit", ...) callback to also handle
signal-terminated exits by checking the signal parameter (when code === null and
signal !== null), set aborted = true, set abortReason to include the signal, and
call logError with a descriptive message including the signal so checkAborted(),
waitForHealth(), and pollIndexingStatus() will immediately detect and fail when
a child was killed by a signal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4811b1b4-bb2c-439e-924e-e8f566a1e840
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
.github/workflows/test_ci.ymlapps/ensapi/package.jsonapps/ensapi/src/test/integration/ci.ts
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ensapi/src/test/integration/ci.ts`:
- Around line 73-85: The health-check loop in waitForHealth is using fetch(url)
without a per-attempt timeout so a hung request can block the whole retry loop;
update waitForHealth to create an AbortController for each attempt, pass its
signal to fetch, and start a per-attempt setTimeout that calls
controller.abort() after a chosen per-attempt timeout (e.g., min(remainingTime,
5000ms)); ensure you clear that timer and ignore/handle the abort error in the
catch block, and continue the loop so the existing res.ok handling (and
log(`${label} ...` messages) still applies.
- Around line 97-125: spawnService currently attaches stdout/stderr/exit
handlers to the spawned child but misses a child.on("error") handler; add a
child.on("error", (err) => { ... }) handler that calls logError with the error
details, sets aborted = true and sets abortReason to include the label and
err.message (or err.toString()), and ensure any necessary cleanup (e.g., killing
the child or marking it as not running) is performed so the orchestration fails
fast on spawn failures; reference the existing child variable, spawnService,
logError, aborted, and abortReason when implementing this.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e723e0d9-a9bd-4ca4-8186-54500913cb0c
📒 Files selected for processing (1)
apps/ensapi/src/test/integration/ci.ts
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 8 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
…rors when killing pnpm-managed processes during cleanup, pnpm exits non-zero (ELIFECYCLE) rather than being signal-terminated, which triggered setAborted() and logged false errors. setting cleanupInProgress at the start of cleanup() prevents this. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
pnpm doesn't forward SIGTERM to its child processes, so the actual node servers (ensapi, ensindexer, ensrainbow) were left running as orphans after pnpm exited. spawn with detached: true to create a new process group per service, then kill(-pid) to terminate the whole group. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
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
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.
closes #1698
Reviewer Focus (Read This First)
packages/integration-test-env/src/ci.ts— the orchestrator. this is the main deliverable. review the phased startup, cleanup ordering, and abort-flag pattern.integration-testsjob intest_ci.yml. simple, but confirm the timeout-minutes and runner config are appropriate.Problem & Motivation
*.integration.test.ts) that test the graphql api against a real stack, but there was no way to run them in CI.What Changed (Concrete)
@ensnode/integration-test-envatpackages/integration-test-env/— owns testcontainers, execa, and ensnode SDK dependenciesci.tsorchestrator script that starts postgres + devnet (parallel via testcontainers), downloads ensrainbow database, starts ensrainbow/ensindexer/ensapi from source, polls for indexing completion, runspnpm test:integration, thencleans up
integration-testsjob in.github/workflows/test_ci.ymlwith 10-minute timeoutpnpm test:integration:ciscript at monorepo root that delegates to the new packagepnpm test:integrationscript at monorepo root using vitest integration config with--silent passed-onlyquery.integration.test.tsv1 eth domain assertion — wastoBeUndefined()with a TODO, now correctly assertstoMatchObject()withmakeENSv1DomainIdENSAPI_GRAPHQL_API_URL→ENSNODE_URLin integration test client configdownload-prebuilt-database.shexecutable (mode 0644 → 0755)permissions: contents: readto the workflow fileDesign & Planning
no upfront design doc. built iteratively: started with raw
docker runcommands, migrated to testcontainers for postgres (dynamic ports, built-in health checks), then added execa for subprocess management after debugging cleanup issues withraw
spawn.considered running ensrainbow/ensindexer/ensapi as Docker containers too, but running from source means CI tests the actual code in the PR.
considered using the ensrainbow Docker entrypoint.sh directly, but it has Docker-specific paths (
/app/...), a netcat placeholder listener, and asleep 2that make it unsuitable for source-based runs.Planning artifacts: none
Reviewed / approved by: n/a
Self-Review
spawn+waitForExit+ChildProcesstracking with execa (cleanup: true,forceKillAfterDelay,reject: false,extendEnv).ENSAPI_GRAPHQL_API_URL→ENSNODE_URLfor consistency.test:integration:ciscript from ensapi.Cross-Codebase Alignment
OmnichainIndexingStatusIds,ENSNamespaceIds,ensTestEnvChain,ENSRAINBOW_DEFAULT_PORT,entrypoint.shapps/ensrainbow/scripts/entrypoint.sh(considered reusing, too Docker-specific),packages/datasources/src/lib/chains.ts(confirmed devnet hardcodeslocalhost:8545),packages/ensnode-sdk/src/indexing-status/(confirmed
OmnichainIndexingStatusIdsexports)entrypoint.shandci.ts— should be extracted to a reusable script (follow-up)Downstream & Consumer Impact
no public API changes
integration tests now run in CI on every PR — regressions will be caught automatically
ENSAPI_GRAPHQL_API_URLenv var renamed toENSNODE_URLin the integration test client (only used by integration tests, not production)Public APIs affected: none
Docs updated: frontmatter in
ci.tsdocuments design decisionsNaming decisions worth calling out:
ENSNODE_URLreplacesENSAPI_GRAPHQL_API_URLTesting Evidence
ran the full orchestrator locally — all integration tests pass (104/104)
ran in CI via the PR — verified the workflow job executes end-to-end
unit tests unaffected (
pnpm testpasses, 84/84 in ensapi)Testing performed: local + CI end-to-end runs
Known gaps: no test for the orchestrator itself (it's the test runner). cleanup behavior under various failure modes was tested manually.
What reviewers have to reason about manually: whether the cleanup ordering and signal handling are correct in edge cases (crash during startup, crash during indexing, repeated Ctrl-C)
Scope Reductions
extract ensrainbow
prepare-database.sh— the download + extract + validate logic is duplicated betweenentrypoint.shandci.ts. should be a shared script that both consume. deferred to keep this PR focused on CI plumbing.Follow-ups: reusable
prepare-database.shin ensrainbowWhy they were deferred: entrypoint.sh refactoring is a separate concern and would expand the blast radius of this PR
Risk Analysis
devnet image pinned to
main-f476641— if that image is removed from ghcr, CI breaks. low risk since it's a published release.fixed ports for devnet (8545/8000) — will conflict if another service uses those ports on the CI runner. acceptable because CI runners are ephemeral.
10-minute timeout — if indexing or image pulls slow down, the job will fail. can be bumped.
testcontainers requires Docker — CI runner must have Docker available. blacksmith runners do.
Risk areas: devnet image availability, port conflicts on CI runners
Mitigations or rollback options: revert the workflow job to disable integration tests. the package is private and has no consumers.
Named owner if this causes problems: @shrugs
Pre-Review Checklist (Blocking)