Skip to content

Commit

Permalink
Core: Restore fake test for "No tests were run" message
Browse files Browse the repository at this point in the history
Historically, the "No tests were run" message was thrown as a real
uncaught error by us, recieved by the native error handler in the
browser or Node.js, and we then listened to that to generate a failing
test case to show in the UI for extra visibility. The use of fake tests
in this way wasn't great and caused problems when they happened after
a "skip" module, or if there error came from QUnit.begin, runStart,
testStart, or beforeEach hooks, since that would prevent the test from
coming through and left internals in a broken state.

We fixed that in QUnit 2.17 by rendering uncaught errors directly to
the UI instead of queueing them up as a fake test that may or may not
make it to the UI.

To preserve API compatibility, we still count this as a failed test
and still make it flip "status" to "failed" in the `QUnit.done()` and
`runEnd` event. Thus, just as before, all uncaught errors are reported
to the browser/Node handler, result in a failing test run, and shown
in the UI.

== Problem

1. I wrongly changed the "No tests" message after 07de3c6 and
   8f5e7ed to be passed directly to our `onUncaughtError`
   function, thus not it is not seen by the browser/Node as a real
   uncaught error. A good reporter will still fail their build based
   on our failing test count or "status" field from done/runEnd, but
   they could no longer find out why (e.g. the "No tests" message).

2. There are reporters, including grunt-contirb-qunit and testem,
   which use neither the`runEnd` event nor the `QUnit.done()` callback
   to find out the test counts, or the overall "status". Instead, they
   rely solely on "testDone" to discover all failures.

In the case of testem, it does listen to QUnit.done but only to get the
test duration and stop the browser, it uses its own test counts to
determine the run status. [a]

In the case of grunt-contrib-qunit, it did utilize the QUnit.done data
to find the number of failed assertions, but had its own logic for
counting the number of tests, and it used the test count to decide the
outcome of the test run. [b] Also, the Puppeteer pageerror event in
grunt-contrib-qunit is not printed and not used to fail the task.

== Change

Per problem 1, let's re-instate the old behaviour. Before QUnit 2.17
the "No tests" message was both an uncaught error *and* a fake test.
With the new safeguards in place we have to choose which one we want.

I went for a fake test because that seems most rebust for reporters,
since evidently some reporters do not report not uncaught errors.

I've also removed the `Logger.warn()` fallback in the HTML Reporter.
I added this in 8f5e7ed, not realizing that uncaught errors are
already rendered in the browser console by default, so this caused it
to show up twice.

There was however once case of an error being passed to
`onUncaughtException` directly without actually being recieved by
window.onerror, and that was the "No tests" message which would
otherwise not be visible anywhere if the UI was disabled. However this
is no longer an issue since this message is now reported as a fake test.

[a] https://github.com/gruntjs/grunt-contrib-qunit/blob/v5.1.0/tasks/qunit.js#L197-L236
[b] https://github.com/testem/testem/blob/v3.5.0/public/testem/qunit_adapter.js

Ref #1651.
  • Loading branch information
Krinkle committed Sep 13, 2021
1 parent 1c89326 commit f30d5de
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 16 deletions.
34 changes: 25 additions & 9 deletions src/core/processing-queue.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import config from "./config";
import onUncaughtException from "./on-uncaught-exception";
import {
extend,
generateHash,
now
} from "./utilities";
Expand All @@ -9,6 +9,9 @@ import {
} from "./logging";

import Promise from "../promise";
import {
test
} from "../test";
import {
globalSuite
} from "../core";
Expand Down Expand Up @@ -158,16 +161,10 @@ function unitSamplerGenerator( seed ) {
* items. It handles emitting the final run events.
*/
function done() {
const storage = config.storage;

const runtime = now() - config.started;
const passed = config.stats.all - config.stats.bad;

ProcessingQueue.finished = true;

// 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 error.
// the process. First, check if we need to emit one final test.
if ( config.stats.testCount === 0 && config.failOnZeroTests === true ) {
let error;
if ( config.filter && config.filter.length ) {
Expand All @@ -182,9 +179,28 @@ function done() {
error = new Error( "No tests were run." );
}

onUncaughtException( error );
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.
advance();
return;
}

const storage = config.storage;

const runtime = now() - config.started;
const passed = config.stats.all - config.stats.bad;

ProcessingQueue.finished = true;

emit( "runEnd", globalSuite.end( true ) );
runLoggingCallbacks( "done", {
passed,
Expand Down
3 changes: 0 additions & 3 deletions src/html-reporter/html.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import QUnit from "../core";
import Logger from "../logger";
import { extend, errorString } from "../core/utilities";
import { window, document, navigator, console } from "../globals";
import "./urlparams";
Expand Down Expand Up @@ -1035,8 +1034,6 @@ export function escapeText( s ) {
if ( !testItem ) {

// HTML Reporter is probably disabled or not yet initialized.
Logger.warn( "global failure" );
Logger.warn( error );
return;
}

Expand Down
7 changes: 7 additions & 0 deletions src/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,13 @@ 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,
Expand Down
10 changes: 6 additions & 4 deletions test/cli/fixtures/expected/tap-outputs.js
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,10 @@ not ok 2 Unhandled Rejections > test passes just fine, but has a rejected promis
`TAP version 13
not ok 1 global failure
---
message: Error: No tests were run.
message: No tests were run.
severity: failed
actual : undefined
expected: undefined
stack: |
Error: No tests were run.
at done (/qunit/qunit/qunit.js)
Expand All @@ -153,7 +155,6 @@ not ok 1 global failure
at unblockAndAdvanceQueue (/qunit/qunit/qunit.js)
at internal
...
Bail out! Error: No tests were run.
1..1
# pass 0
# skip 0
Expand Down Expand Up @@ -236,8 +237,10 @@ ok 1 Zero assertions > has a test
`TAP version 13
not ok 1 global failure
---
message: "Error: No tests matched the filter \\"no matches\\"."
message: "No tests matched the filter \\"no matches\\"."
severity: failed
actual : undefined
expected: undefined
stack: |
Error: No tests matched the filter "no matches".
at done (/qunit/qunit/qunit.js)
Expand All @@ -246,7 +249,6 @@ not ok 1 global failure
at unblockAndAdvanceQueue (/qunit/qunit/qunit.js)
at internal
...
Bail out! Error: No tests matched the filter "no matches".
1..1
# pass 0
# skip 0
Expand Down

0 comments on commit f30d5de

Please sign in to comment.