test: fix test-process-kill-null.js for Windows #14099
Conversation
It's included in Git bash. From
|
|
I want this. |
| @@ -24,24 +24,24 @@ require('../common'); | |||
| const assert = require('assert'); | |||
| const spawn = require('child_process').spawn; | |||
|
|
|||
| const cat = spawn('cat'); | |||
| const child = process.platform === 'win32' ? spawn('cmd.exe') : spawn('cat'); | |||
There was a problem hiding this comment.
The recommended test is common.isWindows (assign const common on L23)
|
|
||
| cat.on('exit', function() { | ||
| child.on('exit', function() { |
| @@ -24,24 +24,24 @@ require('../common'); | |||
| const assert = require('assert'); | |||
| const spawn = require('child_process').spawn; | |||
|
|
|||
| const cat = spawn('cat'); | |||
| const child = process.platform === 'win32' ? spawn('cmd.exe') : spawn('cat'); | |||
| let called; | |||
| }, Error); | ||
| }); | ||
|
|
||
| cat.stdout.on('data', function() { | ||
| child.stdout.on('data', function() { |
| }, Error); | ||
| }); | ||
|
|
||
| cat.stdout.on('data', function() { | ||
| child.stdout.on('data', function() { | ||
| called = true; |
| }); | ||
|
|
||
| // EPIPE when null sig fails | ||
| cat.stdin.write('test'); | ||
| child.stdin.write('test'); | ||
|
|
||
| process.on('exit', function() { |
refack
added
process
starkwang
force-pushed
the
fix-test-process-kill-null
branch
from
7b96888 to
310a19c
Compare
|
Pushed commit to address comments. |
PR-URL: nodejs#14099 Reviewed-By: Refael Ackermann <[email protected]>
|
landed in 44483b6 |
|
@starkwang Running another windows CI: https://ci.nodejs.org/job/node-test-commit-windows-fanned/10278/ |
This reverts commit 44483b6. PR-URL: nodejs#14142 Fixes: nodejs#14141 Refs: nodejs#14099 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
|
@nodejs/platform-windows could the child.stdout.on('data', common.mustCall(function() {
process.kill(child.pid, 'SIGKILL');
} |
|
It probably means the child has exited before the kill was attempted. https://nodejs.org/dist/latest-v8.x/docs/api/child_process.html#child_process_child_kill_signal
|
It's more a case of https://nodejs.org/dist/latest-v8.x/docs/api/process.html#process_process_kill_pid_signal
But presumably in this case we attempt to kill only if we get something from |
|
The child process can exit between Node.js reading stdout and calling the on 'data' callback. |
|
@refack can you open a new PR? Now this has been reverted I don't think we should keep using it, it's likely to confuse things like IDK if there's a standard way to do this, but don't land a PR twice seems like a reasonable rule. |
I consulted with @addaleax and @Trott and this seems like the "standard" way 🤷♂️ |
|
I wouldn’t know that there’s a standard way to handle this either. Opening a new PR is certainly easier for everyone and will make sure the new commit won’t get overlooked, but reverts are probably rare enough that handling them manually is feasible. |
|
@starkwang Would you like to open a new PR for this? |
|
Yes, I'd like to do it. I'm trying to reproduce the flakiness locally. |
|
@starkwang you could try |
|
P.S. @starkwang if you want your full name in the logs you could follow https://help.github.com/articles/setting-your-username-in-git/ |
|
So at the moment your Git author name and email address are set to: People usually choose to use their full names for commits. To set your name globally you can do: git config --global user.name "Weijia Wang"
git config --global user.email "[email protected]"To change the author for a single commit you can do: git commit --amend --author="Weijia Wang <[email protected]>"
git push --force-with-leaseIt's entirely optional. |
|
@gibfahn Thanks for your suggestion! :D |
The test-process-kill-null.js failed in Windows because
catcommand is invalid for Windows cmd.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test