-
-
Notifications
You must be signed in to change notification settings - Fork 3k
feat: add option to use posix exit code upon fatal signal #4989
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
Conversation
This PR hasn't had any recent activity, and I'm labeling it |
We should probably disable the stale action, pending picking up reviewing PRs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice and clean implementation, thanks! ✨
Requesting changes on a few things. But let me know, please, if I'm off base here 🙂.
Note that after this lands, we'll want to file a followup issue about making this the default behavior in some future version of Mocha. |
@JoshuaKGoldberg Thanks for the review! Hopefully that latest update addresses your questions, but let me know if there's anything else you'd like to see. Looking froward to not using our own fork of mocha! 😆 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Progress! Thanks for adding in the tests. I think we'll need to capture some more cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I think just the one point on 0
vs 1
to hammer out? 🙌
I appreciate the need to reduce command line options. Ideally, this posix-compliant behavior will be the default at some point. Adding it as an option for now will allow folks to begin using and experimenting with it, prior to making it the default behavior as a breaking change in a future release. I believe this is how node has introduced certain experimental features as well. |
I see after rebasing, some of the tests are failing so I'll have to spend some time debugging, sorry about the delay. |
Update signal-handling tests; remove unsupported cases
Looks excellent @73rhodes, subscribing to help review :) |
@mochajs/maintenance-crew and @voxpelli as a previous change requestor. This looks good to me, I see the concerns about more complex code but I agree with the Darren (PR author) that adding this as an option early will allow folks to experiment with it before it becomes a breaking change. Plus if there are bugs in this approach we can find them now rather than before a major version upgrade. I'm in favor of merging this now that tests pass and the code looks clean enough to me (adding utility functions for DRYness is my only thought, but that almost makes it less readable in this case) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t want to be a blocker for this, if @mark-wiemer thinks this is good to go then I’m okay with that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Co-authored-by: Josh Goldberg ✨ <[email protected]>
I was very careful to make sure this PR actually passed tests, but I clearly misread the report. Fortunately, the only test that failed is a known flaky issue on main branch that I haven't formally reported yet. I'm busy this weekend but planning to report it on Tuesday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new CLI option (--posix-exit-codes) that allows Mocha to exit using standard POSIX/UNIX shell exit codes rather than the number of failed tests. Key changes include:
- Addition of integration tests to verify correct exit codes for signals and test failures.
- Implementation updates in the CLI and helpers to support the new exit code behavior.
- Documentation updates describing the new option and its expected outcomes.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
test/integration/options/posixExitCodes.spec.js | New tests to validate POSIX exit codes under different conditions |
test/integration/helpers.js | Added SIGNAL_OFFSET constant and signal exit code emulation for tests |
test/integration/fixtures/*.js | New fixtures to simulate various signals and failing tests |
lib/cli/run.js | Added the new --posix-exit-codes command line option |
lib/cli/run-option-metadata.js | Updated option metadata to include posix-exit-codes |
lib/cli/run-helpers.js | Adjusted exit code clamping based on the new option and introduced minor refactoring |
docs/index.md | Updated documentation to explain --posix-exit-codes behavior |
@@ -27,7 +27,7 @@ const {UnmatchedFile} = require('./collect-files'); | |||
*/ | |||
const exitMochaLater = clampedCode => { | |||
process.on('exit', () => { | |||
process.exitCode = clampedCode; | |||
process.exitCode = Math.min(clampedCode, process.argv.includes('--posix-exit-codes') ? 1 : 255); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider extracting the check for '--posix-exit-codes' into a dedicated helper variable or function to improve readability and avoid repetition.
process.exitCode = Math.min(clampedCode, process.argv.includes('--posix-exit-codes') ? 1 : 255); | |
process.exitCode = Math.min(clampedCode, usePosixExitCodes() ? 1 : 255); |
Copilot uses AI. Check for mistakes.
@@ -39,6 +39,8 @@ const exitMochaLater = clampedCode => { | |||
* @private | |||
*/ | |||
const exitMocha = clampedCode => { | |||
const usePosixExitCodes = process.argv.includes('--posix-exit-codes'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Similar flag-check logic appears in multiple places; consider consolidating this behavior into a single helper function for consistency and easier future maintenance.
const usePosixExitCodes = process.argv.includes('--posix-exit-codes'); | |
const usePosixExitCodes = shouldUsePosixExitCodes(); |
Copilot uses AI. Check for mistakes.
Requirements
Description of the Change
Mocha uses the number of failed tests as an exit code, which is unconventional and has led to a number of issues when trying to integrate mocha into ci/cd pipelines.
To resolve these issues, this PR introduces a
--posix-exit-codes
boolean command-line option. If this option is specified, a fatal signal (eg.SIGABRT
, et al) will cause the process to consistently exit with a standard posix exit code (128 + the numeric ID of the signal) when mocha is run as a child process (which is the case when passing node options). This helps to solve issues for toolchains that expect standard posix exit codes, for example by preventing out-of-memory crashes from being silently ignored.The GNU libc manual page provides additional context on why using the number of errors as an exit status is problematic:
Providing the option to exit with standard posix shell exit codes avoids these problems and solves a number of downstream issues listed below.
Alternate Designs
The alternatives considered were not in the scope of the mocha project.
Why should this be in core?
This option is implemented in the core repository where signal handling occurs.
Benefits
This PR provides a solution for #3559 and various downstream issues reported by mocha consumers; it preserves non-zero exit codes when mocha is spawned as a child process via various CI/CD or reporting tools and prevents silently swallowing out-of-memory errors.
Possible Drawbacks
Introduces another option. Requires docs. Leaves non-standard exit code behavior to remain as the default.
Applicable issues
fixes #3559
#3893
#2445
#2438
istanbuljs/nyc#798
cypress-io/cypress#24695