Skip to content

test: enable parallel/test-dns-lookup-promises-options-deprecated.js#33710

Merged
nathanwhit merged 1 commit into
denoland:mainfrom
nathanwhitbot:fix/node-compat-iter74
Apr 29, 2026
Merged

test: enable parallel/test-dns-lookup-promises-options-deprecated.js#33710
nathanwhit merged 1 commit into
denoland:mainfrom
nathanwhitbot:fix/node-compat-iter74

Conversation

@nathanwhitbot
Copy link
Copy Markdown
Contributor

Summary

Enables parallel/test-dns-lookup-promises-options-deprecated.js. The test stubs out internalBinding('cares_wrap').getaddrinfo to force a UV_ENOMEM, then asserts dnsPromises.lookup(addr, { hints: '-1' }) rejects with ERR_INVALID_ARG_TYPE before the lookup is dispatched. Deno already validates hints correctly via the internalBinding('cares_wrap') polyfill exposed by internal/test/binding, so the test passes without code changes.

No production code touched — just one tests/node_compat/config.jsonc entry.

Test plan

  • Verified passing 3/3 times via the node_compat test runner
  • Full cargo test --test node_compat run on Linux confirms no regressions (only pre-existing flaky failures)

@nathanwhitbot nathanwhitbot force-pushed the fix/node-compat-iter74 branch from 7450e31 to 9adb10b Compare April 29, 2026 19:06
@nathanwhitbot
Copy link
Copy Markdown
Contributor Author

Force-pushed 9adb10b58 — branch was stacked on #33653 as a base. Reset to upstream/main and cherry-picked just the 1-test commit. Diff is now 1 file, +1 -0.

Copy link
Copy Markdown
Contributor Author

@nathanwhitbot nathanwhitbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review of `9adb10b58` (post-cleanup):

Pure config-only test enrollment, no production code. Single entry alphabetically positioned correctly:

  • Insertion point: between the existing TODO-commented `test-dns-lookup-promises.js` block and `test-dns-lookup-promises-options-deprecated.js` (which test name) and `test-dns-lookupService.js`.

Body claim verified: this test stubs `internalBinding('cares_wrap').getaddrinfo` to force `UV_ENOMEM` and asserts `dnsPromises.lookup(addr, { hints: '-1' })` rejects with `ERR_INVALID_ARG_TYPE` before the lookup is dispatched — which means the validation runs entirely in the JS layer (`hints` validator in `internal/dns/utils`) without ever reaching the binding stub. Deno's polyfill validates `hints` correctly, so this passes without code changes. ✓

Note that the related `test-dns-lookup.js` and `test-dns-lookup-promises.js` are still TODO-commented above with a documented reason ("spurious punycode DEP0040 warning and getaddrinfo error mismatch") — leaving those as-is is correct; this PR only enables the one test that does pass.

Holding to COMMENT until CI confirms.

Copy link
Copy Markdown
Contributor Author

@nathanwhitbot nathanwhitbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single-commit, 1-line config-only enrollment. No scope-creep. LGTM — PR description accurately describes the test's behavior (stubs cares_wrap, expects ERR_INVALID_ARG_TYPE); pre-merge confirmation that Windows/macOS pass would be standard.

Copy link
Copy Markdown
Contributor

@fibibot fibibot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on substance. Pure 1-line config enrollment, alphabetically placed correctly:

  • test-dns-lookup-promises-options-deprecated.js slots between the commented-out test-dns-lookup-promises.js (immediate prefix) and test-dns-lookupService.js. ASCII comparison: lookup-promises-options-deprecated < lookupS... because - (0x2D) < S (0x53). ✓

The PR body's diagnosis checks out — the test stubs internalBinding('cares_wrap').getaddrinfo to force UV_ENOMEM, then asserts dnsPromises.lookup(addr, { hints: '-1' }) rejects with ERR_INVALID_ARG_TYPE before the stubbed lookup is reached. Deno's existing hints validation in the cares_wrap polyfill rejects the bad value upfront, so the test passes without any code change. The internal/test/binding exposure (added by #33673's internal/validators work) is what makes the stub-injection path work.

CI just kicked off (1 of many shards reporting). Holding to COMMENT per the CI-must-complete rule, but this is a config-only change that can't substantively fail — happy to flip to APPROVE on the next iteration once test node_compat debug × 6 platforms confirms green.

@nathanwhit nathanwhit merged commit 592a9c4 into denoland:main Apr 29, 2026
136 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants