test_runner: count nested tests

PR-URL: #47094 Fixes: #46762 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
nodejs · Jul 6, 2023 · 460bcc0 · 460bcc0
1 parent d562a7c
commit 460bcc0
Show file tree

Hide file tree

Showing 21 changed files with 163 additions and 93 deletions.
diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js
@@ -166,6 +166,17 @@ function setup(root) {
     __proto__: null,
     bootstrapComplete: false,
     coverage: null,
+    counters: {
+      __proto__: null,
+      all: 0,
+      failed: 0,
+      passed: 0,
+      cancelled: 0,
+      skipped: 0,
+      todo: 0,
+      planned: 0,
+      suites: 0,
+    },
   };
   root.startTime = hrtime();
   return root;

diff --git a/lib/internal/test_runner/reporter/tap.js b/lib/internal/test_runner/reporter/tap.js
@@ -113,6 +113,7 @@ function reportDetails(nesting, data = kEmptyObject) {
   let details = `${_indent}  ---\n`;
 
   details += jsToYaml(_indent, 'duration_ms', duration_ms);
+  details += jsToYaml(_indent, 'type', data.type);
   details += jsToYaml(_indent, null, error);
   details += `${_indent}  ...\n`;
   return details;

diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js
@@ -10,10 +10,8 @@ const {
   ArrayPrototypeSome,
   ArrayPrototypeSort,
   ArrayPrototypeSplice,
-  FunctionPrototypeCall,
   Number,
   ObjectAssign,
-  ObjectKeys,
   PromisePrototypeThen,
   SafePromiseAll,
   SafePromiseAllReturnVoid,
@@ -55,8 +53,9 @@ const { YAMLToJs } = require('internal/test_runner/yaml_to_js');
 const { TokenKind } = require('internal/test_runner/tap_lexer');
 
 const {
-  isSupportedFileType,
+  countCompletedTest,
   doesPathMatchFilter,
+  isSupportedFileType,
 } = require('internal/test_runner/utils');
 const { basename, join, resolve } = require('path');
 const { once } = require('events');
@@ -66,7 +65,7 @@ const {
 
 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 kDiagnosticsFilterArgs = ['tests', 'suites', 'pass', 'fail', 'cancelled', 'skipped', 'todo', 'duration_ms'];
 
 const kCanceledTests = new SafeSet()
   .add(kCancelledByParent).add(kAborted).add(kTestTimeoutFailure);
@@ -150,10 +149,10 @@ 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 };
+  #reportedChildren = 0;
   failedSubtests = false;
   #skipReporting() {
-    return this.#counters.all > 0 && (!this.error || this.error.failureType === kSubtestsFailed);
+    return this.#reportedChildren > 0 && (!this.error || this.error.failureType === kSubtestsFailed);
   }
   #checkNestedComment({ comment }) {
     const firstSpaceIndex = StringPrototypeIndexOf(comment, ' ');
@@ -203,11 +202,19 @@ class FileTest extends Test {
         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;
         }
+        this.#reportedChildren++;
+        countCompletedTest({
+          name: node.description,
+          finished: true,
+          skipped: skip,
+          isTodo: todo,
+          passed: pass,
+          cancelled,
+          nesting,
+          reportedType: diagnostics.type,
+        }, this.root.harness);
         break;
 
       }
