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.
There was a problem hiding this comment.
we should not allow override of devnet version, the goal is for it to be pinned for both local development and the integration tests, and it should not be overridable
| 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
There was a problem hiding this comment.
devnet is included there to pin the version used in the monorepo, for both developers running locally and for the integration-test-env (the orchestrate.ts script uses the postgres and devnet from the docker-compose)
if that's confusing, then integration-test-env should define its own docker-compose.yml which would just be those two containers, but then developers don't have a trivial docker compose up devnet to do local development
| @@ -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.
There was a problem hiding this comment.
'all chains supported by ENSIP-19' might be most accurate
|
|
||
| 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!
There was a problem hiding this comment.
i'm ok with refactoring this now that they're used all over the place. let's maybe put them in ensnode-sdk/internal
| @@ -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?
There was a problem hiding this comment.
in my pr, process.env.ENSNODE_URL will be available and guaranteed to exist, so after rebasing on top of that, this should change to
const BASE_URL = process.env.ENSNODE_URL!and we won't need to hardcode the localhost:4334 default anywhere else
| @@ -0,0 +1,86 @@ | |||
| import { describe, expect, it } from "vitest"; | |||
|
|
|||
| const BASE_URL = process.env.ENSNODE_URL || "http://localhost:4334"; | |||
There was a problem hiding this comment.
i agree, let's test the endpoint here, not the client. i also think the ensapi client as-is will likely disappear during the migration to enskit, so not sure it's worth investing in testing for it
| }, | ||
| { | ||
| description: | ||
| "resolves primary name for user address on chain 1 (no primary name set in devnet)", |
There was a problem hiding this comment.
let's extract those constants to DEVNET_OWNER and DEVNET_USER like i've done here packages/ensnode-sdk/src/omnigraph-api/example-queries.ts
and perhaps we should export it from the datasouces package to share those constants? or rather maybe we export from ensnode-sdk/internal
| }, | ||
| }, | ||
| { | ||
| description: "resolves primary names across all supported chains when chainIds is omitted", |
There was a problem hiding this comment.
'all chains supported by ENSIP-19' might be most accurate
| @@ -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.
in my pr, process.env.ENSNODE_URL will be available and guaranteed to exist, so after rebasing on top of that, this should change to
const BASE_URL = process.env.ENSNODE_URL!and we won't need to hardcode the localhost:4334 default anywhere else
|
|
||
| const BASE_URL = process.env.ENSNODE_URL || "http://localhost:4334"; | ||
|
|
||
| const OWNER = "0x70997970c51812dc3a010c7d01b50e0d17dc79c8"; |
There was a problem hiding this comment.
i'm ok with refactoring this now that they're used all over the place. let's maybe put them in ensnode-sdk/internal
| container_name: ensindexer | ||
| image: ghcr.io/namehash/ensnode/ensindexer:latest | ||
| image: ghcr.io/namehash/ensnode/ensindexer:${ENSNODE_VERSION:-latest} | ||
| build: |
There was a problem hiding this comment.
we previously had the build param configured but removed it for some reason. but i can't think of a reason to avoid it now
BUT we should update the docker:build:* scripts in package.json to be docker compose build * instead of a direct docker build, to ensure that there's a single path
| 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.
devnet is included there to pin the version used in the monorepo, for both developers running locally and for the integration-test-env (the orchestrate.ts script uses the postgres and devnet from the docker-compose)
if that's confusing, then integration-test-env should define its own docker-compose.yml which would just be those two containers, but then developers don't have a trivial docker compose up devnet to do local development
| 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.
we should not allow override of devnet version, the goal is for it to be pinned for both local development and the integration tests, and it should not be overridable
3 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)