Exit process with correct error codes#2445
Conversation
|
Many developers are waiting for failed number of tests in exit code of mocha, not just for 0, 1 and 130. Is this a really necessary breaking change? |
|
Whether the full scope of the change is necessary is up for debate. Certainly the false pass has to be fixed.
Do you have any examples in the wild or any known numbers or is this just a case of ad-hoc statistics? The current state of Mocha is at odds with proper unix exit code semantics. You could also argue that these many people have been doing it wrong all the time and misinterpreting signals like "exit caused by SIGINT", "exit caused by invalid command", "file not found" etc as indicating the number of test failures, while the actual error condition was a completely different one |
|
Well in my personal opinion this is a necessary change. I noticed that our QA guys are looking at the process exit code as the cc @boneskull |
|
I don't see this as a necessary breaking change unless someone can document that their workflow is broken because of it. I agree it's "incorrect" and should probably be fixed at some point, but we need to be actually helping people when we break the API--not just in theory. However, the exit code should be capped at 255 or whatever so as to not exit with a false positive 0 if there happen to be 256 failing tests. @Munter Can you modify the PR? |
|
@boneskull Updated |
|
All green after restarting all the tests that failed because of broken phantomjs install |
boneskull
left a comment
There was a problem hiding this comment.
This looks good to me. Given we were tossing the
| process.on('SIGINT', function() { runner.abort(); }) | ||
| process.on('SIGINT', function() { | ||
| runner.abort(); | ||
| process.exit(130); |
There was a problem hiding this comment.
This is going to cause problems, because runner.abort() is attempting to exit cleanly.
Instead, you should be able to override runner.failures; see here.
|
Please ignore the "LGTM" comment; I don't see how to remove it. But line 433 will need fixing..after that this is OK (after conflicts are resolved). Thanks! |
047455b to
758f412
Compare
|
@boneskull Requested changes are implemented, branch is rebased and all test are green |
…roperty-currentretry * upstream/master: helpful error when necessary suite callback omitted; closes mochajs#1744 Fix an issue and add relevant tests when describe and xdescribe fail when not supplied with a callback (issue mochajs#1744). rename more fixtures; closes mochajs#2383 Report non-match to STDERR and exit if no files are matched (mochajs#2450) Exit process with correct error codes (mochajs#2445) fix test files not using .spec suffix fix test fixtures not using .fixture suffix always halt execution when async skip() called; closes mochajs#2465 (mochajs#2479) avoid test flake in "delay" test; closes mochajs#2469 (mochajs#2470) revert accidental change to bin paths disregard function names when testing under Node.js 6.5.0; closes mochajs#2467 (mochajs#2477) lints more files; add more files to lint check; closes mochajs#2457 adds *.orig to .gitignore Exclude the --inspect flag fix check-leaks to catch a leak whose name begins with 'd'
…-files-cache * upstream/master: attempt windows-friendly reproducible case for mochajs#2315 fix: fix uncaught TypeError if error occurs on next tick, closes mochajs#2315 (mochajs#2439) helpful error when necessary suite callback omitted; closes mochajs#1744 Fix an issue and add relevant tests when describe and xdescribe fail when not supplied with a callback (issue mochajs#1744). rename more fixtures; closes mochajs#2383 Report non-match to STDERR and exit if no files are matched (mochajs#2450) Exit process with correct error codes (mochajs#2445) fix test files not using .spec suffix fix test fixtures not using .fixture suffix always halt execution when async skip() called; closes mochajs#2465 (mochajs#2479) avoid test flake in "delay" test; closes mochajs#2469 (mochajs#2470) revert accidental change to bin paths disregard function names when testing under Node.js 6.5.0; closes mochajs#2467 (mochajs#2477) lints more files; add more files to lint check; closes mochajs#2457 adds *.orig to .gitignore Exclude the --inspect flag fix check-leaks to catch a leak whose name begins with 'd'
* Exit with code 130 in SIGINT. Refs mochajs#2438 * Exit with code 255 if more errors than 255 were returned. Fixes mochajs#2438
|
For anyone following this issue: mocha 11.3.0 now supports a --posix-exit-codes command line option that will use a non-zero exit code for fatal signals on unix systems (#4989). |
Exit with code 1 if any error occurredCloses #2438