Core: Fix infinite loop crash when mixing test.only() with module.only()

I noticed that the first branch of `addOnlyTest()` was uncovered in
test coverage reports. In expanding the `only-module-then-test.js`
fixture to cover this (by adding a late test.only call), which
was originally added in #1658.

I decided to also add an early test.only call to see what would happen.
The outcome is irrelevant/ambigious, but it seemed worth observing.
To my surprise, it created an infinite loop.

This further convinces me that we should make "early errors" show
up in the HTML Reporter (TODO added in 37f6e4b), and subsequently
get rid of this re-entry hack. The TapReporter and QUnit CLI are
already able to render early errors. The HtmlReporter not yet. It's
not generally a priority imho, but, because "No tests" is a common
scenario, if we utilize an early error to report it, that makes it
a priority to support in HtmlReporter as otherwise there'd be no
visible feedback for it. It's okay to have to pop open devtools when
causing an early syntax error or something like that in your own code,
but when using the UI to match no tests, there should at least be
some kind of visible feedback and not a blank page.

Author: Krinkle

src/module.js

@@ -144,6 +144,13 @@ module.only = function (...args) {
   if (!focused) {
     // Upon the first module.only() call,
     // delete any and all previously registered modules and tests.
+    //
+    // TODO: This does not clear SuiteReport, which means the empty modules are
+    // left behind and wrongly reported as "skipped" (skipped == total), and
+    // deleted tests wrongly count toward runEnd.testCounts as passing test
+    // (this.getFailedAssertions().length === 0).
+    // This is why /test/cli/fixtures/only-test-only-module-mix.tap.txt reports
+    // 1..3 instead of 1..1.
     config.modules.length = 0;
     config.queue.length = 0;
 

src/reports/suite.js

@@ -29,6 +29,10 @@ export default class SuiteReport {
       tests: this.tests.map(test => test.start()),
       childSuites: this.childSuites.map(suite => suite.start()),
       testCounts: {
+        // TODO: Due to code reuse, this ends up computing getStatus()
+        // for tests that obviously haven't run yet. It's harmless but
+        // quite inefficient recursion, that we repeat many times over
+        // also via test.start() and suite.start() above.
         total: this.getTestCounts().total
       }
     };

src/test.js

@@ -911,7 +911,16 @@ function checkPollution () {
 let focused = false; // indicates that the "only" filter was used
 
 function addTest (settings) {
-  if (focused || config.currentModule.ignored) {
+  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)
+  ) {
     return;
   }
 

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

@@ -10,7 +10,11 @@ QUnit.module('module A', function () {
     });
   });
 
-  QUnit.test('test A2', function (assert) {
+  QUnit.test.only('test A2', function (assert) {
+    assert.true(false, 'not run');
+  });
+
+  QUnit.test('test A3', function (assert) {
     assert.true(false, 'not run');
   });
 });

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

@@ -0,0 +1,58 @@
+// This currently runs 0 tests because stuff cancels out.
+// The intent is ambigious and outcome doesn't really matter.
+// It's covered as regression test against a past crash:
+//
+// * module A:
+//   - start module scope A
+// * test A1:
+//   - queue A1
+// * test.only A2:
+//   - start "test" focus mode.
+//   - clear queue (delete A1).
+//   - queue A2
+// * module.only B:
+//   - start "module" focus mode.
+//   - clear queue (delete A2).
+//   - modify parent A to become "ignored".
+//   - start child-module scope B
+// * test B1:
+//   - ignored, because it's a non-only test and
+//     we are in "focus" mode due to the earlier test.only.
+//     thus the only test we would run is a "test.only" test,
+//     inside a "module.only" block, which this file doesn't have any of.
+// * test A4:
+//   - 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.
+
+QUnit.module('module A', function () {
+  // ... start module scope A
+
+  // ... queue A1
+  QUnit.test('test A1', function (assert) {
+    assert.true(false, 'not run');
+  });
+
+  QUnit.test.only('test A2', function (assert) {
+    assert.true(false, 'not run');
+  });
+
+  QUnit.module.only('module B', function () {
+    QUnit.test('test B1', function (assert) {
+      assert.true(true, 'run');
+    });
+  });
+
+  QUnit.test('test A4', function (assert) {
+    assert.true(false, 'not run');
+  });
+});

test/cli/fixtures/only-test-only-module-mix.tap.txt

@@ -0,0 +1,21 @@
+# command: ["qunit", "only-test-only-module-mix.js"]
+
+TAP version 13
+not ok 1 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..3
+# pass 2
+# skip 0
+# todo 0
+# fail 1
+
+# exit code: 1