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

feat: add option to use posix exit code upon fatal signal #4989

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
8999b7b
Option to use posix exit code upon fatal signal
73rhodes May 23, 2023
3b99517
Update test/integration/options/posixExitCodes.spec.js
73rhodes Mar 15, 2024
2150c30
Address PR review comments mocha org
73rhodes Mar 18, 2024
0fb6ca3
Merge pull request #2 from zendesk/dderidder/X-mochajs-pr-review
73rhodes Mar 18, 2024
62528c5
Address PR review comments mocha org
73rhodes Mar 18, 2024
8a4f2bd
Merge pull request #3 from zendesk/dderidder/X-mochajs-pr-review
73rhodes Mar 18, 2024
817d890
Merge branch 'master' into master
73rhodes Mar 18, 2024
ad30057
Update docs/index.md
73rhodes Mar 25, 2024
592f071
Fix exit code when no signal caught
73rhodes Apr 11, 2024
34f2822
Coverage for posix exit code with normally failing tests
73rhodes Apr 11, 2024
3438da6
Merge pull request #4 from zendesk/dderidder/X-mochajs-pr-review-part2
73rhodes Apr 11, 2024
b6e6d30
Update docs/index.md
73rhodes Apr 15, 2024
d2991a1
Address PR comments; use os.constants
73rhodes Apr 15, 2024
7cf79d8
Merge pull request #5 from zendesk/dderidder/X-mochajs-pr-review-part3
73rhodes Apr 15, 2024
609f94f
Remove test that asserts the problematic behavior
73rhodes Apr 23, 2024
b71eaf3
Merge pull request #6 from zendesk/dderidder/X-mochajs-pr-review-part4
73rhodes Apr 23, 2024
26bebac
Merge branch 'mochajs:master' into master
73rhodes Apr 24, 2024
0aa939f
Omit win32 from signals test suite
73rhodes Apr 26, 2024
59e471d
Merge pull request #7 from zendesk/dderidder/X-win32-fixup
73rhodes Apr 26, 2024
6d237fc
Update docs/index.md
73rhodes May 1, 2024
ca78dbc
Update package.json
73rhodes Jun 7, 2024
1809576
Merge pull request #8 from zendesk/73rhodes-patch-1
73rhodes Jun 7, 2024
df38431
WIP - support posix-exit-codes option for both child-process and in-p…
73rhodes Jul 9, 2024
9ee844a
Merge pull request #11 from zendesk/dderidder/fix-exit-codes-pr-update
73rhodes Aug 2, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions bin/mocha.js
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
* @private
*/

