test_runner: fix todo and only in spec reporter

lib/internal/main/test_runner.js

@@ -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;
+  }
 });

lib/internal/test_runner/harness.js

@@ -190,8 +190,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);
   }

lib/internal/test_runner/reporter/spec.js

@@ -64,17 +64,24 @@ class SpecReporter extends Transform {
       ), `\n${indent}  `);
     return `\n${indent}  ${message}\n`;
   }
-  #formatTestReport(type, data, prefix = '', indent = '', hasChildren = false, skippedSubtest = false) {
+  #formatTestReport(type, data, prefix = '', indent = '', hasChildren = false) {
     let color = colors[type] ?? white;
     let symbol = symbols[type] ?? ' ';
+    const { skip, todo } = data;
     const duration_ms = data.details?.duration_ms ? ` ${gray}(${data.details.duration_ms}ms)${white}` : '';
-    const title = `${data.name}${duration_ms}${skippedSubtest ? ' # SKIP' : ''}`;
+    let title = `${data.name}${duration_ms}`;
+
+    if (skip !== undefined) {
+      title += ` # ${typeof skip === 'string' && skip.length ? skip : 'SKIP'}`;
+    } else if (todo !== undefined) {
+      title += ` # ${typeof todo === 'string' && todo.length ? todo : 'TODO'}`;
+    }
     if (hasChildren) {
       // If this test has had children - it was already reported, so slightly modify the output
       return `${prefix}${indent}${color}${symbols['arrow:right']}${white}${title}\n`;
     }
     const error = this.#formatError(data.details?.error, indent);
