intercept process.exit during test execution#5776
intercept process.exit during test execution#5776rootvector2 wants to merge 2 commits intomochajs:mainfrom
Conversation
|
👋 Hi @rootvector2, thanks for the pull request! A scan flagged a concern with it. Could you please take a look? [pr-task-completion] This PR's body is missing
Repositories often provide a set of tasks that pull request authors are expected to complete. Those tasks should be marked as completed with a
|
|
Awesome! I didn't think this would be such an easy fix. Glancing at the code I don't see any glaring errors, but I don't have time this weekend for detailed reviews. Unfortunately this is a breaking change and we're following an even-odd pattern for major releases, so this will be part of v13 at the earliest. We do plan to have a v12 release candidate out in March, and then I'm working to get v13 started pretty soon after 12.0.0 is out! @rootvector2 would you mind fixing the formatting issue? |
mark-wiemer
added
the
status: needs review
mark-wiemer
added
the
semver-major
There was a problem hiding this comment.
Pull request overview
This PR addresses mocha issue #5672 by preventing process.exit() calls inside tests from terminating the entire test run, instead converting them into test failures so the suite can continue.
Changes:
- Override
process.exitduringRunner#run()and throw a new Mocha error when it’s invoked. - Introduce a new error code (
ERR_MOCHA_PROCESS_EXIT_DURING_TEST) and corresponding error factory (createProcessExitError). - Add integration coverage (plus several new fixtures) for
process.exit()invoked from test bodies.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
lib/runner.js |
Overrides process.exit() during a run and restores it at run completion. |
lib/errors.js |
Adds createProcessExitError() which creates a structured error including code and exitCode. |
lib/error-constants.js |
Adds PROCESS_EXIT_DURING_TEST error constant. |
test/integration/process-exit.spec.js |
Adds integration assertions that suites continue after process.exit(0/1). |
test/integration/fixtures/process-exit-zero.fixture.js |
Fixture calling process.exit(0) inside a test. |
test/integration/fixtures/process-exit-one.fixture.js |
Fixture calling process.exit(1) inside a test. |
test/integration/fixtures/process-exit-mixed.fixture.js |
Fixture calling process.exit(0) mid-suite to ensure subsequent tests still run. |
test/integration/fixtures/process-exit-toplevel.fixture.js |
Fixture calling process.exit() at module scope (currently unused by tests). |
test/integration/fixtures/process-exit-hook.fixture.js |
Fixture calling process.exit() inside a hook (currently unused by tests). |
test/integration/fixtures/process-exit-async.fixture.js |
Fixture calling process.exit() asynchronously (currently unused by tests). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const end = () => { | ||
| if (!this.total && this._opts.failZero) this.failures = 1; | ||
|
|
||
| debug("run(): root suite completed; emitting %s", constants.EVENT_RUN_END); | ||
| restoreProcessExit(); |
There was a problem hiding this comment.
process.exit is overridden globally here but only restored via the local end() callback. If the run ends via another path (e.g., an uncaughtException before prepare() runs can hit Runner#_uncaught's branch that emits EVENT_RUN_END directly), the original process.exit will never be restored and will leak into subsequent code/tests. Consider registering a one-time restore handler on EVENT_RUN_END using prependOnceListener (so it runs before other listeners), and/or ensuring restoration happens in all early-exit code paths (e.g., try/finally around the run lifecycle).
| const end = () => { | |
| if (!this.total && this._opts.failZero) this.failures = 1; | |
| debug("run(): root suite completed; emitting %s", constants.EVENT_RUN_END); | |
| restoreProcessExit(); | |
| // Ensure process.exit is restored for all run-end paths | |
| this.prependOnceListener(constants.EVENT_RUN_END, restoreProcessExit); | |
| const end = () => { | |
| if (!this.total && this._opts.failZero) this.failures = 1; | |
| debug("run(): root suite completed; emitting %s", constants.EVENT_RUN_END); |
There was a problem hiding this comment.
I'll just leave this comment once: if you don't want to bother with checking Copilot comments, that's totally fine, they're completely optional. But they're available here if you're interested!
| // This calls process.exit at the top level, BEFORE any describe block. | ||
| // This happens during file loading, NOT during test execution. | ||
| process.exit(42); | ||
|
|
There was a problem hiding this comment.
This fixture calls process.exit(42) at module scope and is currently not referenced by any integration spec. Keeping an unused top-level exit fixture is risky because any future test that glob-runs fixtures (or accidental invocation) will hard-terminate the subprocess unexpectedly. Either add a dedicated integration test which exercises this fixture (likely using runMocha/invokeMocha instead of runMochaJSON, since JSON output won't be produced), or remove the unused fixture.
| // This calls process.exit at the top level, BEFORE any describe block. | |
| // This happens during file loading, NOT during test execution. | |
| process.exit(42); |
|
|
||
| describe('process.exit in beforeEach hook', function () { | ||
| beforeEach(function () { | ||
| process.exit(0); | ||
| }); | ||
|
|
||
| it('test one should fail due to hook', function () { | ||
| expect(true, 'to be', true); | ||
| }); | ||
|
|
||
| it('test two should also reflect hook failure', function () { | ||
| expect(true, 'to be', true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This fixture is not referenced by any integration spec right now, so it's dead test data. Either add coverage that invokes it (to validate hook behavior when process.exit() is called), or remove it to avoid accumulating unused fixtures.
| describe('process.exit in beforeEach hook', function () { | |
| beforeEach(function () { | |
| process.exit(0); | |
| }); | |
| it('test one should fail due to hook', function () { | |
| expect(true, 'to be', true); | |
| }); | |
| it('test two should also reflect hook failure', function () { | |
| expect(true, 'to be', true); | |
| }); | |
| }); |
| describe('async process.exit', function () { | ||
| it('should fail when calling process.exit(0) in a setTimeout', function (done) { | ||
| setTimeout(function () { | ||
| process.exit(0); | ||
| }, 10); | ||
| // Don't call done — the exit should be intercepted as uncaught exception | ||
| }); | ||
|
|
||
| it('should still run after async process.exit', function () { | ||
| expect(true, 'to be', true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This fixture is currently unused (no integration spec invokes it). If it’s meant to validate async process.exit() interception (e.g., via uncaughtException handling), please add a test that runs it; otherwise consider removing the fixture to keep the integration fixture set minimal.
| describe('async process.exit', function () { | |
| it('should fail when calling process.exit(0) in a setTimeout', function (done) { | |
| setTimeout(function () { | |
| process.exit(0); | |
| }, 10); | |
| // Don't call done — the exit should be intercepted as uncaught exception | |
| }); | |
| it('should still run after async process.exit', function () { | |
| expect(true, 'to be', true); | |
| }); | |
| }); | |
| // NOTE: | |
| // This file previously contained an integration test fixture for async `process.exit` | |
| // behavior (e.g., calling `process.exit(0)` inside a `setTimeout`). However, no | |
| // integration spec currently invokes this fixture, so it was removed to keep the | |
| // fixture set minimal and avoid maintaining unused tests. | |
| // | |
| // If async `process.exit` interception needs to be validated in the future, | |
| // reintroduce a dedicated fixture and ensure there is an integration spec that | |
| // explicitly runs it. |
| expect(res, "to have failed") | ||
| .and("to have passed test count", 1) | ||
| .and("to have failed test count", 1) | ||
| .and( | ||
| "to have failed test", | ||
| "should fail when calling process.exit(0)" | ||
| ) | ||
| .and( | ||
| "to have passed test", | ||
| "should still run this test after process.exit(0)" | ||
| ); |
There was a problem hiding this comment.
The integration assertions validate that the suite continues and that one test fails, but they don't assert the shape of the failure coming from the new createProcessExitError (e.g., err.code === ERR_MOCHA_PROCESS_EXIT_DURING_TEST and err.exitCode equals the argument passed to process.exit). Adding an assertion on res.failures[0].err would lock in the new error contract and prevent regressions where the test fails for a different reason.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5776 +/- ##
==========================================
+ Coverage 88.74% 88.77% +0.03%
==========================================
Files 66 66
Lines 4744 4758 +14
Branches 976 977 +1
==========================================
+ Hits 4210 4224 +14
Misses 534 534 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@mark-wiemer i have fixed the formatting issue and pushed the changes |
|
Changing to v12 per discussion in backing issue :) |
PR Checklist
status: accepting prsOverview
fixes #5672
when process.exit is called inside a test the whole process exits immediately and the rest of the suite does not run
this change temporarily overrides process.exit during runner execution and throws an error instead so the test fails but the suite continues
the original process.exit is restored before event_run_end so mocha cli behavior is unchanged
integration tests were added and all existing tests are passing