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

Jump to bottom

test: don't assume IPv6 in test-regress-GH-5727 #6319

Merged
merged 1 commit into from
Apr 21, 2016
Merged

test: don't assume IPv6 in test-regress-GH-5727 #6319

merged 1 commit into from
Apr 21, 2016
+4 −1

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 21, 2016

Checklist
  • tests and code linting passes
Affected core subsystem(s)

test

Description of change

test/parallel/test-regress-GH-5727 assumed that one of the servers would be listening on IPv6. This breaks when the machine running the test doesn't have IPv6. This commit builds the connection key that is compared dynamically.

Refs #5732

R= @Trott

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 21, 2016
@Trott
Copy link
Member

Trott commented Apr 21, 2016

LGTM but I'm not really sure what _connectionKey is all about, so /cc @indutny @trevnorris just in case there's a subtlety I'm missing...

@Trott
Copy link
Member

Trott commented Apr 21, 2016

/cc @dirceu

@jasnell
Copy link
Member

jasnell commented Apr 21, 2016

LGTM

@dirceu
Copy link
Contributor

dirceu commented Apr 21, 2016

Sorry, I didn't think about this possibility. Good catch. 👍

@Trott
Copy link
Member

Trott commented Apr 21, 2016

I wonder what happened that caused this to suddenly pop up as of just a few hours ago. (Change to the setup/config of the devices in CI? Change in the code base?)

Anyway, looks like it's failing 100% of the time, and the change seems small and easy to undo, so I'd be all for an expedited landing.

/cc @nodejs/build @nodejs/testing

@santigimeno
Copy link
Member

LGTM

@jbergstroem
Copy link
Member

No changes in CI afaik.

@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 21, 2016

@Trott I'm guessing it only started showing up last night because that's when the PR adding the test landed. Apparently, the test failed on the CI run in which it was added (see #5732), but still got landed.

CI for this PR: https://ci.nodejs.org/job/node-test-pull-request/2356/

@jasnell
Copy link
Member

jasnell commented Apr 21, 2016

My fault on that one, I'm guessing that I didn't look closely enough at the results and assumed that it was one of the known flaky tests.

@indutny
Copy link
Member

indutny commented Apr 21, 2016

LGTM

@cjihrig
test: don't assume IPv6 in test-regress-nodejsGH-5727
1e4d053 
test/parallel/test-regress-GH-5727 assumed that one of the
servers would be listening on IPv6. This breaks when the machine
running the test doesn't have IPv6. This commit builds the
connection key that is compared dynamically.

Refs: nodejs#5732
PR-URL: nodejs#6319
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@cjihrig cjihrig merged commit 1e4d053 into nodejs:master Apr 21, 2016
@cjihrig cjihrig deleted the test branch April 21, 2016 15:04
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
@cjihrig @joelostrowski
test: don't assume IPv6 in test-regress-nodejsGH-5727
9996455 
test/parallel/test-regress-GH-5727 assumed that one of the
servers would be listening on IPv6. This breaks when the machine
running the test doesn't have IPv6. This commit builds the
connection key that is compared dynamically.

Refs: nodejs#5732
PR-URL: nodejs#6319
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
@cjihrig @jasnell
test: don't assume IPv6 in test-regress-GH-5727
c83204e 
test/parallel/test-regress-GH-5727 assumed that one of the
servers would be listening on IPv6. This breaks when the machine
running the test doesn't have IPv6. This commit builds the
connection key that is compared dynamically.

Refs: #5732
PR-URL: #6319
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@MylesBorins
Copy link
Contributor

adding dont-land as this file does not exist in v4... please feel free to update

@refack refack mentioned this pull request Oct 18, 2018
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@cjihrig @Trott @jasnell @dirceu @santigimeno @jbergstroem @indutny @MylesBorins @nodejs-github-bot