Feat: add e2e request body parsing test#98
Merged
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
uWestJS Benchmark Results
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/http/body-parsing.e2e.spec.ts (1)
239-252: ⚡ Quick winRepeated-key URL-encoded assertion should validate values, not just presence.
toBeDefined()is too loose; assert exact shape/value(s) to lock behavior (e.g., array or deterministic merge policy).🤖 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 `@test/http/body-parsing.e2e.spec.ts` around lines 239 - 252, The test "should handle array-like parameters" currently only checks presence of parsed.tags; replace that loose assertion with a deterministic value check: call fetch to `${baseUrl}/body-test/urlencoded`, parse JSON into parsed, then assert parsed.tags equals the expected array ['javascript','typescript','nodejs'] (or the exact merge policy your body parser uses) so the test validates the actual values/shape for the endpoint handling repeated URL-encoded keys.
🤖 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 `@package.json`:
- Around line 16-18: The Jest path patterns in package.json scripts (test:unit
and test:integration) use unescaped dots which act as "any character" in regex;
update the patterns for "test:unit" (the --testPathIgnorePatterns value) and
"test:integration" (the --testPathPatterns value) to escape all literal dots
(e.g., change ".integration.spec.ts" and ".e2e.spec.ts" to use escaped dots like
"\.integration\.spec\.ts" and "\.e2e\.spec\.ts" ensuring the JSON string
contains the necessary backslash escaping) so the CLI regex matches the exact
filenames; keep the "test:e2e" script unchanged.
In `@test/http/body-parsing.e2e.spec.ts`:
- Around line 42-47: Add an end-to-end test that exercises the echoBuffer POST
route to verify raw/binary body parsing: send a binary payload (e.g., Buffer of
known bytes) to the 'buffer' endpoint with Content-Type:
application/octet-stream, then assert the response's length equals the buffer
length and the data field equals the buffer encoded as base64; locate the
handler echoBuffer in the controller and the test file
test/http/body-parsing.e2e.spec.ts to add this assertion so raw/binary behavior
is covered for issue `#96`.
- Around line 125-134: The test "should handle empty JSON body for POST (should
throw)" currently uses a permissive assertion (response.status >= 400); replace
it with an explicit expected status check (e.g.,
expect(response.status).toBe(400) or toBe(415) depending on the server's
intended behavior) and document which specific status is authoritative for this
scenario; likewise update the other tests called out in the review (the nearby
POST/JSON tests and the blocks around the other reported ranges) to assert exact
expected statuses (use toBe or a clear toEqual/toContain matcher against a small
explicit array if multiple statuses are acceptable) instead of broad ranges like
>= 400 or >= 200 so failures map to a precise regression and the intended
behavior is clear.
- Around line 255-309: Add a new test case inside the "Text Body Parsing"
describe block (near the other it() tests) named something like "should handle
UTF-16 text" that sends the same endpoint `/body-test/text` but supplies a
UTF‑16 encoded request body and a Content-Type including charset=utf-16;
construct the body as a Node Buffer encoded in UTF‑16 (include the BOM, e.g.
prepend 0xFF 0xFE and use 'utf16le' encoding) and send that Buffer as the fetch
body, then assert response.status is 200 and response.text() equals the original
Unicode string; this will cover the missing UTF‑16 parsing objective for the
tests around the endpoint used in the file.
---
Nitpick comments:
In `@test/http/body-parsing.e2e.spec.ts`:
- Around line 239-252: The test "should handle array-like parameters" currently
only checks presence of parsed.tags; replace that loose assertion with a
deterministic value check: call fetch to `${baseUrl}/body-test/urlencoded`,
parse JSON into parsed, then assert parsed.tags equals the expected array
['javascript','typescript','nodejs'] (or the exact merge policy your body parser
uses) so the test validates the actual values/shape for the endpoint handling
repeated URL-encoded keys.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab16f3df-f05c-42d7-991e-7f8986fc5ee3
📒 Files selected for processing (6)
eslint.config.mjspackage.jsonsrc/http/core/request.spec.tssrc/http/core/request.tstest/http/body-parsing.e2e.spec.tstest/jest.e2e.config.js
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
E2E Tests added:
Closes #96
Summary by CodeRabbit
Tests
test:integration,test:e2e,test:watch,test:cov).Bug Fixes