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 : refactored `test-http-response-splitting` to use countdown #17348

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@mithunsasidharan
Contributor

mithunsasidharan commented Nov 27, 2017

Refactored test case in test-http-response-splitting to use countdown as per #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

@mithunsasidharan mithunsasidharan changed the title from test : Refactored test-http-response-splitting to use countdown to test : refactored `test-http-response-splitting` to use countdown Nov 27, 2017

@mscdex mscdex added the http label Nov 27, 2017

@daxlab

daxlab approved these changes Nov 27, 2017

@mithunsasidharan

This comment has been minimized.

Show comment
Hide comment
@mithunsasidharan

mithunsasidharan Nov 27, 2017

Contributor

@maclover7 : Kindly run the CI for this PR too. Thanks !

Contributor

mithunsasidharan commented Nov 27, 2017

@maclover7 : Kindly run the CI for this PR too. Thanks !

@@ -46,8 +48,7 @@ const server = http.createServer((req, res) => {
default:
assert.fail('should not get to here.');
}
if (count === 3)
server.close();
countdown.dec();

This comment has been minimized.

@thefourtheye

thefourtheye Nov 29, 2017

Contributor

You can remove the count variable itself, if we can return the current value from Countdown.dec

@thefourtheye

thefourtheye Nov 29, 2017

Contributor

You can remove the count variable itself, if we can return the current value from Countdown.dec

This comment has been minimized.

@mithunsasidharan

mithunsasidharan Nov 29, 2017

Contributor

@thefourtheye : For what you've suggested.. the dec() returns the remaining count and not current count right... return this[kLimit]
So I guess we would need the count variable being used in switch else I'll have to do limit - countdown.remaining() or something to get the current count..Correct me if I'm wrong... we're counting down right...!

@mithunsasidharan

mithunsasidharan Nov 29, 2017

Contributor

@thefourtheye : For what you've suggested.. the dec() returns the remaining count and not current count right... return this[kLimit]
So I guess we would need the count variable being used in switch else I'll have to do limit - countdown.remaining() or something to get the current count..Correct me if I'm wrong... we're counting down right...!

This comment has been minimized.

@thefourtheye

thefourtheye Nov 29, 2017

Contributor

That's okay. That can be done in a separate PR though. I think that would be helpful.

@thefourtheye

thefourtheye Nov 29, 2017

Contributor

That's okay. That can be done in a separate PR though. I think that would be helpful.

This comment has been minimized.

@mithunsasidharan

mithunsasidharan Nov 29, 2017

Contributor

@thefourtheye : I'll have it done over another PR !

@mithunsasidharan

mithunsasidharan Nov 29, 2017

Contributor

@thefourtheye : I'll have it done over another PR !

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@@ -46,8 +48,7 @@ const server = http.createServer((req, res) => {
default:
assert.fail('should not get to here.');
}
if (count === 3)
server.close();
countdown.dec();

This comment has been minimized.

@thefourtheye

thefourtheye Nov 29, 2017

Contributor

That's okay. That can be done in a separate PR though. I think that would be helpful.

@thefourtheye

thefourtheye Nov 29, 2017

Contributor

That's okay. That can be done in a separate PR though. I think that would be helpful.

@maclover7

This comment has been minimized.

Show comment
Hide comment
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Dec 1, 2017

Member

Landed in 0f3fd79

Thanks for the PR! 🎉

Member

addaleax commented Dec 1, 2017

Landed in 0f3fd79

Thanks for the PR! 🎉

@addaleax addaleax closed this Dec 1, 2017

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

addaleax added a commit that referenced this pull request Dec 1, 2017

test: refactored test-http-response-splitting to use countdown
PR-URL: #17348
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

@mithunsasidharan mithunsasidharan deleted the mithunsasidharan:pr_5 branch Dec 2, 2017

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

test: refactored test-http-response-splitting to use countdown
PR-URL: #17348
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

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

test: refactored test-http-response-splitting to use countdown
PR-URL: #17348
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@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 19, 2017

test: refactored test-http-response-splitting to use countdown
PR-URL: #17348
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

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

test: refactored test-http-response-splitting to use countdown
PR-URL: #17348
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

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

test: refactored test-http-response-splitting to use countdown
PR-URL: #17348
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@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: refactored test-http-response-splitting to use countdown
PR-URL: #17348
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@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: refactored test-http-response-splitting to use countdown
PR-URL: nodejs#17348
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

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

test: refactored test-http-response-splitting to use countdown
PR-URL: nodejs#17348
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment