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: Use mustCall in http test #17487

Closed
wants to merge 1 commit into from
Closed

test: Use mustCall in http test #17487

wants to merge 1 commit into from

Conversation

sreepurnajasti
Copy link
Contributor

@sreepurnajasti sreepurnajasti commented Dec 6, 2017 •
edited by apapirovski
Loading

Refs: #17169

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 6, 2017
apapirovski
apapirovski reviewed Dec 6, 2017
View reviewed changes
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Could you update to use common.mustCall instead since it only runs 1 iteration? Countdown is better for tests where n > 1.

(Also, the commit message will need to be updated and should be 50 chars or less.)

Thanks!

test/parallel/test-http-pipeline-regr-2639.js Outdated

const COUNT = 10;

let received = 0;
const countdown = new Countdown(1, () => server.close());

const server = http.createServer(function(req, res) {
Copy link
Member

Choose a reason for hiding this comment

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

This test would be better if it used common.mustCall here instead of Countdown since it's just 1 iteration. The server.close() doesn't need to be conditional.

maclover7
maclover7 approved these changes Dec 6, 2017
View reviewed changes
Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

LGTM once @apapirovski's comment is addressed

@sreepurnajasti sreepurnajasti force-pushed the countdown-test-http-pipeline-regr-2639 branch from 52bd113 to e55bba6 Compare December 7, 2017 07:45
@sreepurnajasti
Copy link
Contributor Author

@apapirovski Thanks for the feedback. Modified as per your suggestion. Please, have a look.

@sreepurnajasti sreepurnajasti changed the title test: Update test-http-pipeline-regr-2639 to use countdown timer test: Use mustCallAtleast in http test Dec 7, 2017
apapirovski
apapirovski approved these changes Dec 7, 2017
View reviewed changes
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes. Some feedback below. Also, the process.on('exit'); block can also be removed since there's now a common.mustCall which verifies that the test runs long enough.

test/parallel/test-http-pipeline-regr-2639.js Outdated
@@ -8,7 +8,7 @@ const COUNT = 10;

let received = 0;

const server = http.createServer(function(req, res) {
const server = http.createServer(common.mustCallAtLeast((req, res) => {
// Close the server, we have only one TCP connection anyway
if (received++ === 0)
Copy link
Member

@apapirovski apapirovski Dec 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This line can be removed, we can safely call server.close() multiple times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!!

test/parallel/test-http-pipeline-regr-2639.js Outdated
@@ -8,7 +8,7 @@ const COUNT = 10;

let received = 0;
Copy link
Member

Choose a reason for hiding this comment

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

This can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Done!!

test/parallel/test-http-pipeline-regr-2639.js Outdated
@@ -8,7 +8,7 @@ const COUNT = 10;

let received = 0;

const server = http.createServer(function(req, res) {
const server = http.createServer(common.mustCallAtLeast((req, res) => {
Copy link
Member

@apapirovski apapirovski Dec 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This should be common.mustCall which specifies COUNT as the 2nd argument, that way it will make sure that this function is called the right number of times. Something like:

common.mustCall((req, res) => {
  // test code here
}, COUNT);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for explanation. Its Done.

@sreepurnajasti sreepurnajasti force-pushed the countdown-test-http-pipeline-regr-2639 branch from e55bba6 to de44a99 Compare December 7, 2017 09:58
@apapirovski
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/11942/

@sreepurnajasti sreepurnajasti changed the title test: Use mustCallAtleast in http test test: Use mustCall in http test Dec 8, 2017
@apapirovski
Copy link
Member

Landed in f81bb7b

Thanks for the contribution @sreepurnajasti! 👍

@apapirovski apapirovski closed this Dec 8, 2017
apapirovski pushed a commit that referenced this pull request Dec 8, 2017
@sreepurnajasti @apapirovski
test: Use common.mustCall in http test
f81bb7b 
PR-URL: #17487
Refs: #17169
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@sreepurnajasti sreepurnajasti deleted the countdown-test-http-pipeline-regr-2639 branch December 9, 2017 08:05
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@sreepurnajasti @MylesBorins
test: Use common.mustCall in http test
b83d69b 
PR-URL: #17487
Refs: #17169
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@sreepurnajasti @MylesBorins
test: Use common.mustCall in http test
af8e27d 
PR-URL: #17487
Refs: #17169
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@sreepurnajasti @gibfahn
test: Use common.mustCall in http test
d4d3f50 
PR-URL: #17487
Refs: #17169
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@sreepurnajasti @gibfahn
test: Use common.mustCall in http test
f9e0cb2 
PR-URL: #17487
Refs: #17169
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@sreepurnajasti @gibfahn
test: Use common.mustCall in http test
56c2f0f 
PR-URL: #17487
Refs: #17169
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Merged
@MylesBorins MylesBorins mentioned this pull request Dec 20, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apapirovski apapirovski apapirovski approved these changes

@maclover7 maclover7 maclover7 approved these changes

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.
5 participants
@sreepurnajasti @apapirovski @maclover7 @gibfahn @nodejs-github-bot