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.

  This also removes the need for the 99aee51 workaround, which
  avoided a crash by infinite loop.

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

* Remove "omit stack trace" logic in test.js,
  existed only for this purpose. To keep output for the "No tests"
  error similarly clean and distraction-free, the TAP reporter
  treats error stack traces with a similar cleaner since
  #1789.

* 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 3f08a1a, 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 (fortunatley) nobody has started relying on this. I found only
  copies of QUnit itself.

As a nice side-effect, fixtures like async-module-error.tap.txt
now no longer display a useless "No tests" after an uncaught error
that already bailed the test run. This happened previously because
errors are not tests, and so technically there was no test. No that
"No tests" is itself considered a bail out, TAP absorbs this after
the first error, just like it alreayd does for other cascading
errors.

Closes #1790.

Author: Krinkle

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\

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"

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 = {
 

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;

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;
 
@@ -192,8 +193,11 @@ export default class TapReporter {
     return new TapReporter(runner, options);
   }
 
-  onRunStart (_runSuite) {
-    this.log('TAP version 13');
+  onRunStart () {
+    if (!this.started) {
+      this.log('TAP version 13');
+      this.started = true;
+    }
   }
 
   onError (error) {
@@ -206,6 +210,7 @@ export default class TapReporter {
     // Imitate onTestEnd
     // Skip this if we're past "runEnd" as it would look odd
     if (!this.ended) {
+      this.onRunStart();
       this.testCount = this.testCount + 1;
       this.log(kleur.red(`not ok ${this.testCount} global failure`));
       this.logError(error);

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

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

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

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

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

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

test/cli/fixtures/only-test-only-module-mix.js

@@ -24,15 +24,29 @@
 //   - ignored
 // * ProcessingQueue.end:
 //   - create new Error('No tests were run.')
-//   - emit it via a dynamically created test
-//   - despite forcing validTest=true, the added test (as of QUnit 2.21.0) is ignored
-//     because it is a non-only test and we're in "focus" mode.
-//     This causes an infinite loop between ProcessingQueue.end and ProcessingQueue.advance
-//     because it thinks it ads a test, but doesn't.
-//     This scenerio is surprising because the usual way to active "focused" test mode,
-//     is by defining a test.only() case, in which case the queue by definition isn't
-//     empty. Even if the test.only() case was skipped by a filter (or config.moduleId/testId)
-//     this is taken care of by forcing validTest=true.
+//
+//   - Past problem 1: The error is hidden.
+//
+//     Up until QUnit 2.21.0, this error was emitted via a
+//     dynamically created QUnit.test(), with `validTest=true` set to bypass
+//     filters like config.filter, config.module, and config.testId.
+//     However, because it is a regular test (not an "only" test) it is
+//     ignored because this test file defines one or more "QUnit.test.only" cases,
+//     which puts us in "focus" mode and thus never even reaches Test.valid().
+//
+//   - Past problem 2: Crash by infinite loop.
+//
+//     The first problem causes an infinite loop between ProcessingQueue.end
+//     and ProcessingQueue.advance, because end() thinks it adds a fake test, but
+//     the definition is ignored due to  "focused" test by a previous test.only().
+//     Normally, that would by definition mean the queue can't be empty and so
+//     the "No tests" code would never be reached. Even if the test.only() case
+//     was skipped by a filter (or config.moduleId/testId) this was accounted
+//     for by setting validTest=true.
+//     Fixed in QUnit 3 (99aee51a8a) by exempting validTest from "focused" mode.
+//
+//   - QUnit 3 emits "No tests" as "error" instead of a fake test, which obsoletes
+//     the above workarounds.
 
 QUnit.module('module A', function () {
   // ... start module scope A

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

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

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

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' +