test_runner: flatten TAP output when running using --test

PR-URL: #46440 Fixes: #45833 Refs: #45833 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow · Nov 7, 2022 · Dec 19, 2022 · Dec 22, 2022 · Jan 2, 2023 · Jan 6, 2023
commit f7d8bedce342e963a375efb22458d6051aee1592
diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js
@@ -8,9 +8,11 @@ const {
   ArrayPrototypeSlice,
   ArrayPrototypeSome,
   ArrayPrototypeSort,
+  FunctionPrototypeCall,
+  Number,
   ObjectAssign,
+  ObjectKeys,
   PromisePrototypeThen,
-  SafePromiseAll,
   SafePromiseAllReturnVoid,
   SafePromiseAllSettledReturnVoid,
   SafeMap,
@@ -35,7 +37,14 @@ const { validateArray, validateBoolean } = require('internal/validators');
 const { getInspectPort, isUsingInspector, isInspectorMessage } = require('internal/util/inspector');
 const { kEmptyObject } = require('internal/util');
 const { createTestTree } = require('internal/test_runner/harness');
-const { kSubtestsFailed, Test } = require('internal/test_runner/test');
+const {
+  kAborted,
+  kCancelledByParent,
+  kSubtestsFailed,
+  kTestCodeFailure,
+  kTestTimeoutFailure,
+  Test,
+} = require('internal/test_runner/test');
 const { TapParser } = require('internal/test_runner/tap_parser');
 const { YAMLToJs } = require('internal/test_runner/yaml_to_js');
 const { TokenKind } = require('internal/test_runner/tap_lexer');
@@ -54,6 +63,9 @@ const kFilterArgs = ['--test', '--experimental-test-coverage', '--watch'];
 const kFilterArgValues = ['--test-reporter', '--test-reporter-destination'];
 const kDiagnosticsFilterArgs = ['tests', 'pass', 'fail', 'cancelled', 'skipped', 'todo', 'duration_ms'];
 
+const kCanceledTests = new SafeSet()
+  .add(kCancelledByParent).add(kAborted).add(kTestTimeoutFailure);
+
 // TODO(cjihrig): Replace this with recursive readdir once it lands.
 function processPath(path, testFiles, options) {
   const stats = statSync(path);
@@ -132,6 +144,11 @@ function getRunArgs({ path, inspectPort }) {
 
 class FileTest extends Test {
   #buffer = [];
+  #counters = { __proto__: null, all: 0, failed: 0, passed: 0, cancelled: 0, skipped: 0, todo: 0, totalFailed: 0 };
+  failedSubtests = false;
+  #skipReporting() {
+    return this.#counters.all > 0 && (!this.error || this.error.failureType === kSubtestsFailed);
+  }
   #checkNestedComment({ comment }) {
     const firstSpaceIndex = StringPrototypeIndexOf(comment, ' ');
     if (firstSpaceIndex === -1) return false;
@@ -140,8 +157,6 @@ class FileTest extends Test {
           ArrayPrototypeIncludes(kDiagnosticsFilterArgs, StringPrototypeSlice(comment, 0, firstSpaceIndex));
   }
   #handleReportItem({ kind, node, comments, nesting = 0 }) {
-    nesting += 1;
-
     if (comments) {
       ArrayPrototypeForEach(comments, (comment) => this.reporter.diagnostic(nesting, this.name, comment));
     }
@@ -152,17 +167,20 @@ class FileTest extends Test {
         break;
 
       case TokenKind.TAP_PLAN:
+        if (nesting === 0 && this.#skipReporting()) {
+          break;
+        }
         this.reporter.plan(nesting, this.name, node.end - node.start + 1);
         break;
 
       case TokenKind.TAP_SUBTEST_POINT:
         this.reporter.start(nesting, this.name, node.name);
         break;
 
-      case TokenKind.TAP_TEST_POINT:
-        // eslint-disable-next-line no-case-declarations
+      case TokenKind.TAP_TEST_POINT: {
+
         const { todo, skip, pass } = node.status;
-        // eslint-disable-next-line no-case-declarations
+
         let directive;
 
         if (skip) {
@@ -173,29 +191,22 @@ class FileTest extends Test {
           directive = kEmptyObject;
         }
 
-        if (pass) {
-          this.reporter.ok(
-            nesting,
-            this.name,
-            node.id,
-            node.description,
-            YAMLToJs(node.diagnostics),
-            directive
-          );
-        } else {
-          this.reporter.fail(
-            nesting,
-            this.name,
-            node.id,
-            node.description,
-            YAMLToJs(node.diagnostics),
-            directive
-          );
+        const diagnostics = YAMLToJs(node.diagnostics);
+        const cancelled = kCanceledTests.has(diagnostics.error?.failureType);
+        const testNumber = nesting === 0 ? (Number(node.id) + this.testNumber - 1) : node.id;
+        const method = pass ? 'ok' : 'fail';
+        this.reporter[method](nesting, this.name, testNumber, node.description, diagnostics, directive);
+        if (nesting === 0) {
+          FunctionPrototypeCall(super.countSubtest,
+                                { finished: true, skipped: skip, isTodo: todo, passed: pass, cancelled },
+                                this.#counters);
+          this.failedSubtests ||= !pass;
         }
         break;
 
+      }
       case TokenKind.COMMENT:
-        if (nesting === 1 && this.#checkNestedComment(node)) {
+        if (nesting === 0 && this.#checkNestedComment(node)) {
           // Ignore file top level diagnostics
           break;
         }
@@ -215,10 +226,24 @@ class FileTest extends Test {
     this.reportStarted();
     this.#handleReportItem(ast);
   }
+  countSubtest(counters) {
+    if (this.#counters.all === 0) {
+      return super.countSubtest(counters);
+    }
+    ArrayPrototypeForEach(ObjectKeys(counters), (key) => {
+      counters[key] += this.#counters[key];
+    });
+  }
+  reportStarted() {}
   report() {
-    this.reportStarted();
+    const skipReporting = this.#skipReporting();
+    if (!skipReporting) {
+      super.reportStarted();
+    }
     ArrayPrototypeForEach(this.#buffer, (ast) => this.#handleReportItem(ast));
-    super.report();
+    if (!skipReporting) {
+      super.report();
+    }
   }
 }
 
@@ -273,16 +298,14 @@ function runTestFile(path, root, inspectPort, filesWatcher) {
       subtest.addToReport(ast);
     });
 
-    const { 0: { 0: code, 1: signal } } = await SafePromiseAll([
-      once(child, 'exit', { signal: t.signal }),
-      child.stdout.toArray({ signal: t.signal }),
-    ]);
+    const { 0: code, 1: signal } = await once(child, 'exit', { signal: t.signal });
 
     runningProcesses.delete(path);
     runningSubtests.delete(path);
     if (code !== 0 || signal !== null) {
       if (!err) {
-        err = ObjectAssign(new ERR_TEST_FAILURE('test failed', kSubtestsFailed), {
+        const failureType = subtest.failedSubtests ? kSubtestsFailed : kTestCodeFailure;
+        err = ObjectAssign(new ERR_TEST_FAILURE('test failed', failureType), {
           __proto__: null,
           exitCode: code,
           signal: signal,

diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
@@ -57,6 +57,7 @@ const { cpus } = require('os');
 const { bigint: hrtime } = process.hrtime;
 const kCallbackAndPromisePresent = 'callbackAndPromisePresent';
 const kCancelledByParent = 'cancelledByParent';
+const kAborted = 'testAborted';
 const kParentAlreadyFinished = 'parentAlreadyFinished';
 const kSubtestsFailed = 'subtestsFailed';
 const kTestCodeFailure = 'testCodeFailure';
@@ -390,10 +391,12 @@ class Test extends AsyncResource {
   }
 
   #abortHandler = () => {
-    this.cancel(this.#outerSignal?.reason || new AbortError('The test was aborted'));
+    const error = this.#outerSignal?.reason || new AbortError('The test was aborted');
+    error.failureType = kAborted;
+    this.#cancel(error);
   };
 
-  cancel(error) {
+  #cancel(error) {
     if (this.endTime !== null) {
       return;
     }
@@ -470,7 +473,7 @@ class Test extends AsyncResource {
       return true;
     }
     if (this.#outerSignal?.aborted) {
-      this.cancel(this.#outerSignal.reason || new AbortError('The test was aborted'));
+      this.#abortHandler();
       return true;
     }
   }
@@ -563,7 +566,7 @@ class Test extends AsyncResource {
       try { await afterEach(); } catch { /* test is already failing, let's the error */ }
       if (isTestFailureError(err)) {
         if (err.failureType === kTestTimeoutFailure) {
-          this.cancel(err);
+          this.#cancel(err);
         } else {
           this.fail(err);
         }
@@ -577,9 +580,31 @@ class Test extends AsyncResource {
     this.postRun();
   }
 
-  postRun(pendingSubtestsError) {
-    const counters = { __proto__: null, failed: 0, passed: 0, cancelled: 0, skipped: 0, todo: 0, totalFailed: 0 };
+  countSubtest(counters) {
+    // Check SKIP and TODO tests first, as those should not be counted as
+    // failures.
+    if (this.skipped) {
+      counters.skipped++;
+    } else if (this.isTodo) {
+      counters.todo++;
+    } else if (this.cancelled) {
+      counters.cancelled++;
+    } else if (!this.passed) {
+      counters.failed++;
+    } else {
+      counters.passed++;
+    }
+
+    if (!this.passed) {
+      counters.totalFailed++;
+    }
+    counters.all++;
+  }
 
+  postRun(pendingSubtestsError) {
+    const counters = {
+      __proto__: null, all: 0, failed: 0, passed: 0, cancelled: 0, skipped: 0, todo: 0, totalFailed: 0,
+    };
     // If the test was failed before it even started, then the end time will
     // be earlier than the start time. Correct that here.
     if (this.endTime < this.startTime) {
@@ -594,27 +619,10 @@ class Test extends AsyncResource {
       const subtest = this.subtests[i];
 
       if (!subtest.finished) {
-        subtest.cancel(pendingSubtestsError);
+        subtest.#cancel(pendingSubtestsError);
         subtest.postRun(pendingSubtestsError);
       }
-
-      // Check SKIP and TODO tests first, as those should not be counted as
-      // failures.
-      if (subtest.skipped) {
-        counters.skipped++;
-      } else if (subtest.isTodo) {
-        counters.todo++;
-      } else if (subtest.cancelled) {
-        counters.cancelled++;
-      } else if (!subtest.passed) {
-        counters.failed++;
-      } else {
-        counters.passed++;
-      }
-
-      if (!subtest.passed) {
-        counters.totalFailed++;
-      }
+      subtest.countSubtest(counters);
     }
 
     if ((this.passed || this.parent === null) && counters.totalFailed > 0) {
@@ -634,13 +642,13 @@ class Test extends AsyncResource {
       this.parent.processPendingSubtests();
     } else if (!this.reported) {
       this.reported = true;
-      this.reporter.plan(this.nesting, kFilename, this.subtests.length);
+      this.reporter.plan(this.nesting, kFilename, counters.all);
 
       for (let i = 0; i < this.diagnostics.length; i++) {
         this.reporter.diagnostic(this.nesting, kFilename, this.diagnostics[i]);
       }
 
-      this.reporter.diagnostic(this.nesting, kFilename, `tests ${this.subtests.length}`);
+      this.reporter.diagnostic(this.nesting, kFilename, `tests ${counters.all}`);
       this.reporter.diagnostic(this.nesting, kFilename, `pass ${counters.passed}`);
       this.reporter.diagnostic(this.nesting, kFilename, `fail ${counters.failed}`);
       this.reporter.diagnostic(this.nesting, kFilename, `cancelled ${counters.cancelled}`);
@@ -825,6 +833,8 @@ module.exports = {
   kCancelledByParent,
   kSubtestsFailed,
   kTestCodeFailure,
+  kTestTimeoutFailure,
+  kAborted,
   kUnwrapErrors,
   Suite,
   Test,

diff --git a/test/message/test_runner_abort.out b/test/message/test_runner_abort.out
@@ -40,6 +40,7 @@ TAP version 13
     not ok 7 - not ok 3
       ---
       duration_ms: *
+      failureType: 'testAborted'
       error: 'This operation was aborted'
       code: 20
       stack: |-
@@ -58,6 +59,7 @@ TAP version 13
     not ok 8 - not ok 4
       ---
       duration_ms: *
+      failureType: 'testAborted'
       error: 'This operation was aborted'
       code: 20
       stack: |-
@@ -76,6 +78,7 @@ TAP version 13
     not ok 9 - not ok 5
       ---
       duration_ms: *
+      failureType: 'testAborted'
       error: 'This operation was aborted'
       code: 20
       stack: |-
@@ -94,6 +97,7 @@ TAP version 13
 not ok 1 - promise timeout signal
   ---
   duration_ms: *
+  failureType: 'testAborted'
   error: 'The operation was aborted due to timeout'
   code: 23
   stack: |-
@@ -106,6 +110,7 @@ not ok 1 - promise timeout signal
 not ok 2 - promise abort signal
   ---
   duration_ms: *
+  failureType: 'testAborted'
   error: 'This operation was aborted'
   code: 20
   stack: |-
@@ -160,6 +165,7 @@ not ok 2 - promise abort signal
     not ok 7 - not ok 3
       ---
       duration_ms: *
+      failureType: 'testAborted'
       error: 'This operation was aborted'
       code: 20
       stack: |-
@@ -178,6 +184,7 @@ not ok 2 - promise abort signal
     not ok 8 - not ok 4
       ---
       duration_ms: *
+      failureType: 'testAborted'
       error: 'This operation was aborted'
       code: 20
       stack: |-
@@ -196,6 +203,7 @@ not ok 2 - promise abort signal
     not ok 9 - not ok 5
       ---
       duration_ms: *
+      failureType: 'testAborted'
       error: 'This operation was aborted'
       code: 20
       stack: |-
@@ -214,6 +222,7 @@ not ok 2 - promise abort signal
 not ok 3 - callback timeout signal
   ---
   duration_ms: *
+  failureType: 'testAborted'
   error: 'The operation was aborted due to timeout'
   code: 23
   stack: |-
@@ -226,6 +235,7 @@ not ok 3 - callback timeout signal
 not ok 4 - callback abort signal
   ---
   duration_ms: *
+  failureType: 'testAborted'
   error: 'This operation was aborted'
   code: 20
   stack: |-

diff --git a/test/message/test_runner_abort_suite.out b/test/message/test_runner_abort_suite.out
@@ -64,6 +64,7 @@ TAP version 13
 not ok 1 - describe timeout signal
   ---
   duration_ms: *
+  failureType: 'testAborted'
   error: 'The operation was aborted due to timeout'
   code: 23
   stack: |-
@@ -76,6 +77,7 @@ not ok 1 - describe timeout signal
 not ok 2 - describe abort signal
   ---
   duration_ms: *
+  failureType: 'testAborted'
   error: 'This operation was aborted'
   code: 20
   stack: |-