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.
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
Don't kill fkill
when killing node
#30
Conversation
Tests are failing. |
Tests fixed. |
fkill
when killing node
Co-Authored-By: Sindre Sorhus <sindresorhus@gmail.com>
Co-Authored-By: Sindre Sorhus <sindresorhus@gmail.com>
@yaodingyd This is failing in master on Node.js 12: https://travis-ci.org/sindresorhus/fkill/builds/546047892?utm_medium=notification&utm_source=email |
@sindresorhus tests passed on my local node 12. will keep investigating. |
@sindresorhus I did multiple build on Jenkins and they all passed. Not sure how to deal with this... https://travis-ci.org/yaodingyd/fkill/builds/546255423 |
@@ -73,6 +73,13 @@ test.serial('don\'t kill self', async t => { | |||
Object.defineProperty(process, 'pid', {value: originalFkillPid}); | |||
}); | |||
|
|||
test.serial('don\'t kill `fkill` when killing `node`', async t => { | |||
const originalFkillPid = process.pid; | |||
await fkill('node'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if I may have missed the point of this PR but I don't understand how the this test is meant to work on Windows.
On Windows, if we try to kill node
it won't find any process because the process name for the Node.js runtime it's node.exe
, hence the test would pass because it won't kill fkill
yes, but it also won't kill any other node
processes as well, which kind of make it pointless to try killing by process name in the first place.
As an example, try running on a console node -e "while(true){}"
and try running the test, which should kill all node
processes, but it won't on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix #19