test: fix test-process-kill-null.js for Windows

[starkwang]

The test-process-kill-null.js failed in Windows because cat command is invalid for Windows cmd.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

test

[richardlau]

The test-process-kill-null.js failed in Windows because cat command is invalid for Windows cmd.

It's included in Git bash. From BUILDING.md:

• Basic Unix tools required for some tests, Git for Windows includes Git Bash and tools which can be included in the global PATH.

[refack]

I want this.
The MSYS cat is a ported utility and it's not as robust on windows as it is on *nix.
If we could remove the git-for-windows dependancy, all the better.

[refack]

The recommended test is common.isWindows (assign const common on L23)

[refack]

add common.mustCall

[refack]

not needed

[refack]

add common.mustCall

[refack]

not needed

[refack]

Not needed.

[starkwang]

Pushed commit to address comments.

[refack]

CI: https://ci.nodejs.org/job/node-test-commit/10998/

[refack]

landed in 44483b6

[refack]

Sanity: https://ci.nodejs.org/job/node-test-commit-linuxone/7155/ ✔️
Windows: https://ci.nodejs.org/job/node-test-commit-windows-fanned/10276/

[refack]

@starkwang might will need a followup PR to fix this flakiness:

not ok 235 parallel/test-process-kill-null
  ---
  duration_ms: 0.255
  severity: fail
  stack: |-
    internal/process.js:190
          throw errnoException(err, 'kill');
          ^
    
    Error: kill ESRCH
        at exports._errnoException (util.js:1023:11)
        at process.kill (internal/process.js:190:13)
        at Socket.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vs2015\label\win2012r2\test\parallel\test-process-kill-null.js:38:11)
        at Socket.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vs2015\label\win2012r2\test\common\index.js:518:15)
        at emitOne (events.js:115:13)
        at Socket.emit (events.js:210:7)
        at addChunk (_stream_readable.js:252:12)
        at readableAddChunk (_stream_readable.js:239:11)
        at Socket.Readable.push (_stream_readable.js:197:10)
        at Pipe.onread (net.js:589:20)
  ...

https://ci.nodejs.org/job/node-test-binary-windows/9711/RUN_SUBSET=3,VS_VERSION=vs2015,label=win2012r2/

Running another windows CI: https://ci.nodejs.org/job/node-test-commit-windows-fanned/10278/

[refack]

@nodejs/platform-windows could the kill ESRCH ("No such process") actually be a bug?
micro context:

child.stdout.on('data', common.mustCall(function() {
  process.kill(child.pid, 'SIGKILL');
}

[richardlau]

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

The ChildProcess object may emit an 'error' event if the signal cannot be delivered.

[refack]

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

The ChildProcess object may emit an 'error' event if the signal cannot be delivered.

It's more a case of https://nodejs.org/dist/latest-v8.x/docs/api/process.html#process_process_kill_pid_signal

This method will throw an error if the target pid does not exist.
...
Windows platforms will throw an error if the pid is used to kill a process group.

But presumably in this case we attempt to kill only if we get something from stdout.
I'll try to repro locally. I smell something fishy 👃

[richardlau]

The child process can exit between Node.js reading stdout and calling the on 'data' callback.

[gibfahn]

@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 branch-diff (and collaborators!).

IDK if there's a standard way to do this, but don't land a PR twice seems like a reasonable rule.

[refack]

@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 branch-diff (and collaborators!).

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 🤷‍♂️

[gibfahn]

I consulted with @addaleax and @Trott and this seems like the "standard" way 🤷‍♂️

Happy to be wrong if others disagree.

[Trott]

I consulted with @addaleax and @Trott and this seems like the "standard" way 🤷‍♂️

I'm not 100% sure that re-landing from the same PR is the standard way. But if @addaleax says it's OK, that's good enough for me.

[addaleax]

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.

[refack]

@starkwang Would you like to open a new PR for this?

[starkwang]

Yes, I'd like to do it. I'm trying to reproduce the flakiness locally.

[refack]

@starkwang you could try spawn('type con', { shell: true }) that will start cmd.exe with the right arguments to run type con which is sort of the equivalent to cat with no args

[refack]

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/

[gibfahn]

So at the moment your Git author name and email address are set to:

starkwang <381152119@qq.com>

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 "381152119@qq.com"

To change the author for a single commit you can do:

git commit --amend --author="Weijia Wang <381152119@qq.com>"
git push --force-with-lease

It's entirely optional.

[starkwang]

@gibfahn Thanks for your suggestion! :D
I've changed the user information in Git.