Skip to content

test: verify cluster worker exit #6993

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
Merged
merged 1 commit into from
May 27, 2016
Merged

test: verify cluster worker exit #6993

merged 1 commit into from
May 27, 2016

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented May 26, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

test-cluster-disconnect-handles already includes logic that tries to cleanup any child processes when the test fails. This commit adds additional checks to verify that the child exited normally, and fails the test if that is not the case.

Refs: #6988

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 26, 2016
@bnoordhuis
Copy link
Member

LGTM

@jasnell
Copy link
Member

jasnell commented May 26, 2016

LGTM if CI is green

@cjihrig
Copy link
Contributor Author

cjihrig commented May 26, 2016

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

@Trott
Copy link
Member

Trott commented May 26, 2016

Great idea, LGTM assuming no CI surprises.

@mscdex mscdex added the cluster Issues and PRs related to the cluster subsystem. label May 26, 2016
@santigimeno
Copy link
Member

CI is green except for unrelated flakes. LGTM

@cjihrig
test: verify cluster worker exit
a9492f5 
test-cluster-disconnect-handles already includes logic that
tries to cleanup any child processes when the test fails. This
commit adds additional checks to verify that the child exited
normally, and fails the test if that is not the case.

Refs: nodejs#6988
PR-URL: nodejs#6993
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@cjihrig cjihrig merged commit a9492f5 into nodejs:master May 27, 2016
@cjihrig cjihrig deleted the 6988 branch May 27, 2016 13:17
@MylesBorins MylesBorins mentioned this pull request Jul 11, 2016
Closed
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cluster Issues and PRs related to the cluster subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@cjihrig @bnoordhuis @jasnell @Trott @santigimeno @mscdex @MylesBorins @nodejs-github-bot