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.

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

Investigate flaky test-https-connect-address-family.js and test-tls-connect-address-family #7288

Closed
gibfahn opened this issue Jun 13, 2016 · 7 comments
Labels
test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.

Comments

@gibfahn
Copy link
Member

gibfahn commented Jun 13, 2016

  • Version:v6.2.1
  • Platform:Linux (Ubuntu 14.04, 15.04, Debian 8, others?)
  • Subsystem: test

See #6654 (comment) and nodejs/build#415

These tests were added in v6.2.1 and were marked as flaky due to a problem with resolving localhost on IPv6 on some Linux distros.

Basically /etc/hosts needs to contain the line ::1 localhost in order for localhost to be properly resolved. Most distros seem to have it (e.g. Ubuntu 16.04: ::1 localhost ip6-localhost ip6-loopback) but some, (e.g. Ubuntu 14.04/15.04 ::1 ip6-localhost ip6-loopback) don't. Given the number of people that will be testing their pull requests with make test, it seems important to make sure that this test passes on machines with other strings in their ::1 section.

As ./test/common.js contains this list of options, it should in theory work.

@bnoordhuis @jbergstroem I know you were/are looking into this, I just wanted to make sure we had an issue open for it.

@mscdex mscdex added tls Issues and PRs related to the tls subsystem. test Issues and PRs related to the tests. labels Jun 13, 2016
Trott added a commit to Trott/io.js that referenced this issue Jul 3, 2016
@bnoordhuis
Copy link
Member

As ./test/common.js contains this list of options, it should in theory work.

Theory != practice, unfortunately. The initial version of #6654 used that list but it only caused more CI fallout, not less.

@Trott
Copy link
Member

Trott commented Jul 5, 2016

FWIW, I think that once nodejs/build#415 happens, using the list of IPv6 local hostnames in common.js should become workable.

@mhdawson
Copy link
Member

mhdawson commented Jul 6, 2016

I have to agree this is quite annoying as this fails 100% on my ubuntu 14 box. The result is that other tests are not run, and you have to know to run things like make lint on your own. I wonder if the default runs should exclude all flaky tests ?

@gibfahn
Copy link
Member Author

gibfahn commented Jul 6, 2016

@Trott as I understand it, nodejs/build#415 means localhost will resolve to ::1 on the build machines, but it won't make a difference for anyone running make test on their own machine.

@Trott
Copy link
Member

Trott commented Jul 6, 2016

@gibm I think it will make the "try all the items in the IPv6 local hostnames in common.js" strategy work on CI. Which means we can implement that in the test, and that should fix it for people running make test. I could be wrong though.

@Trott
Copy link
Member

Trott commented Jul 7, 2016

Another option is to confirm that localhost maps to ::1 with something like this:

dns.lookup('localhost', {family:6, all: true}, (err, addresses) => {
  // code that skips the test if err is true or addresses does not contain { address: '::1', family: 6 }
  // otherwise, this kicks off the test
});

@Trott
Copy link
Member

Trott commented Jul 8, 2016

Simple workaround: #7605

Might be an argument for cycling through common.localIPv6Hosts. Counter-argument might be simplicity. Middle-ground: future enhancement if needed!

Trott added a commit to Trott/io.js that referenced this issue Jul 11, 2016
Skip test if localhost does not resolve to ::1.

Fixes: nodejs#7288
Trott added a commit to Trott/io.js that referenced this issue Jul 11, 2016
Skip test if localhost does not resolve to ::1.

Fixes: nodejs#7288
@Trott Trott closed this as completed in 2d77cba Jul 11, 2016
cjihrig pushed a commit that referenced this issue Aug 10, 2016
Skip tests if localhost does not resolve to ::1.

Fixes: #7288
PR-URL: #7605
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Conflicts:
	test/parallel/test-https-connect-address-family.js
	test/parallel/test-tls-connect-address-family.js
MylesBorins pushed a commit that referenced this issue Sep 28, 2016
Skip tests if localhost does not resolve to ::1.

Fixes: #7288
PR-URL: #7605
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
rvagg pushed a commit that referenced this issue Oct 18, 2016
Skip tests if localhost does not resolve to ::1.

Fixes: #7288
PR-URL: #7605
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this issue Oct 26, 2016
Skip tests if localhost does not resolve to ::1.

Fixes: #7288
PR-URL: #7605
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.
Projects
None yet
5 participants