test: make addresses of internet tests configurable #16390

joyeecheung · 2017-10-22T18:45:04Z

This PR makes the addresses used by the internet tests configurable (via NODE_TEST_* environment variables), so it's easier to verify them within a firewall.

Also this moves most google.com references to nodejs.org.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

joyeecheung · 2017-10-22T18:46:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10909/ (do we have one for the internet tests?)

joyeecheung · 2017-10-22T18:48:45Z

test/common/dns.js

@@ -287,4 +287,51 @@ function writeDNSPacket(parsed) {
  }));
 }

-module.exports = { types, classes, writeDNSPacket, parseDNSPacket };
+
+const addresses = {


I am not quite sure if common/dns is the best place to put this. Also pending docs.

If it’s specific to test/internet, it might be better live in a file there (similar to what test/async-hooks has)?

I'm not 100% sure about that... On the one hand, I'd rather see all "harness" code in common, on the other hand this should not be used by any other suite.
/ping @Trott & @nodejs/testing

Alternatively just add another module, test/common/internet.js

I'm not 100% sure about that... On the one hand, I'd rather see all "harness" code in common, on the other hand this should not be used by any other suite.

I think it's not a terrible bad idea to replace other google.com references in parallel tests..also when my ISP DNS hijacks NXDOMAIN there are constant errors from parallel/test-net-connect-immediate-finish and parallel/test-net-better-error-messages-port-hostname, it would be helpful to replace those this.hostname.is.invalid with *** configured via environment variables.

refack

💯

gibfahn

LGTM with docs.

gibfahn · 2017-10-22T21:43:39Z

test/internet/test-dns-cares-domains.js

@@ -20,8 +21,8 @@ const methods = [
 methods.forEach(function(method) {
  const d = domain.create();
  d.run(function() {
-    dns[method]('google.com', function() {
+    dns[method](addresses.INET_HOST, common.mustCall(() => {


Improving the test at the same time? Nice!

@gibfahn There are so many missing common.mustCall in the internet tests...better leave it to other PRs but this is in a loop, so I thought why not :D..

gibfahn · 2017-10-22T21:50:25Z

(do we have one for the internet tests?)

Nope, added to #16390 (comment)

gibfahn · 2017-10-22T21:52:08Z

so it's easier to verify them within a firewall.

Do all these defaults now work behind the GFW?

joyeecheung · 2017-10-23T03:55:31Z

@gibfahn In my case it works reliably when using the 114 DNS(114.114.114.114) or the Google Public DNS(8.8.8.8, I am surprised to find that it is accessible from my ISP again). Other popular public DNS behind GFW like AliDNS and DNSPod DNS all miss some records tested by test-dns.js from time to time.

With the DNS servers designated by my ISP it never works, I will have to set up those env vars using hosts behind GFW. DNS hijacking is very common behind GFW and the situation varies among different ISP and regions. Also, setting up the system to use public DNS servers does not guarantee you are actually connecting to their servers, some ISP hijacks requests to popular public DNS servers (there are traceroute reports showing that from time to time).

(FWIW, I am with China Mobile in Hangzhou when I work on this PR).

gireeshpunathil · 2017-10-23T05:27:44Z

test/internet/test-dns.js


 req.oncomplete = function(err, domains) {
  assert.strictEqual(err, 0);
-  console.log('nodejs.org = ', domains);
+  console.log(`${addresses.INET4_HOST} = `, domains);


How about coalescing the params?

gireeshpunathil · 2017-10-23T05:28:49Z

test/internet/test-dns.js


-assert.doesNotThrow(() => dns.lookupService('0.0.0.0', '0', common.mustCall()));
+assert.doesNotThrow(() =>
+  dns.lookupService('0.0.0.0', '0', common.mustCall()));


How about defining a constant for 0.0.0.0 as well?

gireeshpunathil · 2017-10-23T05:29:24Z

test/internet/test-dns-ipv6.js

+    common.mustCall((err, ip, family) => {
+      assert.ifError(err);
+      assert.ok(isIPv6(ip));
+      assert.strictEqual(family, 6);



How about defining a constant for ::1 as well?

localhost is defined for IPv4 and IPv6:

node/test/common/index.js

Lines 183 to 185 in 27e12e7

let localhostIPv4 = null;

exports.localIPv6Hosts = ['localhost'];

gireeshpunathil · 2017-10-23T05:34:01Z

test/common/dns.js

+  // An accessible IPv4 DNS server
+  DNS4_SERVER: '8.8.8.8'
+};
+


I am not sure about the locality and proximity of our test systems w.r.t the system that hosts http://nodejs.org . If they are too close / under one subnet etc. how some of the tests (like dns resolution, pings, downloads etc.) are going to respond in terms of network level operations - will there be any bypasses?

joyeecheung · 2017-11-06T13:57:56Z

@refack @addaleax @gibfahn @gireeshpunathil I've move the addresses to a new module test/common/internet.js and added documentation, PTAL.

I'll defer the other constants e.g. 0.0.0.0 to another PR (looks like a good first issue).

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

Trott · 2017-11-07T11:26:34Z

test/common/README.md

@@ -498,6 +498,40 @@ Returns the result of
 Returns the result of
 `fs.readFileSync(path.join(fixtures.fixturesDir, 'keys', arg), 'enc')`.

+## Internet Module


Nit: Can you please add an entry for this new module to the table of contents near the start of this file?

@Trott Of course!

This commit introduces test/common/internet.address, which includes a set of addresses for doing internet tests. These addresses can be overriden using NODE_TEST_* environment variables.

This commit introduces test/common/internet.address, which includes a set of addresses for doing internet tests. These addresses can be overriden using NODE_TEST_* environment variables. PR-URL: #16390 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #16390 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

joyeecheung · 2017-11-08T11:09:21Z

Landed in f2cb78c...5dca787, thanks!

This commit introduces test/common/internet.address, which includes a set of addresses for doing internet tests. These addresses can be overriden using NODE_TEST_* environment variables. PR-URL: #16390 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #16390 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2017-11-17T15:26:21Z

Should this be backported to v6.x-staging or v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

This commit introduces test/common/internet.address, which includes a set of addresses for doing internet tests. These addresses can be overriden using NODE_TEST_* environment variables. PR-URL: nodejs#16390 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#16390 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

This commit introduces test/common/internet.address, which includes a set of addresses for doing internet tests. These addresses can be overriden using NODE_TEST_* environment variables. PR-URL: #16390 Backport-PR-URL: #19706 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #16390 Backport-PR-URL: #19706 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

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

joyeecheung commented Oct 22, 2017

View reviewed changes

addaleax approved these changes Oct 22, 2017

View reviewed changes

refack approved these changes Oct 22, 2017

View reviewed changes

gibfahn approved these changes Oct 22, 2017

View reviewed changes

gibfahn mentioned this pull request Oct 22, 2017

Add --without-intl builds to CI matrix nodejs/build#419

Closed

cjihrig approved these changes Oct 22, 2017

View reviewed changes

gireeshpunathil reviewed Oct 23, 2017

View reviewed changes

joyeecheung force-pushed the inet-var branch from 7eb6421 to 915c4f8 Compare November 6, 2017 13:55

gibfahn approved these changes Nov 6, 2017

View reviewed changes

Trott reviewed Nov 7, 2017

View reviewed changes

jasnell approved these changes Nov 8, 2017

View reviewed changes

joyeecheung added 2 commits November 8, 2017 18:34

test: introduce test/common/internet.addresses

34c18d3

This commit introduces test/common/internet.address, which includes a set of addresses for doing internet tests. These addresses can be overriden using NODE_TEST_* environment variables.

test: use internet.addresses in internet tests

b28c152

joyeecheung force-pushed the inet-var branch 2 times, most recently from 49e706b to b28c152 Compare November 8, 2017 10:35

joyeecheung closed this Nov 8, 2017

evanlucas mentioned this pull request Nov 13, 2017

v9.2.0 proposal #16992

Merged

MylesBorins added the backport-requested-v6.x label Nov 17, 2017

MylesBorins added the backport-requested-v8.x label Nov 17, 2017

joyeecheung added dont-land-on-v6.x and removed backport-requested-v6.x labels Jan 24, 2018

gibfahn mentioned this pull request Feb 27, 2018

internet/test-dns fails on v6.x #19032

Closed

joyeecheung mentioned this pull request Mar 30, 2018

[v8.x backport] 16390, 18547, 17296 (internet test flake fixes) #19706

Closed

gibfahn added backported-to-v8.x and removed dont-land-on-v8.x labels Apr 12, 2018

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

richardlau mentioned this pull request Mar 6, 2019

test: de-flake test-dns-idna2008.js #26473

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: make addresses of internet tests configurable #16390

test: make addresses of internet tests configurable #16390

joyeecheung commented Oct 22, 2017

joyeecheung commented Oct 22, 2017

joyeecheung Oct 22, 2017

addaleax Oct 22, 2017

refack Oct 22, 2017 •

edited

Loading

gibfahn Oct 22, 2017

joyeecheung Oct 23, 2017 •

edited

Loading

refack left a comment

gibfahn left a comment

gibfahn Oct 22, 2017

joyeecheung Oct 23, 2017

gibfahn commented Oct 22, 2017

gibfahn commented Oct 22, 2017

joyeecheung commented Oct 23, 2017 •

edited

Loading

gireeshpunathil Oct 23, 2017

gireeshpunathil Oct 23, 2017

gireeshpunathil Oct 23, 2017

gibfahn Oct 23, 2017

gireeshpunathil Oct 23, 2017

joyeecheung commented Nov 6, 2017 •

edited

Loading

Trott Nov 7, 2017

joyeecheung Nov 7, 2017

joyeecheung commented Nov 8, 2017

MylesBorins commented Nov 17, 2017

	let localhostIPv4 = null;

	exports.localIPv6Hosts = ['localhost'];

test: make addresses of internet tests configurable #16390

test: make addresses of internet tests configurable #16390

Conversation

joyeecheung commented Oct 22, 2017

Checklist

Affected core subsystem(s)

joyeecheung commented Oct 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack Oct 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

gibfahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn commented Oct 22, 2017

gibfahn commented Oct 22, 2017

joyeecheung commented Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Nov 8, 2017

MylesBorins commented Nov 17, 2017

refack Oct 22, 2017 •

edited

Loading

joyeecheung Oct 23, 2017 •

edited

Loading

joyeecheung commented Oct 23, 2017 •

edited

Loading

joyeecheung commented Nov 6, 2017 •

edited

Loading