test: handle IPv6 localhost issues within tests #7766

Trott · 2016-07-16T20:46:38Z

Checklist

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

Affected core subsystem(s)

test https tls

Description of change

The issue of hosts that do not resolve localhost to ::1 is now
handled within the tests. Remove flaky status for
test-https-connect-address-family and test-tls-connect-address-family.

The issue of hosts that do not resolve `localhost` to `::1` is now handled within the tests. Remove flaky status for test-https-connect-address-family and test-tls-connect-address-family.

Trott · 2016-07-16T21:10:17Z

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

Trott · 2016-07-19T00:26:11Z

CI all green except the hanging plinux machines.

Trott · 2016-07-19T00:26:37Z

/cc @nodejs/testing

thefourtheye · 2016-07-19T07:13:35Z

test/parallel/test-https-connect-address-family.js

-  if (err)
+  if (err) {
+    if (err.code === 'ENOTFOUND') {
+      common.skip('localhost does not resolve to ::1');


In my Ubuntu machines, I had to manually change the /etc/hosts to make these tests pass. Perhaps if localhost fails, we should give it another try with ipv6-localhost instead of skipping.

The initial version of this test did that (it tried common.localIPv6Hosts in order) but it was actually making the test fail on some of the buildbots...

@thefourtheye That's true with or without the change here, right? Like, without the change here, the test fails (but it is marked as passing if run with test.py or make test because of the setting in parallel.status). With the change here, the test is skipped. In both cases, you need to change /etc/hosts to get the test to pass (running from the command line without the test runner). Or am I mistaken? If I'm not mistaken, then I think this is an improvement. (Skipping is better than wrongly failing.)

@bnoordhuis I think we might be able to go back to trying to resolve addresses in common.localIPv6Hosts once this fix is in there, but I'm not 100% sure. Either way, I'd rather see this (hopefully clear improvement) land, and only then experiment with adding that functionality back in to see if it works any better once this safeguard is there.

Do we have any failure instances to look at? Could the flakiness because of the timeout? I have a feeling that this could be properly fixed.

@thefourtheye It's not really flaky. It's fail-every-time. The only reason CI has been green anyway is because of the setting in parallel.status that basically says "if this test fails, mark it as passing anyway". It's that setting that I'm trying to get rid of in this change.

Example: https://ci.nodejs.org/job/node-test-commit-linux/4243/nodes=centos5-32/console

Note that it's green despite the existence of this:

not ok 560 parallel/test-https-connect-address-family # TODO : Fix flaky test # /home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/test/parallel/test-https-connect-address-family.js:40 # throw err; # ^ # # Error: getaddrinfo ENOTFOUND localhost # at errnoException (dns.js:28:10) # at GetAddrInfoReqWrap.onlookupall [as oncomplete] (dns.js:92:26) --- duration_ms: 0.234

Addendum: I think skipping the test when localhost does not map to the expected address is a more sensible thing to do than to allow the test to pass no matter what error occurs in the test. Hence, this change.

Not this version. I am wondering why the old version with common.localIPv6Hosts failed.

@thefourtheye Ah! That. OK, sorry, I misunderstood. Here's the answer, I think:

I tried something like that in 00f9b60

Sample CI run: https://ci.nodejs.org/job/node-stress-single-test/812/nodes=centos5-32/console:

not ok 1 parallel/test-https-connect-address-family # # assert.js:89 # throw new assert.AssertionError({ # ^ # AssertionError: Could not find an IPv6 host for ::1 # at Object.exports.fail (/home/iojs/build/workspace/node-stress-single-test/nodes/centos5-32/test/common.js:429:10) # at dns.lookup (/home/iojs/build/workspace/node-stress-single-test/nodes/centos5-32/test/parallel/test-https-connect-address-family.js:31:12) # at GetAddrInfoReqWrap.asyncCallback [as callback] (dns.js:65:16) # at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:79:17) --- duration_ms: 0.342

So even cycling through all those hostnames, we still need to account for hosts where none of them are configured.

So I think we can try it again after this lands, since that's basically what this does except that it only tries localhost and not all those other names.

Okay, this should not fail as is. LGTM.

Trott · 2016-07-19T22:28:11Z

bump!

mhdawson · 2016-07-19T22:48:38Z

LGTM. I agree making it not fail erroneously is a good first step provided we are still planning on looking at how we make the test pass as well.

bnoordhuis · 2016-07-20T07:21:39Z

LGTM

thefourtheye · 2016-07-20T07:38:56Z

@Trott I would like to propose #7806 as an alternative. What do you think?

Trott · 2016-07-20T16:40:30Z

@thefourtheye I'd prefer to land this first and then have that be discussed separately as a potential further improvement.

thefourtheye · 2016-07-20T16:41:21Z

Sure. Let's not block this. LGTM.

The issue of hosts that do not resolve `localhost` to `::1` is now handled within the tests. Remove flaky status for test-https-connect-address-family and test-tls-connect-address-family. PR-URL: nodejs#7766 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Trott · 2016-07-20T17:40:50Z

Landed in 814b8c3

The issue of hosts that do not resolve `localhost` to `::1` is now handled within the tests. Remove flaky status for test-https-connect-address-family and test-tls-connect-address-family. PR-URL: #7766 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Trott added tls Issues and PRs related to the tls subsystem. https Issues or PRs related to the https subsystem. test Issues and PRs related to the tests. labels Jul 16, 2016

test: handle IPv6 localhost issues within tests

94d2396

The issue of hosts that do not resolve `localhost` to `::1` is now handled within the tests. Remove flaky status for test-https-connect-address-family and test-tls-connect-address-family.

Trott force-pushed the status branch from 80bcc5c to 94d2396 Compare July 16, 2016 21:08

mscdex added the dns Issues and PRs related to the dns subsystem. label Jul 16, 2016

thefourtheye reviewed Jul 19, 2016
View reviewed changes

Trott closed this Jul 20, 2016

cjihrig mentioned this pull request Aug 8, 2016

v6.4.0 Release Proposal #8020

Closed

cjihrig mentioned this pull request Aug 11, 2016

v6.4.0 Proposal #8070

Merged

gibfahn mentioned this pull request Aug 24, 2016

test: fix localhost error in test-dns-ipv6 #8254

Closed

3 tasks

Trott added the lts-watch-v4.x label Sep 9, 2016

bnoordhuis mentioned this pull request Sep 28, 2016

v4.6.0: test failure, servers not starting? #8831

Closed

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Sep 28, 2016

MylesBorins mentioned this pull request Oct 26, 2016

V4.6.2 proposal #9298

Closed

Trott deleted the status branch January 13, 2022 22:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: handle IPv6 localhost issues within tests #7766

test: handle IPv6 localhost issues within tests #7766

Trott commented Jul 16, 2016

Trott commented Jul 16, 2016

Trott commented Jul 19, 2016 •

edited

Loading

Trott commented Jul 19, 2016

thefourtheye Jul 19, 2016

bnoordhuis Jul 19, 2016

Trott Jul 19, 2016

thefourtheye Jul 20, 2016 •

edited

Loading

Trott Jul 20, 2016 •

edited

Loading

Trott Jul 20, 2016

thefourtheye Jul 20, 2016

Trott Jul 20, 2016

thefourtheye Jul 20, 2016

Trott commented Jul 19, 2016

mhdawson commented Jul 19, 2016

bnoordhuis commented Jul 20, 2016

thefourtheye commented Jul 20, 2016

Trott commented Jul 20, 2016

thefourtheye commented Jul 20, 2016

Trott commented Jul 20, 2016

test: handle IPv6 localhost issues within tests #7766

test: handle IPv6 localhost issues within tests #7766

Conversation

Trott commented Jul 16, 2016

Checklist

Affected core subsystem(s)

Description of change

Trott commented Jul 16, 2016

Trott commented Jul 19, 2016 • edited Loading

Trott commented Jul 19, 2016

thefourtheye Jul 19, 2016

Choose a reason for hiding this comment

bnoordhuis Jul 19, 2016

Choose a reason for hiding this comment

Trott Jul 19, 2016

Choose a reason for hiding this comment

thefourtheye Jul 20, 2016 • edited Loading

Choose a reason for hiding this comment

Trott Jul 20, 2016 • edited Loading

Choose a reason for hiding this comment

Trott Jul 20, 2016

Choose a reason for hiding this comment

thefourtheye Jul 20, 2016

Choose a reason for hiding this comment

Trott Jul 20, 2016

Choose a reason for hiding this comment

thefourtheye Jul 20, 2016

Choose a reason for hiding this comment

Trott commented Jul 19, 2016

mhdawson commented Jul 19, 2016

bnoordhuis commented Jul 20, 2016

thefourtheye commented Jul 20, 2016

Trott commented Jul 20, 2016

thefourtheye commented Jul 20, 2016

Trott commented Jul 20, 2016

Trott commented Jul 19, 2016 •

edited

Loading

thefourtheye Jul 20, 2016 •

edited

Loading

Trott Jul 20, 2016 •

edited

Loading