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: fix flaky test-https-client-get-url #12876

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

sebastianplesciuc
Copy link

@sebastianplesciuc sebastianplesciuc 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-bot nodejs-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/

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

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

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

@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

@cjihrig cjihrig 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
@@ -47,12 +47,15 @@ const server = https.createServer(options, common.mustCall(function(req, res) {
res.writeHead(200, {'Content-Type': 'text/plain'});
res.write('hello\n');
res.end();
server.close();
}, 3));

server.listen(0, function() {
Copy link
Contributor

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

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
https.get(new URL(u));
});
server.listen(0, common.mustCall(() => {
const u = `https://127.0.0.1:${server.address().port}/foo?bar`;
Copy link
Contributor

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

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#12876
Fixes: nodejs#12873
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@refack
Copy link
Contributor

refack commented May 10, 2017

Landed in 317180f

@refack refack closed this May 10, 2017
@refack refack 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#12876
Fixes: nodejs#12873
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell jasnell mentioned this pull request May 28, 2017
Closed
@sebastianplesciuc sebastianplesciuc mentioned this pull request Jun 7, 2017
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@MylesBorins
Copy link
Contributor

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

@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 :)

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

Thanks @sebastianplesciuc

updated labels

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

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@santigimeno santigimeno santigimeno approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
https Issues or PRs related to the https subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.