tests: add tests to ensapi using ens-test-env#1873
tests: add tests to ensapi using ens-test-env#1873sevenzing wants to merge 5 commits intoll/chore-fixesfrom
Conversation
🦋 Changeset detectedLatest commit: ac0822d The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Greptile SummaryThis PR adds integration test coverage for the three REST API resolution endpoints in
Confidence Score: 5/5This PR is safe to merge — all changes are test infrastructure and docker configuration with no production code changes. Both findings are P2 (dead code export and a stylistic URL inconsistency). No logic bugs, security issues, or breaking changes are introduced. The tests are well-structured with comprehensive coverage of success and error paths. apps/ensapi/src/test/integration/resolution-api-client.ts — unused file that should either be imported or removed. Important Files Changed
Sequence DiagramsequenceDiagram
participant Test as Integration Tests
participant API as ENSApi (:4334)
participant DB as PostgreSQL
participant DN as Devnet (chain 15658733)
Test->>API: GET /api/resolve/primary-name/:address/:chainId
API->>DB: query primary name record
DB-->>API: null (no primary set in devnet)
API-->>Test: 200 { name: null, accelerationRequested: false }
Test->>API: GET /api/resolve/primary-names/:address?chainIds=1,15658733
API->>DB: query primary names for each chainId
DB-->>API: { 1: null, 15658733: null }
API-->>Test: 200 { names: { "1": null, "15658733": null }, ... }
Test->>API: GET /api/resolve/records/test.eth?addresses=60
API->>DB: query resolver records
DB-->>API: { addresses: { 60: "0x70997970..." } }
API-->>Test: 200 { records: { addresses: { 60: "0x70997970..." } }, ... }
Test->>API: GET /api/resolve/records/TEST.ETH?addresses=60
API-->>Test: 400 { message: "Invalid Input", details: { name: ["Must be normalized..."] } }
Reviews (1): Last reviewed commit: "docs(changeset): add integration tests f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
apps/ensapi/src/handlers/api/resolution/resolve-records.integration.test.ts
Outdated
Show resolved
Hide resolved
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
apps/ensapi/src/handlers/api/resolution/resolve-records.integration.test.ts
Outdated
Show resolved
Hide resolved
sevenzing
changed the title
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@sevenzing Hey cool to see this. Reviewed and shared some suggestions appreciate your advice.
| ensrainbow: | ||
| container_name: ensrainbow | ||
| image: ghcr.io/namehash/ensnode/ensrainbow:latest | ||
| image: ghcr.io/namehash/ensnode/ensrainbow:${ENSNODE_VERSION:-latest} |
There was a problem hiding this comment.
Could you please suggest all the relevant places for us to add docs about ENSNODE_VERSION when running this docker compose file? I assume this is a new environment variable that's being added.
We want to always have all environment variables formally documented.
| devnet: | ||
| container_name: devnet | ||
| image: ghcr.io/ensdomains/contracts-v2:main-e8696c6 | ||
| image: ghcr.io/ensdomains/contracts-v2:${DEVNET_VERSION:-main-e8696c6} |
There was a problem hiding this comment.
Similar feedback about DEVNET_VERSION.
| ENSDB_URL: postgresql://postgres:password@postgres:5432/postgres | ||
| ENSINDEXER_SCHEMA_NAME: docker_compose_ensindexer_schema | ||
| ENSRAINBOW_URL: http://ensrainbow:3223 | ||
| RPC_URL_15658733: http://devnet:8545 |
There was a problem hiding this comment.
Can you please share your advice here?
I'm worried that we're combining too many goals together into this root docker-compose.yml file.
What if someone wants to docker compose just running ENSNode without also running the devnet? In many cases this will be true. I don't believe we should assume that anyone wanting to run docker compose will also want to run the full devnet (and integration tests).
There was a problem hiding this comment.
yes! agree on that, I also dont know why we included devnet as default option.
I would simply create docker directory and store several composes in it splitting by enviroments
docker-compose.yaml-- basic case when user wants to run ensnode against mainnetdocker-compose.devnet.yaml-- case when user wants to run ensnode against devnet .for example try useextend` feature in docker-composedocker-compose.sepolia.yaml
but i think we can discuss it with team in slack thread
| @@ -105,7 +118,7 @@ services: | |||
|
|
|||
| devnet: | |||
There was a problem hiding this comment.
Please see my other feedback above about making the assumption that anyone running docker-compose also wants to run the devnet.
Advice appreciated.
| "ensapi": patch | ||
| --- | ||
|
|
||
| add integration tests for ensapi |
There was a problem hiding this comment.
| add integration tests for ensapi | |
| add integration tests for resolution APIs in ensapi |
| expectedBody: { name: null, accelerationRequested: false, accelerationAttempted: false }, | ||
| }, | ||
| { | ||
| description: "owner address with accelerate=true returns accelerationRequested: true", |
There was a problem hiding this comment.
What is an "owner address"? Owner of what?
| it.each([ | ||
| { | ||
| description: | ||
| "resolves primary names for owner address on chain 1 (no primary name set in devnet)", |
There was a problem hiding this comment.
What is an "owner" address?
| }, | ||
| }, | ||
| { | ||
| description: "resolves primary names across all supported chains when chainIds is omitted", |
There was a problem hiding this comment.
| description: "resolves primary names across all supported chains when chainIds is omitted", | |
| description: "resolves primary names across all indexed chains when chainIds is omitted", |
Can you please double check the correct terminology here with @shrugs? It will be valuable for us to precisely describe this idea.
I'm not sure just from my memory what determines the chainids that will be returned here by default. Let's precisely document it.
|
|
||
| const BASE_URL = process.env.ENSNODE_URL || "http://localhost:4334"; | ||
|
|
||
| const OWNER = "0x70997970c51812dc3a010c7d01b50e0d17dc79c8"; |
There was a problem hiding this comment.
Can you help me understand why we call this OWNER? I'm concerned about the overloaded use of terminology.
Is there a more specific / distinct name we could give this?
There was a problem hiding this comment.
we already use this notation in project:
ensnode/packages/ensnode-sdk/src/omnigraph-api/example-queries.ts
Lines 32 to 34 in abc535f
ensnode/apps/ensadmin/src/app/inspect/_lib/example-addresses.ts
Lines 17 to 21 in 4567b52
I thought this is well known names for devnet testing 😄
I had an idea to reuse defined const for deployer and user, but importing them from omnigraph/examples-queries.ts looked weird and I didn't want to overload the PR by moving it in a separate module, but I can do it without any problems!
| @@ -0,0 +1,230 @@ | |||
| import { describe, expect, it } from "vitest"; | |||
|
|
|||
| const BASE_URL = process.env.ENSNODE_URL || "http://localhost:4334"; | |||
There was a problem hiding this comment.
Instead of directly reading from environment variables in these integration test files, what if you read from the config object as exported from apps/ensapi/src/config/index.ts?
Goal: Prefer to constrain where the responsibility lives for reading from / parsing environment variables.
There was a problem hiding this comment.
agree, I didt like my solution here, and wanted to use config. I just decided to be concise with current approach here
and didnt want to rewrite existing tests.
Should I simply try to use this approach in resolution ensapi only or try to apply in omnigraph also?
2 participants
Summary
resolve-primary-name,resolve-primary-names,resolve-records)docker-compose.yml: addbuildsection to build images locally, also addedRPC_URL_15658733because without it running indexer withens-test-envresults in fail with error likefailed to make request to localhost:8545Why
Pre-Review Checklist (Blocking)