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 countdown in test file #17646

Closed
wants to merge 1 commit into from
Closed

test: Use countdown in test file #17646

wants to merge 1 commit into from

Conversation

sreepurnajasti
Copy link
Contributor

Fixes: #17169

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • 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 13, 2017
lpinca
lpinca reviewed Dec 13, 2017
View reviewed changes
test/parallel/test-http-pipeline-regr-3332.js Outdated
@@ -26,15 +26,8 @@ const server = http.createServer(function(req, res) {

// Just let the test terminate instead of hanging
client.on('close', function() {
if (received !== COUNT)
if (countdown.remaining !== 0)
server.close();
Copy link
Member

@lpinca lpinca Dec 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@sreepurnajasti do you know why this is needed? Isn't the server.close() above sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lpinca Thanks for the review. This block is used to close the server instead of hanging. However, conditional check may not be required. If it is also, its fine. So, let me know whether to make any changes!!

Copy link
Member

Choose a reason for hiding this comment

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

The whole client.on('close') block should be removed. Since there's now no process.on('exit') check, we should let the test timeout if this isn't working correctly which would then signal that the test is broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lpinca @apapirovski Updated. Please review.

apapirovski
apapirovski approved these changes Dec 14, 2017
View reviewed changes
test/parallel/test-http-pipeline-regr-3332.js Outdated
@@ -26,15 +26,8 @@ const server = http.createServer(function(req, res) {

// Just let the test terminate instead of hanging
client.on('close', function() {
if (received !== COUNT)
if (countdown.remaining !== 0)
server.close();
Copy link
Member

Choose a reason for hiding this comment

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

The whole client.on('close') block should be removed. Since there's now no process.on('exit') check, we should let the test timeout if this isn't working correctly which would then signal that the test is broken.

@apapirovski
Copy link
Member

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

@apapirovski apapirovski added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 17, 2017
gireeshpunathil pushed a commit that referenced this pull request Dec 18, 2017
@sreepurnajasti @gireeshpunathil
test: Use countdown in test file
0a28f94 
Fixes: #17169

PR-URL: #17646
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@gireeshpunathil
Copy link
Member

Landed as 0a28f94

@sreepurnajasti sreepurnajasti deleted the regr-3332 branch December 19, 2017 03:49
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 29, 2017
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@sreepurnajasti @MylesBorins
test: Use countdown in test file
d83d973 
Fixes: #17169

PR-URL: #17646
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@sreepurnajasti @MylesBorins
test: Use countdown in test file
d9190c1 
Fixes: #17169

PR-URL: #17646
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
boingoing pushed a commit to nodejs/node-chakracore that referenced this pull request Jan 18, 2018
@sreepurnajasti @boingoing
test: Use countdown in test file
8fe0074 
Fixes: nodejs/node#17169

PR-URL: nodejs/node#17646
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2018
@sreepurnajasti @MylesBorins
test: Use countdown in test file
ef61f12 
Fixes: #17169

PR-URL: #17646
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2018
@sreepurnajasti @MylesBorins
test: Use countdown in test file
de1743e 
Fixes: #17169

PR-URL: #17646
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
@sreepurnajasti @gibfahn
test: Use countdown in test file
9d4add2 
Fixes: #17169

PR-URL: #17646
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
@sreepurnajasti @MylesBorins
test: Use countdown in test file
01b84b5 
Fixes: #17169

PR-URL: #17646
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
@sreepurnajasti @MylesBorins
test: Use countdown in test file
72be25f 
Fixes: #17169

PR-URL: #17646
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
@sreepurnajasti @MylesBorins
test: Use countdown in test file
bdb535c 
Fixes: #17169

PR-URL: #17646
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

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

[Tracking issue] Converting tests to use Countdown
9 participants
@sreepurnajasti @apapirovski @gireeshpunathil @jasnell @lpinca @BridgeAR @MylesBorins @addaleax @nodejs-github-bot