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

test: fix flaky test-child-process-exec-timeout by santigimeno · Pull Request #12159 · nodejs/node · GitHub

/ node Public

test: fix flaky test-child-process-exec-timeout - #12159

Merged
santigimeno merged 1 commit into
nodejs:masterfrom
santigimeno:fix_flakytest-child-process-exec-timeout
Apr 4, 2017
ConversationCommits (1)ChecksFiles changed
Merged

test: fix flaky test-child-process-exec-timeout#12159
santigimeno merged 1 commit into
nodejs:masterfrom
santigimeno:fix_flakytest-child-process-exec-timeout

Conversation

santigimeno commented Apr 1, 2017

Copy link
Copy Markdown
Member

At least starting with Darwin Kernel Version 16.4.0, sending a SIGTERM
to a process that is still starting up kills it with SIGKILL instead of
SIGTERM.

Refs: libuv/libuv#1226

Relevant backtrace:

=== release test-child-process-exec-timeout ===                    
Path: parallel/test-child-process-exec-timeout
assert.js:81
  throw new assert.AssertionError({
  ^
AssertionError: 'SIGKILL' === 'SIGTERM'
    at cp.exec.common.mustCall (/Users/sgimeno/node/node/test/parallel/test-child-process-exec-timeout.js:21:10)
    at /Users/sgimeno/node/node/test/common.js:461:15
    at ChildProcess.exithandler (child_process.js:253:5)
    at emitTwo (events.js:125:13)
    at ChildProcess.emit (events.js:213:7)
    at maybeClose (internal/child_process.js:893:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:219:5)
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

santigimeno added os Issues and PRs related to the os subsystem. test Issues and PRs related to the tests. labels Apr 1, 2017
Trott approved these changes Apr 1, 2017
View reviewed changes

Trott 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

bnoordhuis approved these changes Apr 1, 2017
View reviewed changes

bnoordhuis 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 modulo style nit.

Comment thread test/parallel/test-child-process-exec-timeout.js Outdated

bnoordhuis Apr 1, 2017

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

Superfluous parens.

mscdex added child_process Issues and PRs related to the child_process subsystem. and removed os Issues and PRs related to the os subsystem. labels Apr 1, 2017

hiroppy commented Apr 2, 2017

Copy link
Copy Markdown
Member

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

gibfahn commented Apr 2, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Stress test on master: https://ci.nodejs.org/job/node-stress-single-test/1150/nodes=osx1010/
Stress test on this PR: https://ci.nodejs.org/job/node-stress-single-test/1151/nodes=osx1010/

santigimeno added the macos Issues and PRs related to the macOS platform / OSX. label Apr 2, 2017

gibfahn commented Apr 3, 2017

Copy link
Copy Markdown
Member

Looks like the stress test on master passed (running --repeat 1000 -J 10 times). Maybe it's triggered by load?

santigimeno commented Apr 3, 2017

Copy link
Copy Markdown
Member Author

@gibfahn I was able to reproduce it locally on a Darwin Kernel Version 16.5.0 after lots of repetitions (more than 1000 for sure). I don't know exactly what version is running the CI bot, but this issue was initially noticed on a Darwin Kernel Version 16.4.0 box.

gibfahn commented Apr 3, 2017

Copy link
Copy Markdown
Member

@santigimeno ahh yes, of course, we don't have a later box to test on. LGTM anyway.

santigimeno force-pushed the fix_flakytest-child-process-exec-timeout branch from 6944160 to ea3fe7c Compare April 4, 2017 08:26

santigimeno commented Apr 4, 2017

Copy link
Copy Markdown
Member Author

Updated with @bnoordhuis suggestion. I'll land this later

test: fix flaky test-child-process-exec-timeout
45c4ad5 
At least starting with Darwin Kernel Version 16.4.0, sending a SIGTERM
to a process that is still starting up kills it with SIGKILL instead of
SIGTERM.

PR-URL: nodejs#12159
Refs: libuv/libuv#1226
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
santigimeno force-pushed the fix_flakytest-child-process-exec-timeout branch from ea3fe7c to 45c4ad5 Compare April 4, 2017 13:02
santigimeno merged commit 45c4ad5 into nodejs:master Apr 4, 2017

santigimeno commented Apr 4, 2017

Copy link
Copy Markdown
Member Author

Landed in 45c4ad5. Thanks

santigimeno deleted the fix_flakytest-child-process-exec-timeout branch April 4, 2017 13:13
jasnell mentioned this pull request Apr 4, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
test: fix flaky test-child-process-exec-timeout
b15dc95 
At least starting with Darwin Kernel Version 16.4.0, sending a SIGTERM
to a process that is still starting up kills it with SIGKILL instead of
SIGTERM.

PR-URL: nodejs#12159
Refs: libuv/libuv#1226
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas mentioned this pull request Apr 10, 2017
Merged
2 tasks
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
test: fix flaky test-child-process-exec-timeout
1f732d3 
At least starting with Darwin Kernel Version 16.4.0, sending a SIGTERM
to a process that is still starting up kills it with SIGKILL instead of
SIGTERM.

PR-URL: #12159
Refs: libuv/libuv#1226
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
test: fix flaky test-child-process-exec-timeout
7af8738 
At least starting with Darwin Kernel Version 16.4.0, sending a SIGTERM
to a process that is still starting up kills it with SIGKILL instead of
SIGTERM.

PR-URL: #12159
Refs: libuv/libuv#1226
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins mentioned this pull request Apr 19, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
test: fix flaky test-child-process-exec-timeout
053e139 
At least starting with Darwin Kernel Version 16.4.0, sending a SIGTERM
to a process that is still starting up kills it with SIGKILL instead of
SIGTERM.

PR-URL: nodejs/node#12159
Refs: libuv/libuv#1226
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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
Trott Trott approved these changes
cjihrig cjihrig approved these changes
richardlau richardlau approved these changes
+3 more reviewers
targos targos approved these changes
hiroppy hiroppy approved these changes
gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

child_process Issues and PRs related to the child_process subsystem. macos Issues and PRs related to the macOS platform / OSX. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL