test_runner: dont set exit code on todo tests

PR-URL: https://github.com/nodejs/node/pull/48929
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

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

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

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/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);