From 5024069d772dd64cc97ea1090488f557aebafa81 Mon Sep 17 00:00:00 2001
From: Timo Tijhof <krinkle@fastmail.com>
Date: Thu, 18 Jul 2024 21:13:12 +0100
Subject: [PATCH] Core: Convert "No tests were run." from fake test to error

* Remove hacky re-entrance from
  ProcessingQueue.done() -> test() + advance() -> done(),
  existed only for this purpose.

* Remove unused internal `test` injection to ProcessingQueue,
  existed only for this purpose.

* Remove unused internal `validTest` mechanism
  existed only for this purpose.

  This was originally impossible to trigger externally because it
  required setting `validTest` to a private symbol. In QUnit 1.16
  this was simplified as part of commit 3f08a1aa1e, to take any
  boolean true value to ease some implementation details, however it
  remained internal in purpose. A search today for `/validTest:/`
  and `/validTest = /` over public GitHub-hosted repositories, shows
  that nobody has unoffiically started relying on this. I found only
  copies of qunit itself.

* Remove "omit stack trace" logic in test.js,
  existed only for this purpose.
---
 demos/grunt-contrib-qunit.js                  |  6 +----
 demos/grunt-contrib-qunit/package.json        |  2 +-
 src/core/core.js                              |  2 +-
 src/core/processing-queue.js                  | 25 ++++---------------
 src/core/reporters/TapReporter.js             | 10 +++++++-
 src/core/test.js                              | 22 +---------------
 test/cli/cli-main.js                          |  7 ++----
 .../async-module-error-promise.tap.txt        |  2 +-
 .../async-module-error-thenable.tap.txt       |  2 +-
 test/cli/fixtures/async-module-error.tap.txt  | 13 +---------
 test/cli/fixtures/no-tests.tap.txt            |  7 ++----
 .../only-test-only-module-mix.tap.txt         |  7 ++----
 test/cli/fixtures/syntax-error.tap.txt        | 13 +---------
 test/cli/fixtures/unhandled-rejection.tap.txt |  3 +--
 test/main/TapReporter.js                      |  7 ++++++
 15 files changed, 36 insertions(+), 92 deletions(-)

