Skip to content

Commit

Permalink
test_runner: dont set exit code on todo tests
Browse files Browse the repository at this point in the history
PR-URL: #48929
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
  • Loading branch information
MoLow authored and RafaelGSS committed Aug 17, 2023
1 parent 52c9490 commit 9136667
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 7 deletions.
6 changes: 4 additions & 2 deletions lib/internal/main/test_runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ if (shardOption) {
}

run({ concurrency, inspectPort, watch: getOptionValue('--watch'), setup: setupTestReporters, shard })
.once('test:fail', () => {
process.exitCode = kGenericUserError;
.on('test:fail', (data) => {
if (data.todo === undefined || data.todo === false) {
process.exitCode = kGenericUserError;
}
});
6 changes: 4 additions & 2 deletions lib/internal/test_runner/harness.js
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,10 @@ let reportersSetup;
function getGlobalRoot() {
if (!globalRoot) {
globalRoot = createTestTree();
globalRoot.reporter.once('test:fail', () => {
process.exitCode = kGenericUserError;
globalRoot.reporter.on('test:fail', (data) => {
if (data.todo === undefined || data.todo === false) {
process.exitCode = kGenericUserError;
}
});
reportersSetup = setupTestReporters(globalRoot);
}
Expand Down
6 changes: 3 additions & 3 deletions lib/internal/test_runner/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -282,8 +282,8 @@ class Test extends AsyncResource {
this.harness = null; // Configured on the root test by the test harness.
this.mock = null;
this.cancelled = false;
this.skipped = !!skip;
this.isTodo = !!todo;
this.skipped = skip !== undefined && skip !== false;
this.isTodo = todo !== undefined && todo !== false;
this.startTime = null;
this.endTime = null;
this.passed = false;
Expand Down Expand Up @@ -634,7 +634,7 @@ class Test extends AsyncResource {
subtest.#cancel(pendingSubtestsError);
subtest.postRun(pendingSubtestsError);
}
if (!subtest.passed) {
if (!subtest.passed && !subtest.isTodo) {
failed++;
}
}
Expand Down
15 changes: 15 additions & 0 deletions test/fixtures/test-runner/todo_exit_code.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
const { describe, test } = require('node:test');

describe('suite should pass', () => {
test.todo('should fail without harming suite', () => {
throw new Error('Fail but not badly')
});
});

test.todo('should fail without effecting exit code', () => {
throw new Error('Fail but not badly')
});

test('empty string todo', { todo: '' }, () => {
throw new Error('Fail but not badly')
});
13 changes: 13 additions & 0 deletions test/parallel/test-runner-exit-code.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,19 @@ if (process.argv[2] === 'child') {
assert.strictEqual(child.status, 0);
assert.strictEqual(child.signal, null);


child = spawnSync(process.execPath, [
'--test',
fixtures.path('test-runner', 'todo_exit_code.js'),
]);
assert.strictEqual(child.status, 0);
assert.strictEqual(child.signal, null);
const stdout = child.stdout.toString();
assert.match(stdout, /# tests 3/);
assert.match(stdout, /# pass 0/);
assert.match(stdout, /# fail 0/);
assert.match(stdout, /# todo 3/);

child = spawnSync(process.execPath, [__filename, 'child', 'fail']);
assert.strictEqual(child.status, 1);
assert.strictEqual(child.signal, null);
Expand Down

0 comments on commit 9136667

Please sign in to comment.