FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: add coverage for spawnSync() killSignal by cjihrig · Pull Request #8960 · nodejs/node · GitHub

/ node Public

test: add coverage for spawnSync() killSignal - #8960

Merged
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:killsignal
Oct 10, 2016
ConversationCommits (1)ChecksFiles changed
Merged

test: add coverage for spawnSync() killSignal#8960
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:killsignal

Conversation

cjihrig commented Oct 6, 2016

Copy link
Copy Markdown
Contributor
Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

This commit adds a test for the killSignal option to spawnSync(), and the other sync child process functions by extension.

This was previously untested according to https://node-core-coverage.addaleax.net/

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2016
jasnell approved these changes Oct 6, 2016
View reviewed changes

jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

mscdex added the child_process Issues and PRs related to the child_process subsystem. label Oct 6, 2016
bnoordhuis approved these changes Oct 7, 2016
View reviewed changes
Comment thread test/parallel/test-child-process-spawnsync-kill-signal.js Outdated

bnoordhuis Oct 7, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You could DRY this as const { SIGUSR2 } = process.binding('constants').os.signals. Minor thing though.

cjihrig commented Oct 10, 2016

Copy link
Copy Markdown
Contributor Author

CI with the suggestion incorporated: https://ci.nodejs.org/job/node-test-pull-request/4445/

cjihrig commented Oct 10, 2016

Copy link
Copy Markdown
Contributor Author

Related failures on Windows.

cjihrig force-pushed the killsignal branch 2 times, most recently from dc1c87f to 532e7f7 Compare October 10, 2016 17:06

cjihrig commented Oct 10, 2016

Copy link
Copy Markdown
Contributor Author

It looks like child.status is 1 on Windows, and 0 elsewhere. Additionally, certain signals are not supported on Windows. Added detection of the exit code, and switched from SIGUSR2 to SIGKILL.

CI: https://ci.nodejs.org/job/node-test-pull-request/4447/. The only failures are now unrelated.

test: add coverage for spawnSync() killSignal
01db04b 
This commit adds a test for the killSignal option to spawnSync(),
and the other sync child process functions by extension.

PR-URL: nodejs#8960
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
cjihrig merged commit 01db04b into nodejs:master Oct 10, 2016
cjihrig deleted the killsignal branch October 10, 2016 18:34
jasnell pushed a commit that referenced this pull request Oct 10, 2016
test: add coverage for spawnSync() killSignal
16c4df9 
This commit adds a test for the killSignal option to spawnSync(),
and the other sync child process functions by extension.

PR-URL: #8960
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
test: add coverage for spawnSync() killSignal
56951b4 
This commit adds a test for the killSignal option to spawnSync(),
and the other sync child process functions by extension.

PR-URL: #8960
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
test: add coverage for spawnSync() killSignal
51b251d 
This commit adds a test for the killSignal option to spawnSync(),
and the other sync child process functions by extension.

PR-URL: #8960
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins mentioned this pull request Nov 22, 2016
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes

Assignees

No one assigned

Labels

child_process Issues and PRs related to the child_process 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.

6 participants

Back | FazBrowse Home | New Git URL