test: cover orpc and next with the shared middleware conformance matrix - #479
test: cover orpc and next with the shared middleware conformance matrix#479HugoRCD wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: ff2e24c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
|
Thank you for following the naming conventions! 🙏 |
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds a shared asynchronous integration conformance suite. It validates event behavior, request IDs, route handling, exclusions, enrichment, draining, and response status across oRPC and Next.js integrations. ChangesIntegration conformance
Sequence Diagram(s)sequenceDiagram
participant Vitest
participant runIntegrationConformance
participant ConformanceMount
participant Integration
Vitest->>runIntegrationConformance: run shared conformance checks
runIntegrationConformance->>ConformanceMount: mount integration
ConformanceMount->>Integration: send test requests
Integration-->>runIntegrationConformance: return response status and events
runIntegrationConformance-->>Vitest: return pass or failure result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/docs/content/6.extend/10.custom-framework.md`:
- Around line 297-304: Declare or define the mount function before the Vitest
test uses it in check.run(mount), reusing the inline mount implementation from
the preceding example so the snippet works when copied in isolation. Keep the
existing integrationConformanceChecks test structure unchanged.
In `@packages/evlog/src/shared/conformance.ts`:
- Around line 16-21: Add per-field JSDoc comments to the exported public types
in conformance.ts: document each field of ConformanceRequest,
ConformanceCheck.name, and ConformanceResult, matching the existing
field-documentation style used by ConformanceApp and BaseEvlogOptions. Keep the
type shapes and behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 284392ef-fdc1-4523-8124-1fd000ab8a82
⛔ Files ignored due to path filters (1)
packages/evlog/test/toolkit/__snapshots__/api-surface.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
.changeset/integration-conformance-suite.mdapps/docs/content/6.extend/10.custom-framework.mdpackages/evlog/src/shared/conformance.tspackages/evlog/src/shared/index.tspackages/evlog/test/frameworks/orpc.test.tspackages/evlog/test/helpers/frameworkMatrix.tspackages/evlog/test/next/handler.test.ts
| ```typescript [test/conformance.test.ts] | ||
| import { it, expect } from 'vitest' | ||
| import { integrationConformanceChecks } from 'evlog/toolkit' | ||
|
|
||
| it.each(integrationConformanceChecks)('$name', async (check) => { | ||
| await expect(check.run(mount)).resolves.toBeUndefined() | ||
| }) | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Undefined mount in the Vitest example.
This snippet references mount in check.run(mount), but the snippet never declares it. The preceding example passes an inline arrow function directly to runIntegrationConformance and never binds it to a mount variable, so a reader copying this snippet in isolation gets a ReferenceError.
📝 Proposed fix
import { it, expect } from 'vitest'
import { integrationConformanceChecks } from 'evlog/toolkit'
+import { mount } from './my-integration' // however your adapter exposes it
it.each(integrationConformanceChecks)('$name', async (check) => {
await expect(check.run(mount)).resolves.toBeUndefined()
})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/docs/content/6.extend/10.custom-framework.md` around lines 297 - 304,
Declare or define the mount function before the Vitest test uses it in
check.run(mount), reusing the inline mount implementation from the preceding
example so the snippet works when copied in isolation. Keep the existing
integrationConformanceChecks test structure unchanged.
5372053 to
4eff662
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
29b1878 to
531bae2
Compare
HugoRCD
changed the title
Consolidates the shared HTTP checks into `src/shared/conformance.ts` as runner-agnostic functions that throw instead of calling `expect`, with `describeStandardHttpMatrix` reduced to a Vitest binding over them — one copy rather than two that can drift. Adds two checks (an `exclude`d route is skipped but still served; `enrich` runs before `drain` with the response status) and brings `evlog/orpc` and `evlog/next` into the matrix. Both pass unmodified. The app may now declare the `route` it serves rather than the suite assuming `/api/users`, which surfaced a hardcoded `/api/**` in the per-route service check. Deliberately **not** exported from `evlog/toolkit`. `mount()` injects `drain` as a function, which only fits integrations configured per request. `evlog/nitro`, `evlog/nitro/v3` and `evlog/nuxt` receive theirs through the `evlog:drain` hook inside a built app, and a function cannot cross that build boundary — so the suite structurally cannot drive evlog's flagship integrations, and publishing it as "the contract every integration satisfies" would be inaccurate. The failure mode is also still wrong: a mount serving a 404 is reported as a conformance failure rather than a broken mount. Public API surface is unchanged.
|
Thanks — both comments landed on `53720531`, before this PR was reworked. Status: 1. Undefined `mount` in the Vitest example ( 2. Missing per-field JSDoc ( |
531bae2 to
ff2e24c
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Why
The contract every framework integration shares lived only in this repo's Vitest helper (
describeStandardHttpMatrix). Two consequences:That is not hypothetical. Both of these were found by pointing this matrix at code that had drifted:
evlog/workershonoured onlycf-rayfor the request id, while every other integration honoursx-request-id(feat(workers): run cloudflare workers through the shared middleware pipeline #472)evlog/nextacceptedpluginswithout ever applying them (refactor(next): reuse the shared middleware pipeline #474)What
src/shared/conformance.tsre-expresses the specs as plain async checks that throw on failure, with no test-framework dependency:Exported as
integrationConformanceChecks(the list) andrunIntegrationConformance()(runner returning per-check results).describeStandardHttpMatrixis now a thin Vitest binding over the same list — the repo and the community assert identical behaviour, not two copies that can drift.The contract
x-request-idroutesservice overridesroutessilently ignoredexclude— but still serves themenrichbeforedrain, with the response statusThe last two are new relative to the old matrix.
Coverage
evlog/orpcandevlog/nextjoin the matrix — both pass unmodified, which is the result I wanted: they had the behaviour, it just was not pinned.Coverage is now hono, express, elysia, fastify, nestjs, react-router, sveltekit, orpc, next (+ workers via #472).
evlog/evestays out — it is turn-based, not HTTP.Notes
Writing the suite surfaced that
event.durationis the human-formatted string the logger writes ("2ms"), not a number. The check asserts presence rather than type; worth knowing if you consumedurationdownstream expecting milliseconds.Docs: new "Verify your integration" section on the custom-framework page, with both the standalone runner and the Vitest binding.
Verification
pnpm run test— 1652/1652 pass; full suite run twice back to back plus the framework subset three times, no flakesvi.useFakeTimers(), so the suite's poll-basedwaitForcannot deadlock against thempnpm run lint— 0 errors (2 pre-existingmax-paramswarnings innitro-v3/plugin.ts)pnpm run typecheck— 26/26 tasks passpnpm run api:snapshot— diff is+ integrationConformanceChecks,+ runIntegrationConformanceBased directly on
main, independent of the other open PRs.Summary by CodeRabbit
Bug Fixes
Tests