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: do not force the process to exit #29923

Closed
wants to merge 1 commit into from
Closed

test: do not force the process to exit #29923

wants to merge 1 commit into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Oct 10, 2019

Refs: #29873 (review)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 10, 2019
Trott
Trott approved these changes Oct 10, 2019
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM. I'd prefer some details in the commit message, but that's not blocking. Maybe something like this?:

Using process.exit() in these tests is unnecessary and may mask other problems.

@lpinca lpinca force-pushed the remove/process-exit branch from b3b9211 to b9139a4 Compare October 11, 2019 13:20
@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 11, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@lpinca
test: do not force the process to exit
5527329 
Using `process.exit()` in these tests is unnecessary and may mask other
problems.

Refs: nodejs#29873 (review)
@lpinca lpinca force-pushed the remove/process-exit branch from b9139a4 to 5527329 Compare October 11, 2019 18:46
@lpinca
Copy link
Member Author

lpinca commented Oct 11, 2019

Added a few more, PTAL.

@nodejs-github-bot
Copy link
Collaborator

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

Trott pushed a commit that referenced this pull request Oct 13, 2019
@lpinca @Trott
test: do not force the process to exit
19a8d22 
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>
@Trott
Copy link
Member

Trott commented Oct 13, 2019

Landed in 19a8d22

@Trott Trott closed this Oct 13, 2019
@lpinca lpinca deleted the remove/process-exit branch October 13, 2019 05:13
targos pushed a commit that referenced this pull request Oct 14, 2019
@lpinca @targos
test: do not force the process to exit
50f0660 
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 targos mentioned this pull request Oct 15, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZYSzys ZYSzys ZYSzys approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@richardlau richardlau richardlau approved these changes

@devnexen devnexen devnexen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@antsmartian antsmartian antsmartian approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@lpinca @nodejs-github-bot @Trott @Fishrock123 @antsmartian @cjihrig @devnexen @richardlau @BridgeAR @ZYSzys