test: refactor client-request.js #4500
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the test file test/client-request.js to modernize its testing patterns and improve consistency. The main purpose is to replace callback-based server listening patterns with promise-based approaches using once() from Node.js events, and to consolidate import statements.
- Replace callback-based
server.listen()patterns withawait once(server.listen(0), 'listening') - Consolidate and reorganize import statements at the top of the file
- Remove redundant closing braces from callback-based patterns
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| }) | ||
|
|
||
| test('request with FormData body', async (t) => { | ||
| t = tspl(t, { plan: 5 }) |
There was a problem hiding this comment.
The plan count of 5 appears incorrect. Based on the test logic, there are no explicit assertions being made in this test that would require 5 planned test cases. This could cause the test to hang or fail unexpectedly.
Copilot uses AI. Check for mistakes.
| queueMicrotask(() => { | ||
| res.end(data.slice(1)) | ||
| }) |
There was a problem hiding this comment.
Using queueMicrotask() instead of setTimeout() changes the timing behavior significantly. queueMicrotask() executes immediately after the current execution stack, while setTimeout() provided a 100ms delay. This could affect the test's ability to properly test multibyte handling across chunks.
Copilot uses AI. Check for mistakes.
| queueMicrotask(() => { | ||
| res.end(data.slice(1)) | ||
| }) |
There was a problem hiding this comment.
Similar to the previous test, replacing setTimeout() with queueMicrotask() changes the timing behavior. The original 100ms delay may have been intentional to test how the client handles chunked multibyte data over time.
Copilot uses AI. Check for mistakes.
| queueMicrotask(() => { | ||
| res.end(data.slice(1)) | ||
| }) |
There was a problem hiding this comment.
Again, replacing setTimeout() with queueMicrotask() changes the timing behavior. This could impact the test's effectiveness in validating multibyte text handling with different encodings when data arrives in separate chunks.
Copilot uses AI. Check for mistakes.
| res.write('hello from server') | ||
| res.end() | ||
| }, 100) | ||
| }, Infinity) |
There was a problem hiding this comment.
Using Infinity as the timeout value will cause the server to never send a response, which will likely cause the test to hang indefinitely. The original 100ms timeout was probably intentional to allow the abort signal to be tested properly.
| }, Infinity) | |
| }, 100) |
Copilot uses AI. Check for mistakes.
This relates to...
Rationale
Changes
Features
Bug Fixes
Breaking Changes and Deprecations
Status