Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v18.x - test-dns-ipv6 fails in 18.17.0 and later #49203

Closed
mhdawson opened this issue Aug 16, 2023 · 7 comments
Closed

v18.x - test-dns-ipv6 fails in 18.17.0 and later #49203

mhdawson opened this issue Aug 16, 2023 · 7 comments
Labels
net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.

Comments

@mhdawson
Copy link
Member

Version

v18.17.0

Platform

at least linux

Subsystem

net

What steps will reproduce the bug?

tools/test.py test/internet/test-dns-ipv6

How often does it reproduce? Is there a required condition?

100%

What is the expected behavior? Why is that the expected behavior?

Test passes

What do you see instead?

Test passes

midawson@drx-hemera io.js]$ tools/test.py test/internet/test-dns-ipv6
=== release test-dns-ipv6 ===                   
Path: internet/test-dns-ipv6
test_resolve6
test_reverse_ipv6
node:internal/errors:496
    ErrorCaptureStackTrace(err);
    ^

Error: getHostByAddr EINVAL 2001:4860:4860::8888
    at node:internal/dns/promises:280:14
    at new Promise (<anonymous>)
    at createResolverPromise (node:internal/dns/promises:267:10)
    at ResolverBase.getHostByAddr (node:internal/dns/promises:299:12)
    at test_reverse_ipv6 (/home/midawson/newpull/io.js/backports/io.js/test/internet/test-dns-ipv6.js:72:36)
    at next (/home/midawson/newpull/io.js/backports/io.js/test/internet/test-dns-ipv6.js:22:7)
    at process.processTicksAndRejections (node:internal/process/task_queues:77:11) {
  errno: -22,
  code: 'EINVAL',
  syscall: 'getHostByAddr',
  hostname: '2001:4860:4860::8888'
}

Additional information

No response

@mhdawson
Copy link
Member Author

mhdawson commented Aug 16, 2023

I did some bisection as part of #49183 which was what led me to open this issue.

@mhdawson
Copy link
Member Author

Using git bisect it seems to indicate that it was this PR that causes the failure:

0dc485eb28fb05eafad4dc78c646d917d8d4024a is the first bad commit
commit 0dc485eb28fb05eafad4dc78c646d917d8d4024a
Author: Yagiz Nizipli <yagiz@nizipli.com>
Date:   Fri Mar 31 09:04:03 2023 -0400

    url: drop ICU requirement for parsing hostnames
    
    PR-URL: https://github.com/nodejs/node/pull/47339
    Backport-PR-URL: https://github.com/nodejs/node/pull/48345
    Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
    Reviewed-By: Rich Trott <rtrott@gmail.com>

 deps/ada/ada.gyp                     | 19 ++-----------------
 doc/api/url.md                       | 25 +++++++++++++++----------
 lib/internal/idna.js                 |  9 ++-------
 test/benchmark/test-benchmark-url.js | 14 +-------------
 test/wpt/status/url.json             | 21 ---------------------
 5 files changed, 20 insertions(+), 68 deletions(-)

@mhdawson
Copy link
Member Author

@anonrig can you take a look at the backport and see if you can spot why it might be causing that test to fail?

@anonrig
Copy link
Member

anonrig commented Aug 16, 2023

We had a similar issue in the past: #48262. Might be related to it. I'll take a look at it soon.

I think we should backport these 2 pull requests as well:

@anonrig
Copy link
Member

anonrig commented Aug 16, 2023

I think this is caused by calling domainToASCII instead of ada::idna::to_ascii.

https://github.com/nodejs/node/blob/v18.x-staging/lib/internal/idna.js#L3

mhdawson added a commit to mhdawson/io.js that referenced this issue Aug 17, 2023
Refs: nodejs#49203

Changes slipped into v18.x regressed
test/internet/test-dns-ipv6 as I assume the action did
not run because no test under test/internet was changed.
Add some of the common paths that include code that might
introduce failures in the internet tests.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
@anonrig
Copy link
Member

anonrig commented Aug 18, 2023

Does this issue persist with the v18 proposal?

nodejs-github-bot pushed a commit that referenced this issue Aug 19, 2023
Refs: #49203

Changes slipped into v18.x regressed
test/internet/test-dns-ipv6 as I assume the action did
not run because no test under test/internet was changed.
Add some of the common paths that include code that might
introduce failures in the internet tests.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
PR-URL: #49218
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@VoltrexKeyva VoltrexKeyva added net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests. labels Aug 22, 2023
UlisesGascon pushed a commit that referenced this issue Sep 10, 2023
Refs: #49203

Changes slipped into v18.x regressed
test/internet/test-dns-ipv6 as I assume the action did
not run because no test under test/internet was changed.
Add some of the common paths that include code that might
introduce failures in the internet tests.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
PR-URL: #49218
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@richardlau
Copy link
Member

This was fixed in Node.js 18.18.0.

targos pushed a commit that referenced this issue Nov 27, 2023
Refs: #49203

Changes slipped into v18.x regressed
test/internet/test-dns-ipv6 as I assume the action did
not run because no test under test/internet was changed.
Add some of the common paths that include code that might
introduce failures in the internet tests.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
PR-URL: #49218
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
Refs: nodejs/node#49203

Changes slipped into v18.x regressed
test/internet/test-dns-ipv6 as I assume the action did
not run because no test under test/internet was changed.
Add some of the common paths that include code that might
introduce failures in the internet tests.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
PR-URL: nodejs/node#49218
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
Refs: nodejs/node#49203

Changes slipped into v18.x regressed
test/internet/test-dns-ipv6 as I assume the action did
not run because no test under test/internet was changed.
Add some of the common paths that include code that might
introduce failures in the internet tests.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
PR-URL: nodejs/node#49218
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

4 participants