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

test: fix flaky test-https-connect-address-family #7605

Closed
wants to merge 3 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 8, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test https

Description of change

Skip test if localhost does not resolve to ::1.

Fixes: #7288

@Trott Trott added https Issues or PRs related to the https subsystem. test Issues and PRs related to the tests. labels Jul 8, 2016
@Trott
Copy link
Member Author

Trott commented Jul 8, 2016

@Trott
Copy link
Member Author

Trott commented Jul 8, 2016

One build failure on Raspberry Pi but otherwise CI is good.

@rvagg
Copy link
Member

rvagg commented Jul 8, 2016

nice, lgtm

@bnoordhuis
Copy link
Member

LGTM

@gibfahn
Copy link
Member

gibfahn commented Jul 8, 2016

@Trott would the same fix work for test-tls-connect-address-family? Assuming it would, would you want to do a separate PR for it?

@Trott
Copy link
Member Author

Trott commented Jul 8, 2016

@gibfahn Whoops, yes, I'll add that file too...

@Trott
Copy link
Member Author

Trott commented Jul 8, 2016

Added a second commit to addres test-tls-connect-address-family the same way. PTAL.

@Trott
Copy link
Member Author

Trott commented Jul 11, 2016

if (err)
throw err;

if (addresses.some((val) => { return val.address === '::1'; }))
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to do

addresses.some((val) => val.address === '::1')

Copy link
Member Author

@Trott Trott Jul 11, 2016

Choose a reason for hiding this comment

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

I thought I was going with the de facto standard in the project code as it exists, but it looks like you are correct. There are over 150 instances where the braces are already omitted and only around 20 cases where there are braces included even though they can be removed.

So, yeah, I'll remove these.

@thefourtheye
Copy link
Contributor

LGTM

Skip test if localhost does not resolve to ::1.

Fixes: nodejs#7288
Skip test if localhost does not resolve to ::1.

Fixes: nodejs#7288
@Trott
Copy link
Member Author

Trott commented Jul 11, 2016

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

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

Trott commented Jul 11, 2016

Landed in 2d77cba

@Trott Trott closed this Jul 11, 2016
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
cjihrig pushed a commit that referenced this pull request 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
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
MylesBorins pushed a commit that referenced this pull request 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 pull request 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 pull request 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>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
@Trott Trott deleted the ipv6localhost branch January 13, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
https Issues or PRs related to the https subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate flaky test-https-connect-address-family.js and test-tls-connect-address-family
6 participants