test: make addresses of internet tests configurable by joyeecheung · Pull Request #16390 · nodejs/node · GitHub

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

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

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.

💯

LGTM with docs.

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..

(do we have one for the internet tests?)

Nope, added to #16390 (comment)

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

Do all these defaults now work behind the GFW?

@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).

How about coalescing the params?

How about defining a constant for 0.0.0.0 as well?

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'];

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?

@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/

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 <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #16390 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #16390 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#16390 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #16390 Backport-PR-URL: #19706 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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

	let localhostIPv4 = null;

	exports.localIPv6Hosts = ['localhost'];

test: make addresses of internet tests configurable by joyeecheung · Pull Request #16390 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: make addresses of internet tests configurable - #16390

test: make addresses of internet tests configurable#16390joyeecheung wants to merge 2 commits intonodejs:masternodejs/node:masterfrom joyeecheung:inet-varjoyeecheung/node:inet-varCopy head branch name to clipboard

Conversation

joyeecheung commented Oct 22, 2017

Checklist

Affected core subsystem(s)

Uh oh!

joyeecheung commented Oct 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack Oct 22, 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!

Choose a reason for hiding this comment

Uh oh!

joyeecheung Oct 23, 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 left a comment

Choose a reason for hiding this comment

Uh oh!

gibfahn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Oct 22, 2017

Uh oh!

gibfahn commented Oct 22, 2017

Uh oh!

joyeecheung commented Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Nov 8, 2017

Uh oh!

MylesBorins commented Nov 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

test: make addresses of internet tests configurable#16390
joyeecheung wants to merge 2 commits into
nodejs:masterfrom
joyeecheung:inet-var

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