chore(nest): add more test for Nest features compatibilty #1156
+1,296
−9
Conversation
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
|
@Mathiasduc is attempting to deploy a commit to the unnoq-team Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis PR adds three compression-related devDependencies to the Nest package and introduces two comprehensive integration test suites for NestJS—one covering core Nest features (interceptors, guards, pipes, middleware, compression) and another validating all supported oRPC response types across different adapters. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50-70 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)packages/nest/tests/nest-features.test.ts (2)
packages/nest/tests/response-types.test.ts (2)
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 |
unnoq
requested changes
Oct 31, 2025
Owner
There was a problem hiding this comment.
Because there tests do not have corresponding .ts file so you should put them into tests folder instead of src
More templates
@orpc/ai-sdk
@orpc/arktype
@orpc/client
@orpc/contract
@orpc/experimental-durable-iterator
@orpc/hey-api
@orpc/interop
@orpc/json-schema
@orpc/nest
@orpc/openapi
@orpc/openapi-client
@orpc/otel
@orpc/experimental-publisher
@orpc/react
@orpc/react-query
@orpc/experimental-react-swr
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-aws-lambda
@orpc/standard-server-fastify
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/standard-server-peer
@orpc/svelte-query
@orpc/tanstack-query
@orpc/trpc
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
dosubot
bot
added
the
size:XXL
Mathiasduc
force-pushed
the
chore/nest/add-test-for-nest-features
branch
from
f072381 to
05476c2
Compare
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/nest/package.json(1 hunks)packages/nest/src/test/nest-features.test.ts(1 hunks)packages/nest/src/test/response-types.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/nest/src/test/response-types.test.ts (2)
packages/contract/src/builder.ts (1)
oc(189-198)packages/nest/src/test/nest-features.test.ts (1)
data(252-261)
packages/nest/src/test/nest-features.test.ts (2)
packages/contract/src/builder.ts (1)
oc(189-198)packages/nest/src/implement.ts (1)
Injectable(102-165)
🔇 Additional comments (1)
packages/nest/package.json (1)
64-75: Compression tooling addition makes senseThese devDependencies line up with the new compression middleware scenarios in the Nest integration tests, so no objections from my side.
Comment on lines
+545
to
+575
| it('should work with global exception filters (useGlobalFilters)', async () => { | ||
| await createApp(TestErrorModule, {}) | ||
|
|
||
| // Request that doesn't throw should succeed | ||
| await request(app.getHttpServer()) | ||
| .post('/error') | ||
| .send({ shouldThrow: false }) | ||
| .expect(200) | ||
| .then((response) => { | ||
| expect(response.body).toEqual({ message: 'No error' }) | ||
| }) | ||
|
|
||
| // Errors thrown inside oRPC handlers are now allowed to bubble up to NestJS | ||
| // so global exception filters can catch and transform them | ||
| await request(app.getHttpServer()) | ||
| .post('/error') | ||
| .send({ shouldThrow: true }) | ||
| .expect(418) // Custom status code from the HttpException | ||
| .then((response) => { | ||
| // The response should be transformed by GlobalHttpExceptionFilter | ||
| expect(response.body).toMatchObject({ | ||
| statusCode: 418, | ||
| message: 'Custom error from handler', | ||
| customFilter: true, // Marker to verify the filter ran | ||
| }) | ||
| expect(response.body.timestamp).toBeDefined() | ||
| }) | ||
|
|
||
| // Ensure that the standard response was not sent via ORPCExceptionFilter | ||
| expect(sendStandardResponseSpy).not.toHaveBeenCalled() | ||
| }) |
There was a problem hiding this comment.
Clear the spy before asserting the error path
In this test we first hit /error with shouldThrow: false, which inevitably calls StandardServerNode.sendStandardResponse. Without clearing the spy afterwards, the later expect(sendStandardResponseSpy).not.toHaveBeenCalled() can never pass. It also accumulates calls across previous Express tests. Please reset the spy before exercising the error scenario (and ideally in a beforeEach) so the assertion actually validates that the exceptional flow bypasses the standard responder.
- await request(app.getHttpServer())
+ await request(app.getHttpServer())
.post('/error')
.send({ shouldThrow: false })
.expect(200)
.then((response) => {
expect(response.body).toEqual({ message: 'No error' })
})
+ sendStandardResponseSpy.mockClear()
+
// Errors thrown inside oRPC handlers are now allowed to bubble up to NestJS📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should work with global exception filters (useGlobalFilters)', async () => { | |
| await createApp(TestErrorModule, {}) | |
| // Request that doesn't throw should succeed | |
| await request(app.getHttpServer()) | |
| .post('/error') | |
| .send({ shouldThrow: false }) | |
| .expect(200) | |
| .then((response) => { | |
| expect(response.body).toEqual({ message: 'No error' }) | |
| }) | |
| // Errors thrown inside oRPC handlers are now allowed to bubble up to NestJS | |
| // so global exception filters can catch and transform them | |
| await request(app.getHttpServer()) | |
| .post('/error') | |
| .send({ shouldThrow: true }) | |
| .expect(418) // Custom status code from the HttpException | |
| .then((response) => { | |
| // The response should be transformed by GlobalHttpExceptionFilter | |
| expect(response.body).toMatchObject({ | |
| statusCode: 418, | |
| message: 'Custom error from handler', | |
| customFilter: true, // Marker to verify the filter ran | |
| }) | |
| expect(response.body.timestamp).toBeDefined() | |
| }) | |
| // Ensure that the standard response was not sent via ORPCExceptionFilter | |
| expect(sendStandardResponseSpy).not.toHaveBeenCalled() | |
| }) | |
| it('should work with global exception filters (useGlobalFilters)', async () => { | |
| await createApp(TestErrorModule, {}) | |
| // Request that doesn't throw should succeed | |
| await request(app.getHttpServer()) | |
| .post('/error') | |
| .send({ shouldThrow: false }) | |
| .expect(200) | |
| .then((response) => { | |
| expect(response.body).toEqual({ message: 'No error' }) | |
| }) | |
| sendStandardResponseSpy.mockClear() | |
| // Errors thrown inside oRPC handlers are now allowed to bubble up to NestJS | |
| // so global exception filters can catch and transform them | |
| await request(app.getHttpServer()) | |
| .post('/error') | |
| .send({ shouldThrow: true }) | |
| .expect(418) // Custom status code from the HttpException | |
| .then((response) => { | |
| // The response should be transformed by GlobalHttpExceptionFilter | |
| expect(response.body).toMatchObject({ | |
| statusCode: 418, | |
| message: 'Custom error from handler', | |
| customFilter: true, // Marker to verify the filter ran | |
| }) | |
| expect(response.body.timestamp).toBeDefined() | |
| }) | |
| // Ensure that the standard response was not sent via ORPCExceptionFilter | |
| expect(sendStandardResponseSpy).not.toHaveBeenCalled() | |
| }) |
🤖 Prompt for AI Agents
In packages/nest/src/test/nest-features.test.ts around lines 545 to 575, the
test leaves sendStandardResponseSpy populated from the non-error request,
causing the final expectation to always fail; clear or reset the spy before
exercising the error scenario (or better, in a beforeEach for the suite) so that
the later expect(sendStandardResponseSpy).not.toHaveBeenCalled() actually
verifies the error path—use sendStandardResponseSpy.mockClear() / mockReset()
(or jest.clearAllMocks()) immediately after the non-throwing request (or in
beforeEach) so the spy has zero calls before the failing request.
Comment on lines
+441
to
+458
| if (!response.text) { | ||
| // Skip if response is undefined (GET with body may not be supported in all scenarios) | ||
| return | ||
| } | ||
|
|
||
| // Parse SSE format | ||
| const events = response.text | ||
| .split('\n\n') | ||
| .filter(Boolean) | ||
| .map((event) => { | ||
| const dataMatch = event.match(/data: (.+)/) | ||
| return dataMatch ? JSON.parse(dataMatch[1] as string) : null | ||
| }) | ||
| .filter(Boolean) | ||
|
|
||
| // Verify we got all events | ||
| expect(events.length).toBeGreaterThanOrEqual(3) | ||
| expect(events[0]).toMatchObject({ message: 'Event 1', index: 0 }) |
There was a problem hiding this comment.
Don’t skip SSE assertions on missing payload
Line 441 currently bails out whenever response.text is falsy, which means the test silently passes even if the stream never delivered any events. That defeats the purpose of the coverage. Please assert that the payload exists before parsing (and apply the same fix to the other SSE helpers below) so a broken stream actually fails the test.
- if (!response.text) {
- // Skip if response is undefined (GET with body may not be supported in all scenarios)
- return
- }
-
- // Parse SSE format
- const events = response.text
+ const rawStream = response.text
+ expect(rawStream, 'SSE payload should not be empty').toBeTruthy()
+
+ // Parse SSE format
+ const events = rawStream📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!response.text) { | |
| // Skip if response is undefined (GET with body may not be supported in all scenarios) | |
| return | |
| } | |
| // Parse SSE format | |
| const events = response.text | |
| .split('\n\n') | |
| .filter(Boolean) | |
| .map((event) => { | |
| const dataMatch = event.match(/data: (.+)/) | |
| return dataMatch ? JSON.parse(dataMatch[1] as string) : null | |
| }) | |
| .filter(Boolean) | |
| // Verify we got all events | |
| expect(events.length).toBeGreaterThanOrEqual(3) | |
| expect(events[0]).toMatchObject({ message: 'Event 1', index: 0 }) | |
| const rawStream = response.text | |
| expect(rawStream, 'SSE payload should not be empty').toBeTruthy() | |
| // Parse SSE format | |
| const events = rawStream | |
| .split('\n\n') | |
| .filter(Boolean) | |
| .map((event) => { | |
| const dataMatch = event.match(/data: (.+)/) | |
| return dataMatch ? JSON.parse(dataMatch[1] as string) : null | |
| }) | |
| .filter(Boolean) | |
| // Verify we got all events | |
| expect(events.length).toBeGreaterThanOrEqual(3) | |
| expect(events[0]).toMatchObject({ message: 'Event 1', index: 0 }) |
🤖 Prompt for AI Agents
In packages/nest/src/test/response-types.test.ts around lines 441 to 458, the
test currently returns early when response.text is falsy which masks missing SSE
payloads; replace the early return with an explicit assertion that response.text
is defined/non-empty (e.g. expect(response.text).toBeTruthy() or
toHaveLength(...) ) so the test fails when no SSE payload is present, then
proceed to parse the SSE text and run the existing event-count and content
assertions; apply the same change to the other SSE helper/test blocks mentioned
in the comment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary by CodeRabbit
Chores
Tests