From a955c534a8753610d3884ca24d502e4bd37c33e2 Mon Sep 17 00:00:00 2001
From: Moshe Atlow <moshe@atlow.co.il>
Date: Wed, 26 Jul 2023 22:18:41 +0300
Subject: [PATCH] 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            |  6 ++++--
 lib/internal/test_runner/harness.js         |  6 ++++--
 lib/internal/test_runner/test.js            |  6 +++---
 test/fixtures/test-runner/todo_exit_code.js | 15 +++++++++++++++
 test/parallel/test-runner-exit-code.js      | 13 +++++++++++++
 5 files changed, 39 insertions(+), 7 deletions(-)
 create mode 100644 test/fixtures/test-runner/todo_exit_code.js

diff --git a/lib/internal/main/test_runner.js b/lib/internal/main/test_runner.js
index 064731d77bede1..4974f4ce0d338b 100644
--- a/lib/internal/main/test_runner.js
+++ b/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;
+  }
 });
diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js
index 59669e48bf4daa..36c36f2de14b04 100644
--- a/lib/internal/test_runner/harness.js
+++ b/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);
   }
diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
index a53ea8b4fc1d44..0d8e3dbd6e8d3c 100644
--- a/lib/internal/test_runner/test.js
+++ b/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++;
       }
     }
diff --git a/test/fixtures/test-runner/todo_exit_code.js b/test/fixtures/test-runner/todo_exit_code.js
new file mode 100644
index 00000000000000..6577eefe52f7dc
--- /dev/null
+++ b/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')
+});
diff --git a/test/parallel/test-runner-exit-code.js b/test/parallel/test-runner-exit-code.js
index 8eeebc21d31753..700480386d5b4a 100644
--- a/test/parallel/test-runner-exit-code.js
+++ b/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);