test(logger): add e2e test for ENOTFOUND error log#1233
Conversation
📝 WalkthroughWalkthroughNarrows a cast in the logger plugin so ChangesProxy Error Logger
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
🤖 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 `@src/plugins/default/logger-plugin.ts`:
- Line 29: The log builds targetHref unsafely by casting target to URL, causing
the string "undefined" when target is absent; change the derivation to safely
check the payload: if target is an instance of URL use target.href, else if
target is an object with a href property coerce that to string, otherwise use an
empty string (or "unknown") — update the expression where targetHref is defined
(the const targetHref in logger-plugin.ts) to perform these runtime checks
instead of an unchecked cast.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2df43af5-0677-4198-906f-a9c5db46e73d
📒 Files selected for processing (2)
src/plugins/default/logger-plugin.tstest/e2e/logger.spec.ts
chimurai
force-pushed
the
test-logger-e2e-ENOTFOUND
branch
from
39cb266 to
abacb50
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/plugins/default/logger-plugin.ts (1)
29-29:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winTest failures confirm
targetis undefined during ENOTFOUND errors.The pipeline failures show the test receives
"undefined"instead of"http://does-not-exist.invalid"in the logged error message. This confirms the existing review comment: whentargetis undefined, the cast toURLand optional chaining produceundefined, which stringifies to"undefined"in the template literal. The fix should fall back tooptions.targetwhen thetargetparameter is absent.🤖 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 `@src/plugins/default/logger-plugin.ts` at line 29, The logged targetHref is becoming the string "undefined" because (target as unknown as URL)?.href yields undefined when target is missing; change construction of targetHref to use options.target as a fallback (e.g., derive a string from target?.href || options.target) so that when target is undefined the log uses options.target instead; update the code that defines targetHref (referencing the target variable and options.target) to coalesce to a sensible string rather than allowing "undefined".
🤖 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.
Duplicate comments:
In `@src/plugins/default/logger-plugin.ts`:
- Line 29: The logged targetHref is becoming the string "undefined" because
(target as unknown as URL)?.href yields undefined when target is missing; change
construction of targetHref to use options.target as a fallback (e.g., derive a
string from target?.href || options.target) so that when target is undefined the
log uses options.target instead; update the code that defines targetHref
(referencing the target variable and options.target) to coalesce to a sensible
string rather than allowing "undefined".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0844273-3afa-4756-ae64-7ae490c8dfad
📒 Files selected for processing (2)
src/plugins/default/logger-plugin.tstest/e2e/logger.spec.ts
chimurai
force-pushed
the
test-logger-e2e-ENOTFOUND
branch
from
abacb50 to
0ef8c6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e/logger.spec.ts`:
- Around line 33-37: The test fails because the logger's targetHref is becoming
undefined for string targets; update the logger code that computes targetHref
(used by format(message, requestHref, targetHref, errorCode, errorReference)) so
it properly resolves string targets into a concrete URL: detect when the proxy
target is a string and coerce it to a full URL (e.g., use new URL(targetString,
requestHref) or targetString.toString()) or otherwise serialize target objects
into an absolute href, then pass that resulting string into targetHref so the
snapshot assertion shows the real URL instead of undefined.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a8f164a-6728-4336-ba91-cf8d4d6015a9
📒 Files selected for processing (2)
src/plugins/default/logger-plugin.tstest/e2e/logger.spec.ts
✅ Files skipped from review due to trivial changes (1)
- src/plugins/default/logger-plugin.ts
| expect( | ||
| format(message, requestHref, targetHref, errorCode, errorReference), | ||
| ).toMatchInlineSnapshot( | ||
| `"[HPM] Error occurred while proxying request example.test/my-path to http://does-not-exist.invalid/ [ENOTFOUND] (https://nodejs.org/api/errors.html#errors_common_system_errors)"`, | ||
| ); |
There was a problem hiding this comment.
CI is red because targetHref resolves to undefined; fix logger target URL resolution before merge.
This assertion is currently failing across runtimes (to undefined), which indicates the logger path is not deriving targetHref correctly for this proxy target. Keep this expectation (it captures the intended behavior) and fix the logger implementation so string targets log a concrete URL.
🧰 Tools
🪛 GitHub Check: Coverage
[failure] 35-35: test/e2e/logger.spec.ts > logger > should log target ENOTFOUND errors
Error: Snapshot logger > should log target ENOTFOUND errors 1 mismatched
Expected: ""[HPM] Error occurred while proxying request example.test/my-path to http://does-not-exist.invalid/ [ENOTFOUND] (https://nodejs.org/api/errors.html#errors_common_system_errors)""
Received: ""[HPM] Error occurred while proxying request example.test/my-path to undefined [ENOTFOUND] (https://nodejs.org/api/errors.html#errors_common_system_errors)""
❯ test/e2e/logger.spec.ts:35:7
🪛 GitHub Check: Test (22.x)
[failure] 35-35: test/e2e/logger.spec.ts > logger > should log target ENOTFOUND errors
Error: Snapshot logger > should log target ENOTFOUND errors 1 mismatched
Expected: ""[HPM] Error occurred while proxying request example.test/my-path to http://does-not-exist.invalid/ [ENOTFOUND] (https://nodejs.org/api/errors.html#errors_common_system_errors)""
Received: ""[HPM] Error occurred while proxying request example.test/my-path to undefined [ENOTFOUND] (https://nodejs.org/api/errors.html#errors_common_system_errors)""
❯ test/e2e/logger.spec.ts:35:7
🪛 GitHub Check: Test (24.x)
[failure] 35-35: test/e2e/logger.spec.ts > logger > should log target ENOTFOUND errors
Error: Snapshot logger > should log target ENOTFOUND errors 1 mismatched
Expected: ""[HPM] Error occurred while proxying request example.test/my-path to http://does-not-exist.invalid/ [ENOTFOUND] (https://nodejs.org/api/errors.html#errors_common_system_errors)""
Received: ""[HPM] Error occurred while proxying request example.test/my-path to undefined [ENOTFOUND] (https://nodejs.org/api/errors.html#errors_common_system_errors)""
❯ test/e2e/logger.spec.ts:35:7
🪛 GitHub Check: Test (26.x)
[failure] 35-35: test/e2e/logger.spec.ts > logger > should log target ENOTFOUND errors
Error: Snapshot logger > should log target ENOTFOUND errors 1 mismatched
Expected: ""[HPM] Error occurred while proxying request example.test/my-path to http://does-not-exist.invalid/ [ENOTFOUND] (https://nodejs.org/api/errors.html#errors_common_system_errors)""
Received: ""[HPM] Error occurred while proxying request example.test/my-path to undefined [ENOTFOUND] (https://nodejs.org/api/errors.html#errors_common_system_errors)""
❯ test/e2e/logger.spec.ts:35:7
🤖 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/e2e/logger.spec.ts` around lines 33 - 37, The test fails because the
logger's targetHref is becoming undefined for string targets; update the logger
code that computes targetHref (used by format(message, requestHref, targetHref,
errorCode, errorReference)) so it properly resolves string targets into a
concrete URL: detect when the proxy target is a string and coerce it to a full
URL (e.g., use new URL(targetString, requestHref) or targetString.toString()) or
otherwise serialize target objects into an absolute href, then pass that
resulting string into targetHref so the snapshot assertion shows the real URL
instead of undefined.
1 participant
add missing e2e logger test for ENOTFOUND
related: unjs/httpxy#135
Summary by CodeRabbit
Refactor
Tests