Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: handle IPv6 localhost issues within tests #7766

Closed
wants to merge 1 commit into from
Closed

test: handle IPv6 localhost issues within tests #7766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions test/parallel/parallel.status
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,6 @@ test-tick-processor : PASS,FLAKY
[$system==linux]
test-tick-processor : PASS,FLAKY

# Flaky until https://github.com/nodejs/build/issues/415 is resolved.
# On some of the buildbots, AAAA queries for localhost don't resolve
# to an address and neither do any of the alternatives from the
# localIPv6Hosts list from test/common.js.
test-https-connect-address-family : PASS,FLAKY
test-tls-connect-address-family : PASS,FLAKY

[$system==macos]

[$system==solaris] # Also applies to SmartOS
Expand Down
7 changes: 6 additions & 1 deletion test/parallel/test-https-connect-address-family.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,13 @@ function runTest() {
}

dns.lookup('localhost', {family: 6, all: true}, (err, addresses) => {
if (err)
if (err) {
if (err.code === 'ENOTFOUND') {
common.skip('localhost does not resolve to ::1');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@thefourtheye thefourtheye Jul 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@Trott Trott Jul 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this should not fail as is. LGTM.

return;
}
throw err;
}

if (addresses.some((val) => val.address === '::1'))
runTest();
Expand Down
7 changes: 6 additions & 1 deletion test/parallel/test-tls-connect-address-family.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,13 @@ function runTest() {
}

dns.lookup('localhost', {family: 6, all: true}, (err, addresses) => {
if (err)
if (err) {
if (err.code === 'ENOTFOUND') {
common.skip('localhost does not resolve to ::1');
return;
}
throw err;
}

if (addresses.some((val) => val.address === '::1'))
runTest();
Expand Down