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-http-client-get-url #13516

Closed
wants to merge 1 commit into from
Closed

test: fix flaky test-http-client-get-url #13516

wants to merge 1 commit into from

Conversation

sebastianplesciuc
Copy link

@sebastianplesciuc sebastianplesciuc commented Jun 7, 2017 •
edited by refack
Loading

Fixed test-http-client-get-url by waiting on HTTP GET requests to finish before closing the server.

Fixes: #13507

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 Jun 7, 2017
refack
refack approved these changes Jun 7, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

👍

test/parallel/test-http-client-get-url.js Outdated
http.get(url.parse(u));
http.get(new URL(u));
});
server.listen(0, common.mustCall(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could you add common.localhostIPv4 as arg (just for explicitness)

Copy link
Author

Choose a reason for hiding this comment

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

Done

@refack
Copy link
Contributor

refack commented Jun 7, 2017

🥇 I took at this file, and totally missed the server.close race.

refack
refack reviewed Jun 7, 2017
View reviewed changes
test/parallel/test-http-client-get-url.js Outdated
http.get(new URL(u));
});
server.listen(0, common.mustCall(() => {
const u = `http://${common.localhostIPv4}:${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 2: could you parameterize /foo?bar here and line 31

Copy link
Author

Choose a reason for hiding this comment

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

Done

@refack
Copy link
Contributor

refack commented Jun 7, 2017 •
edited
Loading

Added Fixes: https://github.com/nodejs/node/issues/13507 to OP

@refack refack self-assigned this Jun 7, 2017
@refack
Copy link
Contributor

refack commented Jun 7, 2017

/cc @nodejs/testing

test: fix flaky test-http-client-get-url
bc5ca4a 
Fixed test-http-client-get-url by waiting on HTTP GET
requests to finish before closing the server.

Fixes: #13507
refack
refack approved these changes Jun 7, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

💯

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Jun 7, 2017
@refack
Copy link
Contributor

refack commented Jun 11, 2017

CI: https://ci.nodejs.org/job/node-test-commit/10503/

refack pushed a commit to refack/node that referenced this pull request Jun 11, 2017
@refack
test: fix flaky test-http-client-get-url
61adb26 
Fixed test-http-client-get-url by waiting on HTTP GET
requests to finish before closing the server.

PR-URL: nodejs#13516
Fixes: nodejs#13507
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@refack
Copy link
Contributor

refack commented Jun 11, 2017

Landed in 61adb26

@refack refack closed this Jun 11, 2017
addaleax pushed a commit that referenced this pull request Jun 12, 2017
@addaleax
test: fix flaky test-http-client-get-url
4c5457f 
Fixed test-http-client-get-url by waiting on HTTP GET
requests to finish before closing the server.

PR-URL: #13516
Fixes: #13507
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@addaleax addaleax mentioned this pull request Jun 12, 2017
Merged
@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 do-not-land if it is being backported

@sebastianplesciuc
Copy link
Author

Same as for #12876.

@MylesBorins This can't be backported.

The test flaked because of multiple http.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 http.get call: https://github.com/nodejs/node/blob/v6.x-staging/test/parallel/test-http-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 :)

@refack
Copy link
Contributor

refack commented Jul 17, 2017

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

@sebastianplesciuc AFAICT you're right. So the labels are set right. Thanks for following up.

@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

@refack refack refack approved these changes

@santigimeno santigimeno santigimeno approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
http Issues or PRs related to the http 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.
7 participants
@sebastianplesciuc @refack @MylesBorins @santigimeno @cjihrig @mscdex @nodejs-github-bot