feat: ens-test-kit initial#2028
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
|
||
| ## Status | ||
|
|
||
| Proposal for review. Authored after iterative design conversation. Once approved, see [IMPL.md](./IMPL.md) for the PR-by-PR rollout. |
There was a problem hiding this comment.
this is WIP, just want to share my vision first
|
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 a design proposal (
Confidence Score: 4/5Safe to discuss/merge as a design doc, but the Vitest-coupling P1 should be addressed before implementation begins. Only one file changed (a documentation/proposal), so there is no runtime risk from merging. The P1 finding is a design flaw in the proposal itself that needs to be corrected before the described code is written; if unaddressed, it would cause real bugs in the implementation. .memory-bank/tasks/0006-ens-test-kit/IDEA.md — see Vitest coupling issue at line 240 and dedup behaviour at line 291. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
TK["@ensnode/ens-test-kit"]
TK --> Cases["cases/\n(TestCase<Api>[])"]
TK --> Seeder["seeder/\n(seedFixtures)"]
TK --> Interfaces["interfaces/\n(ResolutionsApi, DomainsApi, ...)"]
TK --> Vitest["vitest/\n(runSuite)"]
TK --> CLI["bin/ens-test-kit seed"]
Seeder --> Fixtures["fixtures/\n(text-record, registration, ...)"]
Seeder --> Docker["devnet/Dockerfile\n(seeds on startup)"]
ITE["@ensnode/integration-test-env"]
ITE --> Adapters["adapters/\n(rest-adapter, omnigraph-adapter)"]
ITE --> Tests["tests/\n(runSuite calls)"]
TK --> ITE
Interfaces --> Adapters
Cases --> Tests
Vitest --> Tests
Reviews (1): Last reviewed commit: "commit idea file" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const account = await api.getAccount(OWNER_ADDRESS); | ||
| return account?.domains.map((d) => d.name); | ||
| }, | ||
| expected: expect.arrayContaining(["test.eth"]), |
There was a problem hiding this comment.
expect matcher embedded in "just data" test case
expected: expect.arrayContaining(["test.eth"]) embeds a live Vitest expect matcher inside the TestCase struct, which is described elsewhere as "just data." This contradicts the design goal: expect is only available at test runtime (inside a describe/it block), so any code that constructs or imports ownershipCases outside a Vitest context — for example the seed CLI, a documentation generator, or a future JSON exporter — will throw at module load time.
The expected: unknown type on TestCase permits this silently today but runSuite will need to distinguish plain values from matchers anyway. Consider using a discriminated union (e.g. { kind: "arrayContaining"; values: string[] }) or a narrow Matcher wrapper that runSuite maps to Vitest matchers internally. This keeps case data serialisable and framework-agnostic while still supporting asymmetric matching.
| import { applyTextRecord } from "./fixtures/text-record"; | ||
| import { applyPrimaryName } from "./fixtures/primary-name"; | ||
| // ... | ||
|
|
||
| const HANDLERS = { | ||
| "text-record": applyTextRecord, | ||
| "primary-name": applyPrimaryName, | ||
| "multicoin-address": applyMulticoinAddress, | ||
| "contenthash": applyContenthash, |
There was a problem hiding this comment.
id could silently discard fixture variants
dedupeFixtures(fixtures) collapses by id. If two independent test cases share a fixture ID but differ in any field (e.g. value, resolverAddress), the second definition is silently dropped. The seeder will then run only the first variant, making one of the two cases test the wrong on-chain state — producing a misleading pass or a hard-to-diagnose failure.
Consider either (a) throwing an error when two fixtures share an id but are not deeply equal, or (b) using content-hash equality as the dedup key so structural differences are always caught at seed time.
1 participant
Summary
Introduce my idea of
ens-test-kit.One might find it easier to review full file like that:
https://github.com/namehash/ensnode/blob/ll/ens-test-kit/.memory-bank/tasks/0006-ens-test-kit/IDEA.md
Why
Testing
Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)