Skip to content
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

test: add a simple abort check in windows #12914

Closed
wants to merge 1 commit into from

Conversation

@sreepurnajasti
Copy link
Contributor

commented May 9, 2017

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

Working with @gireeshpunathil, I came up with this PR following @refack's suggestion
in #12856 . Though associated with that, this test is logically independent of that PR,
and does not need to co-exist with that one, and hence a separate one.

Make a simple assertion that Windows aborts with exit code 3.

@mscdex mscdex added the windows label May 9, 2017

@mscdex

This comment has been minimized.

Copy link
Contributor

commented May 9, 2017

@refack
refack approved these changes May 9, 2017
@gibfahn
gibfahn approved these changes May 9, 2017
@gibfahn

This comment has been minimized.

Copy link
Member

commented May 9, 2017

This looks great, if you could add a two line summary of what the test tests that would be ideal (see the writing tests guide).

@sreepurnajasti

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2017

@gibfahn Done. Added summary. Thanks

@cjihrig
cjihrig approved these changes May 9, 2017
Copy link
Contributor

left a comment

LGTM with a couple nits.

test/parallel/test-windows-abort-exitcode.js Outdated
if (process.argv[2] === 'child') {
process.abort();
} else {
const child = spawn(process.argv[0], [process.argv[1], 'child']);

This comment has been minimized.

Copy link
@cjihrig

cjihrig May 9, 2017

Contributor

Nit, but could you use process.execPath and __filename instead of the argv values? I think it's a little more readable.

This comment has been minimized.

Copy link
@sreepurnajasti

sreepurnajasti May 10, 2017

Author Contributor

Done.

test/parallel/test-windows-abort-exitcode.js Outdated
const assert = require('assert');

// This test makes sure that an aborted node process
// exits with code 3 in windows.

This comment has been minimized.

Copy link
@cjihrig

cjihrig May 9, 2017

Contributor

in windows -> on Windows

This comment has been minimized.

Copy link
@sreepurnajasti

sreepurnajasti May 10, 2017

Author Contributor

Done. Thanks.

test/parallel/test-windows-abort-exitcode.js Outdated
// This test makes sure that an aborted node process
// exits with code 3 in windows.
// Spawn a child, force an abort, and then check the
// exit code in the parent. Assert on code and signal

This comment has been minimized.

Copy link
@cjihrig

cjihrig May 9, 2017

Contributor

You can probably drop the last sentence.

This comment has been minimized.

Copy link
@sreepurnajasti

sreepurnajasti May 10, 2017

Author Contributor

Done.

@jasnell
jasnell approved these changes May 9, 2017
Copy link
Member

left a comment

LGTM with @cjihrig's comments addressed.

Sreepurna Jasti
test: add a simple abort check in windows
raise(SIGABRT) or CRT abort causes exit code 3 and
null signal in windows. Looks like this simple assertion
is not present in windows. Make this assertion.
@refack

This comment has been minimized.

Copy link
Member

commented May 12, 2017

Landed in 642bd4d

@refack refack closed this May 12, 2017

refack added a commit that referenced this pull request May 12, 2017
test: add a simple abort check in windows
raise(SIGABRT) or CRT abort causes exit code 3 and
null signal in windows. Looks like this simple assertion
is not present in windows. Make this assertion.

PR-URL: #12914
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack

This comment has been minimized.

Copy link
Member

commented May 12, 2017

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Sreepurna Jasti Olivier Martin
test: add a simple abort check in windows
raise(SIGABRT) or CRT abort causes exit code 3 and
null signal in windows. Looks like this simple assertion
is not present in windows. Make this assertion.

PR-URL: nodejs#12914
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jun 22, 2017

does this make sense for v6.x?

@refack

This comment has been minimized.

Copy link
Member

commented Jun 22, 2017

does this make sense for v6.x?

Yes. (just checked it passes as is).

@bnoordhuis bnoordhuis referenced this pull request Jun 29, 2017
3 of 3 tasks complete
MylesBorins added a commit that referenced this pull request Jul 17, 2017
test: add a simple abort check in windows
raise(SIGABRT) or CRT abort causes exit code 3 and
null signal in windows. Looks like this simple assertion
is not present in windows. Make this assertion.

PR-URL: #12914
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins referenced this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.