Skip to content

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

Closed
starkwang wants to merge 1 commit into
nodejs:masterfrom
starkwang:fix-test-process-kill-null
Closed

test: fix test-process-kill-null.js for Windows#14099
starkwang wants to merge 1 commit into
nodejs:masterfrom
starkwang:fix-test-process-kill-null

Conversation

@starkwang
Copy link
Copy Markdown
Contributor

@starkwang starkwang commented Jul 6, 2017

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 6, 2017
@richardlau
Copy link
Copy Markdown
Member

richardlau commented Jul 6, 2017

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.

@starkwang starkwang closed this Jul 6, 2017
@refack refack reopened this Jul 7, 2017
@refack
Copy link
Copy Markdown
Contributor

refack commented Jul 7, 2017

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
refack suggested changes Jul 7, 2017
View reviewed changes
Comment thread test/parallel/test-process-kill-null.js Outdated
Copy link
Copy Markdown
Contributor

@refack refack Jul 7, 2017

Choose a reason for hiding this comment

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

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

Comment thread test/parallel/test-process-kill-null.js Outdated
Copy link
Copy Markdown
Contributor

@refack refack Jul 7, 2017

Choose a reason for hiding this comment

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

add common.mustCall

Comment thread test/parallel/test-process-kill-null.js Outdated
Copy link
Copy Markdown
Contributor

@refack refack Jul 7, 2017

Choose a reason for hiding this comment

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

not needed

Comment thread test/parallel/test-process-kill-null.js Outdated
Copy link
Copy Markdown
Contributor

@refack refack Jul 7, 2017

Choose a reason for hiding this comment

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

add common.mustCall

Comment thread test/parallel/test-process-kill-null.js Outdated
Copy link
Copy Markdown
Contributor

@refack refack Jul 7, 2017

Choose a reason for hiding this comment

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

not needed

Comment thread test/parallel/test-process-kill-null.js Outdated
Copy link
Copy Markdown
Contributor

@refack refack Jul 7, 2017

Choose a reason for hiding this comment

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

Not needed.

@refack refack added process Issues and PRs related to the process subsystem. windows Issues and PRs related to the Windows platform. labels Jul 7, 2017
@starkwang starkwang force-pushed the fix-test-process-kill-null branch from 7b96888 to 310a19c Compare July 7, 2017 02:25
@starkwang
Copy link
Copy Markdown
Contributor Author

starkwang commented Jul 7, 2017

Pushed commit to address comments.

@refack
Copy link
Copy Markdown
Contributor

refack commented Jul 7, 2017

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

refack pushed a commit to refack/node that referenced this pull request Jul 8, 2017
@starkwang @refack
test: improve test-process-kill-null for Windows
44483b6 
PR-URL: nodejs#14099
Reviewed-By: Refael Ackermann <refack@gmail.com>
@refack
Copy link
Copy Markdown
Contributor

refack commented Jul 8, 2017

landed in 44483b6

@refack refack closed this Jul 8, 2017
@refack
Copy link
Copy Markdown
Contributor

refack commented Jul 8, 2017 •
edited
Loading

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

@refack
Copy link
Copy Markdown
Contributor

refack commented Jul 8, 2017 •
edited
Loading

@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 refack mentioned this pull request Jul 9, 2017
Merged
3 tasks
refack added a commit to refack/node that referenced this pull request Jul 9, 2017
@refack
Revert "test: improve test-process-kill-null for Windows"
d69ecc6 
This reverts commit 44483b6.

PR-URL: nodejs#14142
Fixes: nodejs#14141
Refs: nodejs#14099
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@refack refack reopened this Jul 9, 2017
@refack
Copy link
Copy Markdown
Contributor

refack commented Jul 9, 2017

@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
Copy link
Copy Markdown
Member

richardlau commented Jul 9, 2017

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.