const os = require('os');
const {loadOptions} = require('../lib/cli/options');
const {
unparseNodeFlags,
Expand All @@ -22,6 +23,9 @@ const {aliases} = require('../lib/cli/run-option-metadata');

const mochaArgs = {};
const nodeArgs = {};
const EXIT_SUCCESS = 0;
const EXIT_FAILURE = 1;
const SIGNAL_OFFSET= 128;
let hasInspect = false;

const opts = loadOptions(process.argv.slice(2));
Expand Down Expand Up @@ -109,7 +113,15 @@ if (mochaArgs['node-option'] || Object.keys(nodeArgs).length || hasInspect) {
proc.on('exit', (code, signal) => {
process.on('exit', () => {
if (signal) {
process.kill(process.pid, signal);
const numericSignal =
typeof signal === 'string' ? os.constants.signals[signal] : signal;
Comment on lines +116 to +117
Copy link
Member

Choose a reason for hiding this comment

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

According to the documentation the signal argument of the exit event on child_process.spawn() is always a string – when is it ever something else?

if (mochaArgs['posix-exit-codes'] === true) {
process.exit(SIGNAL_OFFSET + numericSignal);
} else {
process.kill(process.pid, signal);
}
Comment on lines +118 to +122
Copy link
Member

Choose a reason for hiding this comment

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

If the process.kill(process.pid, signal) is important because it will trigger handles like the process.on('SIGINT', then that's what always should be used here.

Else the process.exit() variant should always be used.

Though the logic of Mocha here is kind of weird – the signal comes from the child process, and the SIGNAL_OFFSET + numericSignal should be sent if the process itself, not one of its childs, gets sent the signal – so I would say that process.exit(SIGNAL_OFFSET + numericSignal) here is wrong and that the original process.kill(process.pid, signal) is weird (as its essentially bubbling up the process signal? Is that common practice?)

} else if (code !== 0 && mochaArgs['posix-exit-codes'] === true) {
process.exit(EXIT_FAILURE);
} else {
process.exit(code);
73rhodes marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +123 to 126
Copy link
Member

Choose a reason for hiding this comment

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

Could probably make it this:

Suggested change
} else if (code !== 0 && mochaArgs['posix-exit-codes'] === true) {
process.exit(EXIT_FAILURE);
} else {
process.exit(code);
} else {
process.exit(Math.min(code, mochaArgs['posix-exit-codes'] ? 1 : 255));

That reuses the Math.min(code, 255) logic that's in other places.

Not sure the EXIT_FAILURE helps much here, the direct 1 makes it possible to use the Math.min like this

}
Expand All @@ -126,7 +138,7 @@ if (mochaArgs['node-option'] || Object.keys(nodeArgs).length || hasInspect) {
// be needed.
if (!args.parallel || args.jobs < 2) {
// win32 does not support SIGTERM, so use next best thing.
if (require('os').platform() === 'win32') {
if (os.platform() === 'win32') {
proc.kill('SIGKILL');
} else {
// using SIGKILL won't cleanly close the output streams, which can result
Expand Down
12 changes: 12 additions & 0 deletions docs/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -936,6 +936,18 @@ Define a global variable name. For example, suppose your app deliberately expose

By using this option in conjunction with `--check-leaks`, you can specify a whitelist of known global variables that you _expect_ to leak into global scope.

### `--posix-exit-codes`
73rhodes marked this conversation as resolved.
Show resolved Hide resolved

Exits with standard POSIX exit codes instead of the number of failed tests.
73rhodes marked this conversation as resolved.
Show resolved Hide resolved

Those exit codes are:

* `0`: if all tests passed
* `1`: if any test failed
* `128 + <signal>` if given a signal, such as:
* 134: `SIGABRT` (`128 + 6`)
* 143: `SIGTERM` (`128 + 15`)

73rhodes marked this conversation as resolved.
Show resolved Hide resolved
### `--retries <n>`

Retries failed tests `n` times.
Expand Down
1 change: 1 addition & 0 deletions lib/cli/run-option-metadata.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ const TYPES = (exports.types = {
'list-reporters',
'no-colors',
'parallel',
'posix-exit-codes',
'recursive',
'sort',
'watch'
Expand Down
4 changes: 4 additions & 0 deletions lib/cli/run.js
73rhodes marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,10 @@ exports.builder = yargs =>
description: 'Run tests in parallel',
group: GROUPS.RULES
},
'posix-exit-codes': {
description: 'Use POSIX and UNIX shell exit codes as Mocha\'s return value',
group: GROUPS.RULES
},
recursive: {
description: 'Look for tests in subdirectories',
group: GROUPS.FILES
Expand Down
23 changes: 23 additions & 0 deletions test/integration/fixtures/failing.fixture.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';

// One passing test and three failing tests

var assert = require('assert');

describe('suite', function () {
it('test1', function () {
assert(true);
});

it('test2', function () {
assert(false);
});

it('test3', function () {
assert(false);
});

it('test4', function () {
assert(false);
});
});
7 changes: 7 additions & 0 deletions test/integration/fixtures/signals-sigabrt.fixture.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use strict';

describe('signal suite', function () {
it('test SIGABRT', function () {
process.kill(process.pid, 'SIGABRT');
});
});
7 changes: 7 additions & 0 deletions test/integration/fixtures/signals-sigterm.fixture.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use strict';

describe('signal suite', function () {
it('test SIGTERM', function () {
process.kill(process.pid, 'SIGTERM');
});
});
63 changes: 63 additions & 0 deletions test/integration/options/posixExitCodes.spec.js
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
'use strict';

var helpers = require('../helpers');
var runMocha = helpers.runMocha;
var os = require('os');

const EXIT_SUCCESS = 0;
const EXIT_FAILURE = 1;
const SIGNAL_OFFSET = 128;

describe('--posix-exit-codes', function () {
describe('when enabled, and with node options', function () {
var args = ['--no-warnings', '--posix-exit-codes'];
73rhodes marked this conversation as resolved.
Show resolved Hide resolved

it('should exit with correct POSIX shell code on SIGABRT', function (done) {
var fixture = 'signals-sigabrt.fixture.js';
runMocha(fixture, args, function postmortem(err, res) {
if (err) {
return done(err);
}
expect(res.code, 'to be', SIGNAL_OFFSET + os.constants.signals.SIGABRT);
done();
});
});

it('should exit with correct POSIX shell code on SIGTERM', function (done) {
var fixture = 'signals-sigterm.fixture.js';
runMocha(fixture, args, function postmortem(err, res) {
if (err) {
return done(err);
}
expect(res.code, 'to be', SIGNAL_OFFSET + os.constants.signals.SIGTERM);
done();
});
});

it('should exit with code 1 if there are test failures', function (done) {
var fixture = 'failing.fixture.js';
runMocha(fixture, args, function postmortem(err, res) {
if (err) {
return done(err);
}
expect(res.code, 'to be', EXIT_FAILURE);
done();
});
});
});

73rhodes marked this conversation as resolved.
Show resolved Hide resolved
describe('when not enabled, and with node options', function () {
it('should exit with the number of failed tests', function (done) {
73rhodes marked this conversation as resolved.
Show resolved Hide resolved
var fixture = 'failing.fixture.js'; // contains three failing tests
var numFailures = 3;
var args = ['--no-warnings'];
runMocha(fixture, args, function postmortem(err, res) {
if (err) {
return done(err);
}
expect(res.code, 'to be', numFailures);
done();
});
});
});
});
Loading