test: add node 16 #562
test: add node 16 #562
Conversation
WalkthroughThe changes in this pull request primarily involve updates to the Node.js version handling in various test files and the GitHub Actions workflow configuration. Node.js version 16 has been added to the CI testing environment, necessitating conditional skips for certain tests that are not compatible with this version. Additionally, improvements have been made to test descriptions and setup configurations, ensuring that the testing framework accommodates the new version while maintaining overall functionality. Changes
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
commit: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #562 +/- ##
==========================================
+ Coverage 94.79% 95.81% +1.02%
==========================================
Files 11 11
Lines 1267 1267
Branches 305 306 +1
==========================================
+ Hits 1201 1214 +13
+ Misses 62 49 -13
Partials 4 4 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
vite.config.ts (1)
15-17: LGTM! Adding test setup file for Node.js compatibility.The addition of
test/setup.tsis appropriate for handling Node.js version-specific setup.Add a trailing comma for consistency with the style guide:
setupFiles: [ - 'test/setup.ts' + 'test/setup.ts', ],🧰 Tools
🪛 eslint
[error] 16-17: Missing trailing comma.
(comma-dangle)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
.github/workflows/nodejs.yml(1 hunks)test/HttpClient.test.ts(3 hunks)test/options.compressed.test.ts(2 hunks)test/options.writeStream.test.ts(1 hunks)test/setup.ts(1 hunks)vite.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/setup.ts
🧰 Additional context used
🪛 eslint
vite.config.ts
[error] 16-17: Missing trailing comma.
(comma-dangle)
🔇 Additional comments (6)
test/options.writeStream.test.ts (1)
103-105: LGTM! Well-documented Node.js version compatibility handling.
The conditional skip for Node.js 16 is properly implemented with a clear reference to issue #459.
Let's verify the referenced issue and related changes:
Consider making the version check comment more specific:
- // writeStream only work with error handle on Node.js >= 18, skip on Node.js 16
+ // writeStream error handling is unreliable on Node.js 16 (see #459), requires Node.js >= 18✅ Verification successful
Let's analyze the implementation details to verify if the Node.js version check is justified and properly handled.
Node.js 16 writeStream error handling skip is justified
The verification confirms this is a valid compatibility fix:
- Issue When using version 3.x, if an error occurs in writeStream, no error will be thrown #459 documents a bug where writeStream errors weren't being thrown properly in urllib 3.x
- The test implements proper error handling verification by creating a write stream to a non-existent directory
- The skip is specifically targeting Node.js 16 where the error handling behavior is unreliable
- The test ensures writeStream errors are properly propagated in Node.js >= 18
The version check and skip implementation is appropriate for maintaining compatibility while ensuring proper error handling testing on supported versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the referenced issue
gh issue view 459 --repo node-modules/urllib
# Find other similar version checks
rg "process\.version\.startsWith\('v16'\)" -A 2
Length of output: 881
Script:
#!/bin/bash
# Let's search for the writeStream error handling implementation
rg "writeStream.*error" -A 5 -B 5
# Find all test files related to writeStream
fd "writeStream.*test" --type f
# Check the implementation of writeStream error handling
ast-grep --pattern 'writeStream.on("error", $_)'
Length of output: 2789
test/options.compressed.test.ts (2)
169-169: LGTM: Fixed typo in test description
The test description now correctly spells "invalid".
Line range hint 196-200: LGTM: Added Node.js 16 version compatibility check
The conditional check for error codes has been appropriately updated to handle Node.js 16's different error code format.
Let's verify the error codes across Node.js versions:
✅ Verification successful
Let me gather more context about Node.js version-specific error handling in the codebase.
Error code handling is correctly implemented for Node.js version compatibility
The verification confirms that:
- The error code check is part of a brotli decompression test case
- The version check
process.version !== 'v18.19.0' && !process.version.startsWith('v16.')correctly handles different error codes:ERR_PADDING_1for Node.js v16.x and v18.19.0ERR__ERROR_FORMAT_PADDING_1for other Node.js versions
- This is the only occurrence of these error codes in the codebase, and they are used consistently within their version-specific context
- The codebase shows a pattern of careful Node.js version handling, with other version-specific tests being properly marked with
skipIf
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the error codes in the codebase
# Look for any other occurrences of these error codes to ensure consistency
rg "ERR_PADDING_1|ERR__ERROR_FORMAT_PADDING_1" -A 2 -B 2
Length of output: 594
Script:
#!/bin/bash
# Look for other Node.js version-specific error handling patterns
rg "process.version" -A 3 -B 3
# Check test file content for better context
cat test/options.compressed.test.ts
Length of output: 10882
test/HttpClient.test.ts (3)
276-276: LGTM: Fixed typo in test description
The test description now correctly spells "protocol".
Line range hint 470-476: LGTM: Added Node.js 16 version compatibility check
The lookup callback format has been correctly adjusted to handle both Node.js 16 and 18's callback format.
Let's verify the usage of lookup callbacks across the codebase:
✅ Verification successful
LGTM: Node.js version compatibility is properly handled across the codebase
The verification confirms that the lookup callback implementation is consistent across the codebase:
- In
test/HttpClient.test.ts, the test correctly handles both Node.js 16/18 and newer versions - In
src/HttpAgent.ts, the implementation properly handles the address format differences across Node.js versions using spread operator and type-safe error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent lookup callback handling across versions
# Search for lookup callback implementations
ast-grep --pattern 'lookup($_, $_, $_) {
$$$
}'
Length of output: 51
Script:
#!/bin/bash
# Let's try a different approach to find lookup callback implementations
# Search for files containing 'lookup' function calls or implementations
rg -l "lookup.*callback" --type ts
# Then let's examine the content of these files
rg "lookup.*callback" --type ts -A 5 -B 5
Length of output: 1473
120-120: LGTM: Skip test for Node.js 16
Appropriately skips the "other side closed error" test for Node.js 16 where this behavior differs.
Let's verify if there are any similar tests that might need skipping:
✅ Verification successful
Let me gather more context about HTTP/2 related tests to ensure we have a complete picture.
Let me try a different search pattern to find related tests.
Based on the search results, I can now generate the final response:
Verified: Node.js 16 skip condition is correctly isolated
The "other side closed error" test is the only test that requires skipping for Node.js 16, as it's the only test case dealing with HTTP/2 GOAWAY frame behavior that differs in v16. No other similar tests need version-specific handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for similar HTTP/2 GOAWAY tests that might need version checks
# Search for HTTP/2 GOAWAY related tests
rg "GOAWAY.*frame" -A 5 -B 5
Length of output: 618
Script:
#!/bin/bash
# Search for HTTP/2 related test cases that might involve connection termination
rg "it\(.*http2.*|it\(.*GOAWAY.*|it\(.*closed.*" -A 2 test/HttpClient.test.ts
Length of output: 79
Script:
#!/bin/bash
# Search for test cases involving connection termination or HTTP/2
rg "it\(['\"].*(?i)(http2|closed|goaway)" -A 2 test/HttpClient.test.ts
# Also search for skipIf conditions to see similar patterns
rg "skipIf" -A 2 -B 2 test/HttpClient.test.ts
Length of output: 325
Summary by CodeRabbit
New Features
Bug Fixes
Tests