test, win: fix IPv6 detection on Windows #14865

bzoz · 2017-08-16T16:47:05Z

I've noticed that common.hasIPv6 is set to false on my Win box. This adds proper IPv6 detection on loopback devices on Windows.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

/cc @nodejs/testing

bzoz · 2017-08-16T16:47:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9689/

tniessen · 2017-08-16T23:02:48Z

cc @nodejs/platform-windows

bzoz · 2017-08-17T17:20:57Z

Turns out that 2 tests are failing now, which previously where skipped. The reason is the same as with #14111 - :: is used as server address, and Windows needs ::1. I've added a commit to this PR to fix this, PTAL

refack · 2017-08-17T17:31:23Z

test/async-hooks/test-graph.tcp.js

@@ -17,7 +17,7 @@ const server = net

 server.listen(common.PORT);

-net.connect({ port: server.address().port, host: server.address().address },
+net.connect({ port: server.address().port, host: '::1' },


I'd rather have this as a property of common like:
https://github.com/nodejs/node/blob/73894dad9d74671377c79dc7542b41813237c3cc/test/common/index.js#L200-L221

I'm +1 on this and +1 on fixing adress().adress, but this PR is about fixing common.hasIPv6. I would like to make just the test suit pass, and leave such improvements for other PRs.

Does localhost work? If so can we use that (see references in #14900 (comment)).

I don't think localhost will work because of #6307

Does that apply? Doesn't windows have an equivalent of /etc/hosts that it just looks up to find the address? I'm sure we're using localhost in a bunch of other tests.

But it will resolve to 127.0.0.1.
~~I'm trying to figure out if :: was just used for convenience, or if IPv6 has any significance.~~
I'm trying to figure out why this test has
https://github.com/nodejs/node/blob/73894dad9d74671377c79dc7542b41813237c3cc/test/async-hooks/test-graph.tcp.js#L4-L5

refack · 2017-08-17T17:35:43Z

Cross-ref: #14900

refack

::1 is easy enough to grep

refack · 2017-08-17T22:02:01Z

test/async-hooks/test-graph.tcp.js

@@ -17,7 +17,7 @@ const server = net

 server.listen(common.PORT);

-net.connect({ port: server.address().port, host: server.address().address },
+net.connect({ port: server.address().port, host: '::1' },


I don't think localhost will work because of #6307

refack · 2017-08-17T22:03:32Z

I opened #14900 for fixing server.address().address.
I'm convinced that ::1 is a good stop gap solution.

refack · 2017-08-18T22:55:48Z

@bzoz since I found test/async-hooks/test-graph.tcp.js to be confusing (and independent of IPv6), I've added a suggesting to clean it up. Obviously feel free to push it out.

bzoz · 2017-08-21T17:48:54Z

@refack I've pushed your commit out because I want to keep this simple. I've added a simple fix for the failing tests only to keep the CI green. With this PR I want to only fix IPv6 detection for tests.

gibfahn · 2017-08-21T17:58:05Z

@bzoz did you try localhost?

refack · 2017-08-21T19:10:39Z

@refack I've pushed your commit out because I want to keep this simple. I've added a simple fix for the failing tests only to keep the CI green. With this PR I want to only fix IPv6 detection for tests.

No problems.
I'll open a follow up PR.

refack

Still LGTM

Add proper IPv6 detection on loopback device on Windows. PR-URL: nodejs#14865 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

The 'test-graph.tcp' and 'test-tcpwrap' tests are using '::' as server address. This is mapped to localhost on Linux, but fails on Windows. PR-URL: nodejs#14865 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

bzoz · 2017-08-24T12:33:27Z

CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/9823/

Add proper IPv6 detection on loopback device on Windows. PR-URL: #14865 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

The 'test-graph.tcp' and 'test-tcpwrap' tests are using '::' as server address. This is mapped to localhost on Linux, but fails on Windows. PR-URL: #14865 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

bzoz · 2017-08-24T14:01:52Z

Landed in 1a0f19a, ca9b3f2

Fishrock123 · 2017-08-24T14:57:12Z

Looks like the last commit was pushed with some problematic ^M in the metadata:

test: fix async-hooks tests

The 'test-graph.tcp' and 'test-tcpwrap' tests are using '::' as server
address. This is mapped to localhost on Linux, but fails on Windows.

PR-URL: https://github.com/nodejs/node/pull/14865^MReviewed-By: James M Snell <jasnell@gmail.com>^MReviewed-By: Refael Ackermann <refack@gmail.com>

Fixing now. There will be a force-push to master!

The 'test-graph.tcp' and 'test-tcpwrap' tests are using '::' as server address. This is mapped to localhost on Linux, but fails on Windows. PR-URL: nodejs#14865 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Fishrock123 · 2017-08-24T14:58:57Z

Re-pushed as 88b8592, back to your regularly scheduled business! :)

Add proper IPv6 detection on loopback device on Windows. PR-URL: nodejs/node#14865 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

The 'test-graph.tcp' and 'test-tcpwrap' tests are using '::' as server address. This is mapped to localhost on Linux, but fails on Windows. PR-URL: nodejs/node#14865 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Add proper IPv6 detection on loopback device on Windows. PR-URL: nodejs/node#14865 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

The 'test-graph.tcp' and 'test-tcpwrap' tests are using '::' as server address. This is mapped to localhost on Linux, but fails on Windows. PR-URL: nodejs/node#14865 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Add proper IPv6 detection on loopback device on Windows. PR-URL: #14865 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

The 'test-graph.tcp' and 'test-tcpwrap' tests are using '::' as server address. This is mapped to localhost on Linux, but fails on Windows. PR-URL: #14865 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Add proper IPv6 detection on loopback device on Windows. PR-URL: #14865 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

The 'test-graph.tcp' and 'test-tcpwrap' tests are using '::' as server address. This is mapped to localhost on Linux, but fails on Windows. PR-URL: #14865 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

bzoz added test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform. labels Aug 16, 2017

nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 16, 2017

refack reviewed Aug 17, 2017

View reviewed changes

refack mentioned this pull request Aug 17, 2017

net: node on Linux allows opening a connection to 0.0.0.0/:: while on Windows it doesn't #14900

Closed

refack approved these changes Aug 17, 2017

View reviewed changes

bzoz force-pushed the bartek-win-test-ipv6 branch from df419d8 to 73894da Compare August 21, 2017 17:45

refack approved these changes Aug 21, 2017

View reviewed changes

jasnell approved these changes Aug 21, 2017

View reviewed changes

bzoz added 2 commits August 24, 2017 14:29

test, win: fix IPv6 detection on Windows

5e491e7

Add proper IPv6 detection on loopback device on Windows. PR-URL: nodejs#14865 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

test: fix async-hooks tests

59a2128

The 'test-graph.tcp' and 'test-tcpwrap' tests are using '::' as server address. This is mapped to localhost on Linux, but fails on Windows. PR-URL: nodejs#14865 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

bzoz force-pushed the bartek-win-test-ipv6 branch from 73894da to 59a2128 Compare August 24, 2017 12:31

bzoz added a commit that referenced this pull request Aug 24, 2017

test, win: fix IPv6 detection on Windows

ca9b3f2

Add proper IPv6 detection on loopback device on Windows. PR-URL: #14865 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

bzoz closed this Aug 24, 2017

jscissr mentioned this pull request Aug 24, 2017

doc,stream: remove wrong remark on readable.read #15014

Closed

2 tasks

MylesBorins pushed a commit that referenced this pull request Sep 10, 2017

test, win: fix IPv6 detection on Windows

17bd4f6

Add proper IPv6 detection on loopback device on Windows. PR-URL: #14865 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins pushed a commit that referenced this pull request Sep 12, 2017

test, win: fix IPv6 detection on Windows

4d6da3f

Add proper IPv6 detection on loopback device on Windows. PR-URL: #14865 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

MylesBorins added the dont-land-on-v6.x label Sep 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test, win: fix IPv6 detection on Windows #14865

test, win: fix IPv6 detection on Windows #14865

bzoz commented Aug 16, 2017

bzoz commented Aug 16, 2017

tniessen commented Aug 16, 2017

bzoz commented Aug 17, 2017

refack Aug 17, 2017

bzoz Aug 17, 2017

gibfahn Aug 17, 2017

refack Aug 17, 2017

gibfahn Aug 17, 2017

refack Aug 17, 2017 •

edited

Loading

refack commented Aug 17, 2017

refack left a comment

refack Aug 17, 2017

refack commented Aug 17, 2017

refack commented Aug 18, 2017

bzoz commented Aug 21, 2017

gibfahn commented Aug 21, 2017

refack commented Aug 21, 2017

refack left a comment

bzoz commented Aug 24, 2017

bzoz commented Aug 24, 2017

Fishrock123 commented Aug 24, 2017

Fishrock123 commented Aug 24, 2017

test, win: fix IPv6 detection on Windows #14865

test, win: fix IPv6 detection on Windows #14865

Conversation

bzoz commented Aug 16, 2017

Checklist

Affected core subsystem(s)

bzoz commented Aug 16, 2017

tniessen commented Aug 16, 2017

bzoz commented Aug 17, 2017

refack Aug 17, 2017

Choose a reason for hiding this comment

bzoz Aug 17, 2017

Choose a reason for hiding this comment

gibfahn Aug 17, 2017

Choose a reason for hiding this comment

refack Aug 17, 2017

Choose a reason for hiding this comment

gibfahn Aug 17, 2017

Choose a reason for hiding this comment

refack Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

refack commented Aug 17, 2017

refack left a comment

Choose a reason for hiding this comment

refack Aug 17, 2017

Choose a reason for hiding this comment

refack commented Aug 17, 2017

refack commented Aug 18, 2017

bzoz commented Aug 21, 2017

gibfahn commented Aug 21, 2017

refack commented Aug 21, 2017

refack left a comment

Choose a reason for hiding this comment

bzoz commented Aug 24, 2017

bzoz commented Aug 24, 2017

Fishrock123 commented Aug 24, 2017

Fishrock123 commented Aug 24, 2017

refack Aug 17, 2017 •

edited

Loading