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

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

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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]>^MReviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Add proper IPv6 detection on loopback device on Windows. PR-URL: nodejs/node#14865 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Add proper IPv6 detection on loopback device on Windows. PR-URL: #14865 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Add proper IPv6 detection on loopback device on Windows. PR-URL: #14865 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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

test/async-hooks/test-graph.tcp.js Outdated
Copy link
Contributor
refackAug 17, 2017
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think localhost will work because of #6307

bzoz force-pushed the bartek-win-test-ipv6 branch from df419d8 to 73894daCompare 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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

bzoz force-pushed the bartek-win-test-ipv6 branch from 73894da to 59a2128Compare 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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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

Uh oh!

test, win: fix IPv6 detection on Windows#14865

test, win: fix IPv6 detection on Windows #14865

Uh oh!

Conversation

bzoz commented Aug 16, 2017

Checklist

Affected core subsystem(s)

Uh oh!

bzoz commented Aug 16, 2017

Uh oh!

tniessen commented Aug 16, 2017

Uh oh!

bzoz commented Aug 17, 2017

Uh oh!

refackAug 17, 2017

Choose a reason for hiding this comment

Uh oh!

bzozAug 17, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahnAug 17, 2017

Choose a reason for hiding this comment

Uh oh!

refackAug 17, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahnAug 17, 2017

Choose a reason for hiding this comment

Uh oh!

refackAug 17, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 17, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refackAug 17, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 17, 2017

Uh oh!

refack commented Aug 18, 2017

Uh oh!

bzoz commented Aug 21, 2017

Uh oh!

gibfahn commented Aug 21, 2017

Uh oh!

refack commented Aug 21, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

bzoz commented Aug 24, 2017

Uh oh!

bzoz commented Aug 24, 2017

Uh oh!

Fishrock123 commented Aug 24, 2017

Uh oh!

Fishrock123 commented Aug 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

refackAug 17, 2017•
edited
Loading