diff --git a/demos/grunt-contrib-qunit.js b/demos/grunt-contrib-qunit.js
index 3cc0176c2..f92cf691e 100644
--- a/demos/grunt-contrib-qunit.js
+++ b/demos/grunt-contrib-qunit.js
@@ -21,11 +21,7 @@ QUnit.test.each('failing tests', {
 >> Actual: false
 >> Expected: true
 >>   at `],
-  'no-tests': ['fail-no-tests', `Testing fail-no-tests.html F
->> global failure
->> Message: No tests were run.
->> Actual: undefined
->> Expected: undefined
+  'no-tests': ['fail-no-tests', `Testing fail-no-tests.html 
 >> Error: No tests were run.
 >>     at `],
   uncaught: ['fail-uncaught', `Testing fail-uncaught.html \n\
diff --git a/demos/grunt-contrib-qunit/package.json b/demos/grunt-contrib-qunit/package.json
index dba8cc5ce..1f241245c 100644
--- a/demos/grunt-contrib-qunit/package.json
+++ b/demos/grunt-contrib-qunit/package.json
@@ -2,7 +2,7 @@
   "private": true,
   "devDependencies": {
     "grunt": "1.6.1",
-    "grunt-contrib-qunit": "10.0.0"
+    "grunt-contrib-qunit": "10.1.1"
   },
   "scripts": {
     "test": "grunt"
diff --git a/src/core/core.js b/src/core/core.js
index e7a6a9ee2..17ea04be8 100644
--- a/src/core/core.js
+++ b/src/core/core.js
@@ -26,7 +26,7 @@ import { createStartFunction } from './start.js';
 // rather than partly in config.js and partly here.
 config.currentModule.suiteReport = runSuite;
 
-config._pq = new ProcessingQueue(test);
+config._pq = new ProcessingQueue();
 
 const QUnit = {
 
diff --git a/src/core/processing-queue.js b/src/core/processing-queue.js
index 29fb7ddb5..fb7eceae3 100644
--- a/src/core/processing-queue.js
+++ b/src/core/processing-queue.js
@@ -1,8 +1,9 @@
 import config from './config.js';
-import { extend, generateHash, performance } from './utilities.js';
+import { generateHash, performance } from './utilities.js';
 import { runLoggingCallbacks } from './callbacks.js';
 import Promise from './promise.js';
 import { runSuite } from './module.js';
+import onUncaughtException from './on-uncaught-exception.js';
 import { emit } from './events.js';
 import { setTimeout } from './globals.js';
 
@@ -28,11 +29,7 @@ function unitSamplerGenerator (seed) {
 }
 
 class ProcessingQueue {
-  /**
-   * @param {Function} test Reference to the QUnit.test() method
-   */
-  constructor (test) {
-    this.test = test;
+  constructor () {
     this.priorityCount = 0;
     this.unitSampler = null;
 
@@ -168,7 +165,7 @@ class ProcessingQueue {
   done () {
     // We have reached the end of the processing queue and are about to emit the
     // "runEnd" event after which reporters typically stop listening and exit
-    // the process. First, check if we need to emit one final test.
+    // the process. First, check if we need to emit an error event.
     if (config.stats.testCount === 0 && config.failOnZeroTests === true) {
       let error;
       if (config.filter && config.filter.length) {
@@ -183,19 +180,7 @@ class ProcessingQueue {
         error = new Error('No tests were run.');
       }
 
-      this.test('global failure', extend(function (assert) {
-        assert.pushResult({
-          result: false,
-          message: error.message,
-          source: error.stack
-        });
-      }, { validTest: true }));
-
-      // We do need to call `advance()` in order to resume the processing queue.
-      // Once this new test is finished processing, we'll reach `done` again, and
-      // that time the above condition will evaluate to false.
-      this.advance();
-      return;
+      onUncaughtException(error);
     }
 
     const storage = config.storage;
diff --git a/src/core/reporters/TapReporter.js b/src/core/reporters/TapReporter.js
index 4960760b7..500015d5f 100644
--- a/src/core/reporters/TapReporter.js
+++ b/src/core/reporters/TapReporter.js
@@ -179,6 +179,7 @@ export default class TapReporter {
     this.log = options.log || console.log.bind(console);
 
     this.testCount = 0;
+    this.started = false;
     this.ended = false;
     this.bailed = false;
 
@@ -193,7 +194,10 @@ export default class TapReporter {
   }
 
   onRunStart (_runSuite) {
-    this.log('TAP version 13');
+    if (!this.started) {
+      this.log('TAP version 13');
+      this.started = true;
+    }
   }
 
   onError (error) {
@@ -206,6 +210,10 @@ export default class TapReporter {
     // Imitate onTestEnd
     // Skip this if we're past "runEnd" as it would look odd
     if (!this.ended) {
+      if (!this.started) {
+        this.log('TAP version 13');
+        this.started = true;
+      }
       this.testCount = this.testCount + 1;
       this.log(kleur.red(`not ok ${this.testCount} global failure`));
       this.logError(error);
diff --git a/src/core/test.js b/src/core/test.js
index f65aa866a..05cb638b0 100644
--- a/src/core/test.js
+++ b/src/core/test.js
@@ -87,12 +87,6 @@ export default function Test (settings) {
 
   ++Test.count;
   this.errorForStack = new Error();
-  if (this.callback && this.callback.validTest) {
-    // Omit the test-level trace for the internal "No tests" test failure,
-    // There is already an assertion-level trace, and that's noisy enough
-    // as it is.
-    this.errorForStack.stack = undefined;
-  }
   this.testReport = new TestReport(this.testName, this.module.suiteReport, {
     todo: this.todo,
     skip: this.skip,
@@ -782,11 +776,6 @@ Test.prototype = {
   },
 
   valid: function () {
-    // Internally-generated tests are always valid
-    if (this.callback && this.callback.validTest) {
-      return true;
-    }
-
     function moduleChainIdMatch (testModule, selectedId) {
       return (
         // undefined or empty array
@@ -909,16 +898,7 @@ function checkPollution () {
 let focused = false; // indicates that the "only" filter was used
 
 function addTest (settings) {
-  if (
-    // Never ignore the internal "No tests" error, as this would infinite loop.
-    // See /test/cli/fixtures/only-test-only-module-mix.tap.txt
-    //
-    // TODO: If we improve HtmlReporter to buffer and replay early errors,
-    // we can change "No tests" to be a global error, and thus get rid of
-    // the "validTest" concept and the ProcessQueue re-entry hack.
-    !(settings.callback && settings.callback.validTest) &&
-    (focused || config.currentModule.ignored)
-  ) {
+  if (focused || config.currentModule.ignored) {
     return;
   }
 
diff --git a/test/cli/cli-main.js b/test/cli/cli-main.js
index 39e058f34..f0c2ad59c 100644
--- a/test/cli/cli-main.js
+++ b/test/cli/cli-main.js
@@ -202,15 +202,12 @@ ReferenceError: varIsNotDefined is not defined
     assert.equal(execution.snapshot, `TAP version 13
 not ok 1 global failure
   ---
-  message: "No tests matched the filter \\"no matches\\"."
+  message: "Error: No tests matched the filter \\"no matches\\"."
   severity: failed
-  actual  : undefined
-  expected: undefined
   stack: |
     Error: No tests matched the filter "no matches".
-        at qunit.js
-        at internal
   ...
+Bail out! Error: No tests matched the filter "no matches".
 1..1
 # pass 0
 # skip 0
diff --git a/test/cli/fixtures/async-module-error-promise.tap.txt b/test/cli/fixtures/async-module-error-promise.tap.txt
index 1d602ac36..9f3f90570 100644
--- a/test/cli/fixtures/async-module-error-promise.tap.txt
+++ b/test/cli/fixtures/async-module-error-promise.tap.txt
@@ -1,6 +1,7 @@
 # name: module() with promise return value
 # command: ["qunit","async-module-error-promise.js"]
 
+TAP version 13
 not ok 1 global failure
   ---
   message: |+
@@ -13,7 +14,6 @@ not ok 1 global failure
         at internal
   ...
 Bail out! Error: Failed to load file async-module-error-promise.js
-TAP version 13
 ok 2 module manually returning a promise > has a test
 1..2
 # pass 1
diff --git a/test/cli/fixtures/async-module-error-thenable.tap.txt b/test/cli/fixtures/async-module-error-thenable.tap.txt
index 76df6e5cf..a16840f20 100644
--- a/test/cli/fixtures/async-module-error-thenable.tap.txt
+++ b/test/cli/fixtures/async-module-error-thenable.tap.txt
@@ -1,6 +1,7 @@
 # name: module() with promise return value
 # command: ["qunit","async-module-error-thenable.js"]
 
+TAP version 13
 not ok 1 global failure
   ---
   message: |+
@@ -13,7 +14,6 @@ not ok 1 global failure
         at internal
   ...
 Bail out! Error: Failed to load file async-module-error-thenable.js
-TAP version 13
 ok 2 module manually returning a thenable > has a test
 1..2
 # pass 1
diff --git a/test/cli/fixtures/async-module-error.tap.txt b/test/cli/fixtures/async-module-error.tap.txt
index ee5855bcf..3d33143f8 100644
--- a/test/cli/fixtures/async-module-error.tap.txt
+++ b/test/cli/fixtures/async-module-error.tap.txt
@@ -1,6 +1,7 @@
 # name: module() with async function
 # command: ["qunit","async-module-error.js"]
 
+TAP version 13
 not ok 1 global failure
   ---
   message: |+
@@ -13,18 +14,6 @@ not ok 1 global failure
         at internal
   ...
 Bail out! Error: Failed to load file async-module-error.js
-TAP version 13
-not ok 2 global failure
-  ---
-  message: No tests were run.
-  severity: failed
-  actual  : undefined
-  expected: undefined
-  stack: |
-    Error: No tests were run.
-        at qunit.js
-        at internal
-  ...
 1..2
 # pass 0
 # skip 0
diff --git a/test/cli/fixtures/no-tests.tap.txt b/test/cli/fixtures/no-tests.tap.txt
index 5dc50674f..d2d4eacbb 100644
--- a/test/cli/fixtures/no-tests.tap.txt
+++ b/test/cli/fixtures/no-tests.tap.txt
@@ -4,15 +4,12 @@
 TAP version 13
 not ok 1 global failure
   ---
-  message: No tests were run.
+  message: Error: No tests were run.
   severity: failed
-  actual  : undefined
-  expected: undefined
   stack: |
     Error: No tests were run.
-        at qunit.js
-        at internal
   ...
+Bail out! Error: No tests were run.
 1..1
 # pass 0
 # skip 0
diff --git a/test/cli/fixtures/only-test-only-module-mix.tap.txt b/test/cli/fixtures/only-test-only-module-mix.tap.txt
index 0342b745d..2af0bc893 100644
--- a/test/cli/fixtures/only-test-only-module-mix.tap.txt
+++ b/test/cli/fixtures/only-test-only-module-mix.tap.txt
@@ -3,15 +3,12 @@
 TAP version 13
 not ok 1 global failure
   ---
-  message: No tests were run.
+  message: Error: No tests were run.
   severity: failed
-  actual  : undefined
-  expected: undefined
   stack: |
     Error: No tests were run.
-        at qunit.js
-        at internal
   ...
+Bail out! Error: No tests were run.
 1..3
 # pass 2
 # skip 0
diff --git a/test/cli/fixtures/syntax-error.tap.txt b/test/cli/fixtures/syntax-error.tap.txt
index fa9f317cb..7f6d2c777 100644
--- a/test/cli/fixtures/syntax-error.tap.txt
+++ b/test/cli/fixtures/syntax-error.tap.txt
@@ -1,6 +1,7 @@
 # name: load file with syntax error
 # command: ["qunit", "syntax-error.js"]
 
+TAP version 13
 not ok 1 global failure
   ---
   message: |+
@@ -13,18 +14,6 @@ not ok 1 global failure
         at internal
   ...
 Bail out! Error: Failed to load file syntax-error.js
-TAP version 13
-not ok 2 global failure
-  ---
-  message: No tests were run.
-  severity: failed
-  actual  : undefined
-  expected: undefined
-  stack: |
-    Error: No tests were run.
-        at qunit.js
-        at internal
-  ...
 1..2
 # pass 0
 # skip 0
diff --git a/test/cli/fixtures/unhandled-rejection.tap.txt b/test/cli/fixtures/unhandled-rejection.tap.txt
index e9afdcfa5..4d331724b 100644
--- a/test/cli/fixtures/unhandled-rejection.tap.txt
+++ b/test/cli/fixtures/unhandled-rejection.tap.txt
@@ -1,6 +1,7 @@
 # name: unhandled rejection
 # command: ["qunit","unhandled-rejection.js"]
 
+TAP version 13
 not ok 1 global failure
   ---
   message: Error: outside of a test context
@@ -13,7 +14,6 @@ not ok 1 global failure
         at internal
   ...
 Bail out! Error: outside of a test context
-TAP version 13
 not ok 2 Unhandled Rejections > test passes just fine, but has a rejected promise
   ---
   message: global failure: Error: Error thrown in non-returned promise!
@@ -23,7 +23,6 @@ not ok 2 Unhandled Rejections > test passes just fine, but has a rejected promis
   stack: |
     Error: Error thrown in non-returned promise!
         at /qunit/test/cli/fixtures/unhandled-rejection.js:10:13
-        at internal
   ...
 1..2
 # pass 0
diff --git a/test/main/TapReporter.js b/test/main/TapReporter.js
index 9ec70355c..03b4609de 100644
--- a/test/main/TapReporter.js
+++ b/test/main/TapReporter.js
@@ -50,6 +50,9 @@ QUnit.module('TapReporter', function (hooks) {
       },
       emit: function (event, data) {
         this.on[event](data);
+      },
+      clear: function () {
+        buffer = '';
       }
     };
     last = undefined;
@@ -161,6 +164,8 @@ QUnit.module('TapReporter', function (hooks) {
   });
 
   QUnit.test('output global failure (string)', function (assert) {
+    emitter.emit('runStart');
+    emitter.clear();
     emitter.emit('error', 'Boo');
 
     assert.strictEqual(buffer, kleur.red('not ok 1 global failure') + '\n' +
@@ -175,6 +180,8 @@ QUnit.module('TapReporter', function (hooks) {
   QUnit.test('output global failure (Error)', function (assert) {
     var err = new ReferenceError('Boo is not defined');
     mockStack(err);
+    emitter.emit('runStart');
+    emitter.clear();
     emitter.emit('error', err);
 
     assert.strictEqual(buffer, kleur.red('not ok 1 global failure') + '\n' +