@@ -232,14 +239,6 @@ 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() {
     const skipReporting = this.#skipReporting();

diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
@@ -34,6 +34,7 @@ const { MockTracker } = require('internal/test_runner/mock');
 const { TestsStream } = require('internal/test_runner/tests_stream');
 const {
   createDeferredCallback,
+  countCompletedTest,
   isTestFailureError,
   parseCommandLine,
 } = require('internal/test_runner/utils');
@@ -186,6 +187,7 @@ class Test extends AsyncResource {
       this.runOnlySubtests = this.only;
       this.testNumber = 0;
       this.timeout = kDefaultTimeout;
+      this.root = this;
     } else {
       const nesting = parent.parent === null ? parent.nesting :
         parent.nesting + 1;
@@ -197,6 +199,7 @@ class Test extends AsyncResource {
       this.runOnlySubtests = !this.only;
       this.testNumber = parent.subtests.length + 1;
       this.timeout = parent.timeout;
+      this.root = parent.root;
     }
 
     switch (typeof concurrency) {
@@ -575,31 +578,7 @@ class Test extends AsyncResource {
     this.postRun();
   }
 
-  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) {
@@ -610,19 +589,22 @@ class Test extends AsyncResource {
     // The test has run, so recursively cancel any outstanding subtests and
     // mark this test as failed if any subtests failed.
     this.pendingSubtests = [];
+    let failed = 0;
     for (let i = 0; i < this.subtests.length; i++) {
       const subtest = this.subtests[i];
 
       if (!subtest.finished) {
         subtest.#cancel(pendingSubtestsError);
         subtest.postRun(pendingSubtestsError);
       }
-      subtest.countSubtest(counters);
+      if (!subtest.passed) {
+        failed++;
+      }
     }
 
-    if ((this.passed || this.parent === null) && counters.totalFailed > 0) {
-      const subtestString = `subtest${counters.totalFailed > 1 ? 's' : ''}`;
-      const msg = `${counters.totalFailed} ${subtestString} failed`;
+    if ((this.passed || this.parent === null) && failed > 0) {
+      const subtestString = `subtest${failed > 1 ? 's' : ''}`;
+      const msg = `${failed} ${subtestString} failed`;
 
       this.fail(new ERR_TEST_FAILURE(msg, kSubtestsFailed));
     }
@@ -637,18 +619,19 @@ class Test extends AsyncResource {
       this.parent.processPendingSubtests();
     } else if (!this.reported) {
       this.reported = true;
-      this.reporter.plan(this.nesting, kFilename, counters.all);
+      this.reporter.plan(this.nesting, kFilename, this.root.harness.counters.planned);
 
       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 ${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}`);
-      this.reporter.diagnostic(this.nesting, kFilename, `skipped ${counters.skipped}`);
-      this.reporter.diagnostic(this.nesting, kFilename, `todo ${counters.todo}`);
+      this.reporter.diagnostic(this.nesting, kFilename, `tests ${this.root.harness.counters.all}`);
+      this.reporter.diagnostic(this.nesting, kFilename, `suites ${this.root.harness.counters.suites}`);
+      this.reporter.diagnostic(this.nesting, kFilename, `pass ${this.root.harness.counters.passed}`);
+      this.reporter.diagnostic(this.nesting, kFilename, `fail ${this.root.harness.counters.failed}`);
+      this.reporter.diagnostic(this.nesting, kFilename, `cancelled ${this.root.harness.counters.cancelled}`);
+      this.reporter.diagnostic(this.nesting, kFilename, `skipped ${this.root.harness.counters.skipped}`);
+      this.reporter.diagnostic(this.nesting, kFilename, `todo ${this.root.harness.counters.todo}`);
       this.reporter.diagnostic(this.nesting, kFilename, `duration_ms ${this.#duration()}`);
 
       if (this.harness?.coverage) {
@@ -689,6 +672,7 @@ class Test extends AsyncResource {
   }
 
   report() {
+    countCompletedTest(this);
     if (this.subtests.length > 0) {
       this.reporter.plan(this.subtests[0].nesting, kFilename, this.subtests.length);
     } else {
@@ -703,6 +687,10 @@ class Test extends AsyncResource {
       directive = this.reporter.getTodo(this.message);
     }
 
+    if (this.reportedType) {
+      details.type = this.reportedType;
+    }
+
     if (this.passed) {
       this.reporter.ok(this.nesting, kFilename, this.testNumber, this.name, details, directive);
     } else {
@@ -746,6 +734,7 @@ class TestHook extends Test {
 }
 
 class Suite extends Test {
+  reportedType = 'suite';
   constructor(options) {
     super(options);
 

diff --git a/lib/internal/test_runner/utils.js b/lib/internal/test_runner/utils.js
@@ -223,8 +223,33 @@ function parseCommandLine() {
   return globalTestOptions;
 }
 
+function countCompletedTest(test, harness = test.root.harness) {
+  if (test.nesting === 0) {
+    harness.counters.planned++;
+  }
+  if (test.reportedType === 'suite') {
+    harness.counters.suites++;
+    return;
+  }
+  // Check SKIP and TODO tests first, as those should not be counted as
+  // failures.
+  if (test.skipped) {
+    harness.counters.skipped++;
+  } else if (test.isTodo) {
+    harness.counters.todo++;
+  } else if (test.cancelled) {
+    harness.counters.cancelled++;
+  } else if (!test.passed) {
+    harness.counters.failed++;
+  } else {
+    harness.counters.passed++;
+  }
+  harness.counters.all++;
+}
+
 module.exports = {
   convertStringToRegExp,
+  countCompletedTest,
   createDeferredCallback,
   doesPathMatchFilter,
   isSupportedFileType,

diff --git a/test/message/test_runner_abort.out b/test/message/test_runner_abort.out
@@ -260,10 +260,11 @@ not ok 4 - callback abort signal
     *
   ...
 1..4
-# tests 4
-# pass 0
+# tests 22
+# suites 0
+# pass 8
 # fail 0
-# cancelled 4
+# cancelled 14
 # skipped 0
 # todo 0
 # duration_ms *
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: *
+  type: 'suite'
   failureType: 'testAborted'
   error: 'The operation was aborted due to timeout'
   code: 23
@@ -78,6 +79,7 @@ not ok 1 - describe timeout signal
 not ok 2 - describe abort signal
   ---
   duration_ms: *
+  type: 'suite'
   failureType: 'testAborted'
   error: 'This operation was aborted'
   code: 20
@@ -94,10 +96,11 @@ not ok 2 - describe abort signal
     *
   ...
 1..2
-# tests 2
-# pass 0
+# tests 9
+# suites 2
+# pass 4
 # fail 0
-# cancelled 2
+# cancelled 5
 # skipped 0
 # todo 0
 # duration_ms *