Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

test: remove extra process.exit() #29873

Closed
wants to merge 1 commit into from
Closed

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Oct 7, 2019

I missed this during the review of #29816.

process.exit() has a tendency to hide test failures because it forces the process to exit. This test doesn't need the process.exit(), so this commit removes it.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

process.exit() has a tendency to hide test failures because
it forces the process to exit. This test doesn't need the
process.exit(), so this commit removes it.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 7, 2019
@richardlau richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 7, 2019
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@ZYSzys ZYSzys left a comment

Choose a reason for hiding this comment

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

So maybe we should remove those process.exit() too ?

res.on('end', () => {
server.close();
process.exit();
});

res.on('end', function() {
server.close();
process.exit();
});

danbev pushed a commit that referenced this pull request Oct 10, 2019
process.exit() has a tendency to hide test failures because
it forces the process to exit. This test doesn't need the
process.exit(), so this commit removes it.

PR-URL: #29873
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@danbev
Copy link
Contributor

danbev commented Oct 10, 2019

Landed in 7682874.

@danbev danbev closed this Oct 10, 2019
BridgeAR pushed a commit that referenced this pull request Oct 10, 2019
process.exit() has a tendency to hide test failures because
it forces the process to exit. This test doesn't need the
process.exit(), so this commit removes it.

PR-URL: #29873
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR BridgeAR mentioned this pull request Oct 10, 2019
@cjihrig cjihrig deleted the test branch October 11, 2019 16:07
lpinca added a commit to lpinca/node that referenced this pull request Oct 11, 2019
Using `process.exit()` in these tests is unnecessary and may mask other
problems.

Refs: nodejs#29873 (review)
Trott pushed a commit that referenced this pull request Oct 13, 2019
Using `process.exit()` in these tests is unnecessary and may mask other
problems.

Refs: #29873 (review)

PR-URL: #29923
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
targos pushed a commit that referenced this pull request Oct 14, 2019
Using `process.exit()` in these tests is unnecessary and may mask other
problems.

Refs: #29873 (review)

PR-URL: #29923
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants