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.

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

test: use common.mustCall in http test #17439

Closed
wants to merge 1 commit into
base: master
from

Conversation

@mithunsasidharan
Contributor

mithunsasidharan commented Dec 3, 2017

Refactored the test case in test-http-malformed-request to use common.mustCall, as per issue #17169

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

@daxlab

daxlab approved these changes Dec 3, 2017

@apapirovski

This test should instead be refactored to use common.mustCall for the response function passed to http.createServer. The nrequests_completed and nrequests_expected variables should still be removed. The server.close() can just be unconditional.

@mithunsasidharan

This comment has been minimized.

Show comment
Hide comment
@mithunsasidharan

mithunsasidharan Dec 3, 2017

Contributor

@apapirovski : Thanks for the feedback. I've updated the PR to include the recommended changes. Kindly review the PR now.

Contributor

mithunsasidharan commented Dec 3, 2017

@apapirovski : Thanks for the feedback. I've updated the PR to include the recommended changes. Kindly review the PR now.

@apapirovski

LGTM but the commit message & PR title should be updated

@apapirovski

This comment has been minimized.

Show comment
Hide comment

@mithunsasidharan mithunsasidharan changed the title from refactor : `test-http-malformed-request` to use countdown to refactor : `test-http-malformed-request` to use common#mustCall Dec 3, 2017

@mithunsasidharan mithunsasidharan changed the title from refactor : `test-http-malformed-request` to use common#mustCall to refactor : `test-http-malformed-request` to use common.mustCall Dec 3, 2017

@mithunsasidharan

This comment has been minimized.

Show comment
Hide comment
@mithunsasidharan

mithunsasidharan Dec 3, 2017

Contributor

@apapirovski : Thats's done too.. Thanks.

Contributor

mithunsasidharan commented Dec 3, 2017

@apapirovski : Thats's done too.. Thanks.

@mscdex mscdex added the http label Dec 3, 2017

@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Dec 3, 2017

Member

The commit message is still too long. Something like test: use common.mustCall in http test would be fine. We don't need to specify the exact test file changed within the message. Thanks!

Member

apapirovski commented Dec 3, 2017

The commit message is still too long. Something like test: use common.mustCall in http test would be fine. We don't need to specify the exact test file changed within the message. Thanks!

@mithunsasidharan mithunsasidharan changed the title from refactor : `test-http-malformed-request` to use common.mustCall to test: use common.mustCall in http test Dec 3, 2017

@mithunsasidharan

This comment has been minimized.

Show comment
Hide comment
@mithunsasidharan

mithunsasidharan Dec 3, 2017

Contributor

@apapirovski : Thanks for the feedback. I've updated the commit message. Kindly review the PR now.

Contributor

mithunsasidharan commented Dec 3, 2017

@apapirovski : Thanks for the feedback. I've updated the commit message. Kindly review the PR now.

@apapirovski

Still LGTM

@lpinca

lpinca approved these changes Dec 4, 2017

@jasnell

jasnell approved these changes Dec 5, 2017

@maclover7

This comment has been minimized.

Show comment
Hide comment
@maclover7

maclover7 Dec 5, 2017

Member

Landing...

Member

maclover7 commented Dec 5, 2017

Landing...

@maclover7 maclover7 self-assigned this Dec 5, 2017

@maclover7

This comment has been minimized.

Show comment
Hide comment
@maclover7

maclover7 Dec 5, 2017

Member

Thank you for your contribution, landed in f6b2839.
❤️ 💚 💙 💛 💜

Member

maclover7 commented Dec 5, 2017

Thank you for your contribution, landed in f6b2839.
❤️ 💚 💙 💛 💜

@maclover7 maclover7 closed this Dec 5, 2017

maclover7 added a commit to maclover7/node that referenced this pull request Dec 5, 2017

test: use common.mustCall in test-http-malformed-request
PR-URL: nodejs#17439
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@mithunsasidharan mithunsasidharan deleted the mithunsasidharan:pr_9 branch Dec 5, 2017

@addaleax addaleax removed the author ready label Dec 7, 2017

MylesBorins added a commit that referenced this pull request Dec 12, 2017

test: use common.mustCall in test-http-malformed-request
PR-URL: #17439
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Dec 12, 2017

test: use common.mustCall in test-http-malformed-request
PR-URL: #17439
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Dec 12, 2017

Merged

v9.3.0 proposal #17631

gibfahn added a commit that referenced this pull request Dec 20, 2017

test: use common.mustCall in test-http-malformed-request
PR-URL: #17439
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

gibfahn added a commit that referenced this pull request Dec 20, 2017

test: use common.mustCall in test-http-malformed-request
PR-URL: #17439
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@gibfahn gibfahn referenced this pull request Dec 20, 2017

Closed

v8.9.4 proposal #17772

gibfahn added a commit that referenced this pull request Dec 20, 2017

test: use common.mustCall in test-http-malformed-request
PR-URL: #17439
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@gibfahn gibfahn referenced this pull request Dec 20, 2017

Merged

v8.9.4 proposal #17774

@MylesBorins MylesBorins referenced this pull request Dec 20, 2017

Merged

v6.12.3 proposal #17776

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

test: use common.mustCall in test-http-malformed-request
PR-URL: nodejs#17439
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

test: use common.mustCall in test-http-malformed-request
PR-URL: nodejs#17439
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment