test: avoid connecting to :: in test-net-timeout

[gibfahn]

Checklist

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

Affected core subsystem(s)

test

Description of change

Fixes #7291

This test tries to connect to :: or 0.0.0.0, which resolves to localhost
on some platforms but not others. Change to specify localhost.

@quaidn I basically did what you suggested in #7291 (comment), but used common.hasIPv6 instead.

@cjihrig I don't think this will make a difference to the changes you made to gc/test-net-timeout.js a few days ago, but I'd appreciate a check.

cc @Trott

EDIT: I would have replaced server.address().address with 'localhost', but until #7288 is resolved that can cause problems on some machines.

EDIT 2: Actually you can use localhost, as if you don't specify IPv4 or IPv6 localhost should resolve to 127.0.0.1 if ::1 isn't defined.

[gibfahn]

@bnoordhuis c.f. #7728 (comment)

Connecting to :: works on some platforms but not others and is a somewhat nonsensical thing to do. Tests that do that should be fixed to connect to the actual server address (i.e., server.address().address.)

EDIT: 'Nonsensical' is perhaps a poor choice of words. One way to think of it is as asking for a connection to any local address that is accepting connections on the selected port. That is meaningful and sensible but alas, not very portable.

This test does currently connect to server.address().address, but as the server is listening on all addresses, server.address().address returns ::.

[bnoordhuis]

Can you either use common.localhostIPv4 here or (more succinctly) make the server listen on that address?

[gibfahn]

@bnoordhuis do you want

  var host = common.localhostIPv4
else
  var host = common.localhostIPv6

or should we just only use IPv4 here?

[bnoordhuis]

My preference is for this:

diff --git a/test/gc/test-net-timeout.js b/test/gc/test-net-timeout.js
index 2645ccc..c050ddb 100644
--- a/test/gc/test-net-timeout.js
+++ b/test/gc/test-net-timeout.js
@@ -29,7 +29,7 @@ let countGC = 0;
 console.log('We should do ' + todo + ' requests');

 var server = net.createServer(serverHandler);
-server.listen(0, getall);
+server.listen(0, common.localhostIPv4, getall);

 function getall() {
   if (count >= todo)

BTW, there is no common.localhostIPv6.

[gibfahn]

Tried this, failing with TypeError: Cannot read property 'port' of null

diff --git a/test/gc/test-net-timeout.js b/test/gc/test-net-timeout.js
index 2645ccc..d129bd0 100644
--- a/test/gc/test-net-timeout.js
+++ b/test/gc/test-net-timeout.js
@@ -19,7 +19,7 @@ function serverHandler(sock) {

 const net = require('net');
 const weak = require('weak');
-require('../common');
+const common = require('../common');
 const assert = require('assert');
 const todo = 500;
 let done = 0;
@@ -29,7 +29,7 @@ let countGC = 0;
 console.log('We should do ' + todo + ' requests');

 var server = net.createServer(serverHandler);
-server.listen(0, getall);
+server.listen(0, common.localhostIPv4, getall);

 function getall() {
   if (count >= todo)

EDIT: how do you get syntax highlighting on diffs? ```diff, worked it out

[gibfahn]

@bnoordhuis also do we need to specify a port (i.e. server.listen(common.PORT, common.localhostIPv4, getall);)?

/tmp/gib/node/test/gc/test-net-timeout.js:38
  const req = net.connect(server.address().port, server.address().address);
                                          ^

TypeError: Cannot read property 'port' of null
    at getall (/tmp/gib/node/test/gc/test-net-timeout.js:38:43)

[bnoordhuis]

Left a comment. I wonder, is this the only test with that problem?

[thefourtheye]

Indentation is not right. Also, if we are doing this ternary operator is okay I guess

[cjihrig]

If you're touching this line, it might be worthwhile to move it to the top of the file.

[gibfahn]

Okay, will do. EDIT: done

[Trott]

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

[gibfahn]

@Trott I don't think the test-ci job runs the gc test suite.

[Trott]

@gibfahn You're right. The only useful part of the CI run for this is the linter. :-|

[gibfahn]

Thanks @Trott , unfortunately the test is still failing for me with:

  const req = net.connect(server.address().port, server.address().address);
                                          ^

TypeError: Cannot read property 'port' of null
    at getall (C:\Users\gib\node\test\gc\test-net-timeout.js:39:43)
    at Object.<anonymous> (C:\Users\gib\node\test\gc\test-net-timeout.js:54:3)

[Trott]

@gibfahn The for loop that calls getAll() ten times in a row is running before the server is bound to the port, and that's causing it to fail. Not sure why it doesn't fail without the changes in this test, but I think that for loop might be superfluous. You can remove it or you can add a check inside getAll() to see if the server.address() returns null or not. Not sure if one or both approaches messes up what the test is supposed to test, though.

/cc (based on line counts in git blame, not the best approach, but hopefully not the worst either) @isaacs @cjihrig @targos @piscisaureus @tjfontaine @nodejs/testing

[gibfahn]

Fixed by #10854