Skip to content

test: fix flaky test-https-client-get-url#12876

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
Closed
sebastianplesciuc wants to merge 1 commit into from
Closed

test: fix flaky test-https-client-get-url #12876

sebastianplesciuc wants to merge 1 commit into from

Conversation

@sebastianplesciuc
Copy link

@sebastianplesciucsebastianplesciuc commented May 6, 2017•
edited by refack
Loading

Fixes test-https-client-get-url by waiting on HTTPS GET
requests to finish before closing the server.

Fixes: #12873

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

test

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label May 6, 2017
@refack
Copy link
Contributor

refack commented May 6, 2017

@sebastianplesciuc definatly better.
And for the good work I brought you a present: https://ci.nodejs.org/job/node-test-commit/9706/

@refackrefack self-assigned this May 6, 2017
@refack
Copy link
Contributor

refack commented May 6, 2017•
edited
Loading

@sebastianplesciuc I changed the "Ref" -> "Fixes" in the first comment

You can change it in the commit message, so when this lands, the issue will be closed automatically.

@mscdexmscdex added the https Issues or PRs related to the https subsystem. label May 6, 2017
@mscdex
Copy link
Contributor

mscdex commented May 6, 2017

Stress test: https://ci.nodejs.org/job/node-stress-single-test/1202/

jasnell
jasnell approved these changes May 7, 2017
View reviewed changes
Copy link
Member

@jasnelljasnell left a comment

Choose a reason for hiding this comment

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

LGTM if the stress test results look good

@mscdex
Copy link
Contributor

mscdex commented May 7, 2017

I think it's still waiting on a stuck job from earlier today...?

@sebastianplesciuc
Copy link
Author

sebastianplesciuc commented May 7, 2017

@refack Changed the Refs to Fixes in commit message. Thank you!

@refack
Copy link
Contributor

refack commented May 7, 2017

New stress: https://ci.nodejs.org/job/node-stress-single-test/1203/

@refack
Copy link
Contributor

refack commented May 7, 2017

Stress looks solid.

@refack
Copy link
Contributor

refack commented May 7, 2017

CI (macOS only): https://ci.nodejs.org/job/node-test-commit-osx/9624/

@addaleax
Copy link
Member

addaleax commented May 7, 2017

@refack Like the CI tells you to, could you please avoid running stress-tests on all platforms unless that’s really necessary?

cjihrig
cjihrig approved these changes May 8, 2017
View reviewed changes
Copy link
Contributor

@cjihrigcjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with one request.

test/parallel/test-https-client-get-url.js Outdated
Copy link
Contributor

@cjihrigcjihrigMay 8, 2017

Choose a reason for hiding this comment

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

Can you add common.mustCall() here.

Copy link
Author

@sebastianplesciucsebastianplesciucMay 8, 2017

Choose a reason for hiding this comment

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

@cjihrig Added. Thanks!

thefourtheye
thefourtheye approved these changes May 8, 2017
View reviewed changes
test/parallel/test-https-client-get-url.js Outdated
Copy link
Contributor

@thefourtheyethefourtheyeMay 8, 2017

Choose a reason for hiding this comment

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

Nit: Instead of 127.0.0.1, you can use common.localhostIPv4

Copy link
Author

@sebastianplesciucsebastianplesciucMay 8, 2017

Choose a reason for hiding this comment

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

@thefourtheye Fixed! Thank you!

test: fix flaky test-https-client-get-url
d3f29e7
Fixed test-https-client-get-url by waiting on HTTPS GET requests to finish before closing the server. Fixes: #12873
refack pushed a commit to refack/node that referenced this pull request May 10, 2017
@refack
test: fix flaky test-https-client-get-url
317180f
Fixed test-https-client-get-url by waiting on HTTPS GET requests to finish before closing the server. PR-URL: nodejs#12876Fixes: nodejs#12873 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@refack
Copy link
Contributor

refack commented May 10, 2017

Landed in 317180f

@refackrefack closed this May 10, 2017
@refackrefack mentioned this pull request May 10, 2017
Closed
@refack
Copy link
Contributor

refack commented May 10, 2017•
edited
Loading

Post land CI:https://ci.nodejs.org/job/node-test-commit/9785/
(Since the CI is on master the one CI job covers three small lands I did almost together.)

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
test: fix flaky test-https-client-get-url
078e28d
Fixed test-https-client-get-url by waiting on HTTPS GET requests to finish before closing the server. PR-URL: nodejs#12876Fixes: nodejs#12873 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@jasnelljasnell mentioned this pull request May 28, 2017
Closed
@sebastianplesciucsebastianplesciuc mentioned this pull request Jun 7, 2017
Closed
@gibfahngibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@MylesBorins
Copy link
Contributor

MylesBorins commented Jul 17, 2017

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land

@sebastianplesciuc
Copy link
Author

sebastianplesciuc commented Jul 17, 2017

@MylesBorins This can't be backported.

The test flaked because of multiple https.get requests using both the Legacy and the latest version of url. The version of the test that is in the 6.x-staging branch only has one https.get call: https://github.com/nodejs/node/blob/v6.x-staging/test/parallel/test-https-client-get-url.js

Docs also seem to confirm this, URL in Node 6 LTS vs URL in Node 8.

Please correct me if I'm wrong and also please set the proper labels because I can't :)

@sebastianplesciucsebastianplesciuc mentioned this pull request Jul 17, 2017
Closed
3 tasks
@MylesBorins
Copy link
Contributor

MylesBorins commented Jul 17, 2017

Thanks @sebastianplesciuc

updated labels

@refackrefack removed their assignment Oct 20, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

@santigimenosantigimenosantigimeno approved these changes

@lpincalpincalpinca approved these changes

@cjihrigcjihrigcjihrig approved these changes

+2 more reviewers

@refackrefackrefack approved these changes

@thefourtheyethefourtheyethefourtheye approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

httpsIssues or PRs related to the https subsystem.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

12 participants

@sebastianplesciuc@refack@mscdex@addaleax@MylesBorins@jasnell@thefourtheye@santigimeno@lpinca@cjihrig@gibfahn@nodejs-github-bot