-    if (skippedSubtest) {
+    if (skip !== undefined) {
       color = gray;
       symbol = symbols['hyphen:minus'];
     }
@@ -101,9 +108,8 @@ class SpecReporter extends Transform {
       ArrayPrototypeShift(this.#reported);
       hasChildren = true;
     }
-    const skippedSubtest = subtest && data.skip && data.skip !== undefined;
     const indent = this.#indent(data.nesting);
-    return `${this.#formatTestReport(type, data, prefix, indent, hasChildren, skippedSubtest)}\n`;
+    return `${this.#formatTestReport(type, data, prefix, indent, hasChildren)}\n`;
   }
   #handleEvent({ type, data }) {
     switch (type) {

lib/internal/test_runner/test.js

@@ -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;
@@ -634,7 +634,7 @@ class Test extends AsyncResource {
         subtest.#cancel(pendingSubtestsError);
         subtest.postRun(pendingSubtestsError);
       }
-      if (!subtest.passed) {
+      if (!subtest.passed && !subtest.isTodo) {
         failed++;
       }
     }

test/fixtures/test-runner/output/describe_it.js

@@ -13,12 +13,12 @@ it.todo('sync pass todo', () => {
 it('sync pass todo with message', { todo: 'this is a passing todo' }, () => {
 });
 
-it.todo('sync fail todo', () => {
-  throw new Error('thrown from sync fail todo');
+it.todo('sync todo', () => {
+  throw new Error('should not count as a failure');
 });
 
-it('sync fail todo with message', { todo: 'this is a failing todo' }, () => {
-  throw new Error('thrown from sync fail todo with message');
+it('sync todo with message', { todo: 'this is a failing todo' }, () => {
+  throw new Error('should not count as a failure');
 });
 
 it.skip('sync skip pass', () => {

test/fixtures/test-runner/output/describe_it.snapshot

@@ -9,12 +9,12 @@ ok 2 - sync pass todo with message # TODO this is a passing todo
   ---
   duration_ms: *
   ...
-# Subtest: sync fail todo
-not ok 3 - sync fail todo # TODO
+# Subtest: sync todo
+not ok 3 - sync todo # TODO
   ---
   duration_ms: *
   failureType: 'testCodeFailure'
-  error: 'thrown from sync fail todo'
+  error: 'should not count as a failure'
   code: 'ERR_TEST_FAILURE'
   stack: |-
     *
@@ -25,12 +25,12 @@ not ok 3 - sync fail todo # TODO
     *
     *
   ...
-# Subtest: sync fail todo with message
-not ok 4 - sync fail todo with message # TODO this is a failing todo
+# Subtest: sync todo with message
+not ok 4 - sync todo with message # TODO this is a failing todo
   ---
   duration_ms: *
   failureType: 'testCodeFailure'
-  error: 'thrown from sync fail todo with message'
+  error: 'should not count as a failure'
   code: 'ERR_TEST_FAILURE'
   stack: |-
     *

test/fixtures/test-runner/output/spec_reporter.snapshot

@@ -1,6 +1,6 @@
- sync pass todo (*ms)
- sync pass todo with message (*ms)
- sync fail todo (*ms)
+ sync pass todo (*ms) # TODO
+ sync pass todo with message (*ms) # this is a passing todo
+ sync fail todo (*ms) # TODO
   Error: thrown from sync fail todo
       *
       *
@@ -10,7 +10,7 @@
       *
       *
 
- sync fail todo with message (*ms)
+ sync fail todo with message (*ms) # this is a failing todo
   Error: thrown from sync fail todo with message
       *
       *
@@ -21,7 +21,7 @@
       *
 
  sync skip pass (*ms) # SKIP
- sync skip pass with message (*ms) # SKIP
+ sync skip pass with message (*ms) # this is skipped
  sync pass (*ms)
  this test should pass
  sync throw fail (*ms)
@@ -130,7 +130,7 @@
 
  invalid subtest - pass but subtest fails (*ms)
  sync skip option (*ms) # SKIP
- sync skip option with message (*ms) # SKIP
+ sync skip option with message (*ms) # this is skipped
  sync skip option is false fail (*ms)
   Error: this should be executed
       *
@@ -151,8 +151,8 @@
  functionAndOptions (*ms) # SKIP
  escaped description \ # \#\ 
  	     (*ms)
- escaped skip message (*ms) # SKIP
- escaped todo message (*ms)
+ escaped skip message (*ms) # #skip
+ escaped todo message (*ms) # #todo
  escaped diagnostic (*ms)
  #diagnostic
  callback pass (*ms)
@@ -307,7 +307,7 @@
 
  failing tests:
 
- sync fail todo (*ms)
+ sync fail todo (*ms) # TODO
   Error: thrown from sync fail todo
       *
       *
@@ -317,7 +317,7 @@
       *
       *
 
- sync fail todo with message (*ms)
+ sync fail todo with message (*ms) # this is a failing todo
   Error: thrown from sync fail todo with message
       *
       *
@@ -347,7 +347,7 @@
       *
       *
 
- async skip fail (*ms)
+ async skip fail (*ms) # SKIP
   Error: thrown from async throw fail
       *
       *

test/fixtures/test-runner/output/spec_reporter_cli.snapshot

@@ -1,6 +1,6 @@
- sync pass todo (*ms)
- sync pass todo with message (*ms)
- sync fail todo (*ms)
+ sync pass todo (*ms) # TODO
+ sync pass todo with message (*ms) # this is a passing todo
+ sync fail todo (*ms) # TODO
   Error: thrown from sync fail todo
       *
       *
@@ -10,7 +10,7 @@
       *
       *
 
- sync fail todo with message (*ms)
+ sync fail todo with message (*ms) # this is a failing todo
   Error: thrown from sync fail todo with message
       *
       *
@@ -21,7 +21,7 @@
       *
 
  sync skip pass (*ms) # SKIP
- sync skip pass with message (*ms) # SKIP
+ sync skip pass with message (*ms) # this is skipped
  sync pass (*ms)
  this test should pass
  sync throw fail (*ms)
@@ -130,7 +130,7 @@
 
  invalid subtest - pass but subtest fails (*ms)
  sync skip option (*ms) # SKIP
- sync skip option with message (*ms) # SKIP
+ sync skip option with message (*ms) # this is skipped
  sync skip option is false fail (*ms)
   Error: this should be executed
       *
@@ -151,8 +151,8 @@
  functionAndOptions (*ms) # SKIP
  escaped description \ # \#\ 
  	     (*ms)
- escaped skip message (*ms) # SKIP
- escaped todo message (*ms)
+ escaped skip message (*ms) # #skip
+ escaped todo message (*ms) # #todo
  escaped diagnostic (*ms)
  #diagnostic
  callback pass (*ms)
@@ -307,7 +307,7 @@
 
  failing tests:
 
- sync fail todo (*ms)
+ sync fail todo (*ms) # TODO
   Error: thrown from sync fail todo
       *
       *
@@ -317,7 +317,7 @@
       *
       *
 
- sync fail todo with message (*ms)
+ sync fail todo with message (*ms) # this is a failing todo
   Error: thrown from sync fail todo with message
       *
       *
@@ -347,7 +347,7 @@
       *
       *
 
- async skip fail (*ms)
+ async skip fail (*ms) # SKIP
   Error: thrown from async throw fail
       *
       *

test/fixtures/test-runner/todo_exit_code.js

@@ -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')
+});

test/parallel/test-runner-exit-code.js

@@ -50,6 +50,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);