Speed up FormData multipart serialization - #31379
Merged
Merged
Claude / Claude Code Review
completed
May 25, 2026 in 11m 35s
Code review found 2 potential issues
Found 5 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | test/js/web/html/FormData-multipart-serialization.test.ts:144-146 |
Spawn test asserts exitCode before stdout (CLAUDE.md ordering) |
| 🟡 Nit | test/js/web/html/FormData-multipart-serialization.test.ts:1 |
New test file created instead of adding to existing FormData.test.ts |
Annotations
Check warning on line 146 in test/js/web/html/FormData-multipart-serialization.test.ts
claude / Claude Code Review
Spawn test asserts exitCode before stdout (CLAUDE.md ordering)
nit: per CLAUDE.md ("tests should `expect(stdout).toBe(...)` BEFORE `expect(exitCode).toBe(0)`") and the canonical spawn pattern in `test/CLAUDE.md`, consider asserting `expect(stderr).toBe("")` before the `exitCode` check so a subprocess crash surfaces the actual output in the assertion diff rather than only via `console.error`. Not blocking — the `console.error(stderr)` guard already covers most of the intent.
Check warning on line 1 in test/js/web/html/FormData-multipart-serialization.test.ts
claude / Claude Code Review
New test file created instead of adding to existing FormData.test.ts
nit: both `CLAUDE.md` (line 55) and `test/CLAUDE.md` (line 152) say to add tests to the existing test file rather than creating a new one — `test/js/web/html/FormData.test.ts` already exists and would be the natural home for these as a `describe("multipart serialization", ...)` block. The `FormData-file-error-leak.test.ts` precedent you cite is itself an exception (it spawns a leak-tracking subprocess), not the documented default. Not blocking, but consider folding these in.
Loading