Core: Fix late onerror handling

== Background ==

Previously, QUnit.onError and QUnit.onUnhandledRejection could report
global errors by synthesizing a new test, even after a run has ended.

This is problematic when an errors ocurrs after all modules (and their
hooks) have finished, and the overall test run has ended.

The most immediate problem is that hooks having finished already,
means it is illegal for a new test to start since "after" has already
run. To protect against such illegal calls, the hooks object is
emptied internally, and this new test causes an internal error:

```
TypeError: Cannot read property 'length' of undefined
```

This is not underlying problem though, but rather our internal
safeguard working as intended. The higher-level problem is that there
is no appropiate way to report a late error as a test since the run
has already ended. The `QUnit.done()` callbacks have run, and
the `runEnd` event has been emitted.

== Approach ==

Instead of trying to report (late) errors as a test, only print them
to `console.warn()`, which goes to stderr in Node.js. For the CLI, also
remember that uncaught errors were found and use that to make sure we
don't change exitCode back to zero (e.g. in case we have an uncaught
error after the last test but before our `runEnd` callback is called).

== Changes ==

* Generalise `QUnit.onUnhandledRejection` and re-use it for
  `window.onerror` (browser), and uncaught exceptions (CLI).

* Fix broken use of `QUnit.onError` in `process.on( "uncaughtException" )`.
  This was passing the wrong parameters. Use the new onUncaughtException
  method instead.

* Clarify that `QUnit.onError` is only for `window.onerror`. For now,
  keep its strange non-standard signature as-is (with the custom object
  parameter), but document this and its return value.

* Remove the unused "..args" from `QUnit.onError`. This was only ever
  passed from one of our unit tests to give one extra argument (a
  string of "actual"), which then ended up passed as "actual" parameter
  to `pushFailure()`. We never used this in the actual onError binding,
  so remove this odd variadic construct for now.

* Change `ProcessingQueue#done`, which is in charge of reporting
  the "No tests were run" error, to no longer rely on the way that
  `QUnit.onError` previously queued a late test.

  The first part of this function may run twice (same as before, once
  after an empty test run, and one more time after the synthetic
  test has finished and the queue is empty again). Change this so that
  we no longer assign `finished = true` in that first part. This means
  we will still support queueing of this one late test. But, since the
  quueue is empty, we do need to call `advance()` manually as otherwise
  it'd never get processed.

  Previously, `finished = true` was assigned first, which meant that
  `QUnit.onError` was adding a test under that condition. But this
  worked anyway because `Test#queue` internally had manual advancing
  exactly for this use case, which is also where we now emit a
  deprecation warning (to become an error in QUnit 3). Note that using
  this for anything other than the "No tests run" error was already
  unreliable since generally runEnd would have been emitted already.
  The "No tests run" test was exactly done from the one sweet spot
  where it was (and remains) safe because that threw an error and thus
  prevented runEnd from being emitted.

Fixes #1377.
Ref #1322.
Ref #1446.

docs/config/QUnit.onUncaughtException.md

@@ -0,0 +1,33 @@
+---
+layout: default
+title: QUnit.onUncaughtException()
+excerpt: Handle a global error.
+categories:
+ - config
+version_added: "unversioned"
+---
+
+`QUnit.onUncaughtException( error )`
+
+Handle a global error that should result in a failed test run.
+
+| name | description |
+|------|-------------|
+| `error` (any) | Usually an `Error` object, but any other thrown or rejected value may be given as well. |
+
+### Examples
+
+```js
+const error = new Error( "Failed to reverse the polarity of the neutron flow" );
+QUnit.onUncaughtException( error );
+```
+
+```js
+process.on( "uncaughtException", QUnit.onUncaughtException );
+```
+
+```js
+window.addEventListener( "unhandledrejection", function( event ) {
+  QUnit.onUncaughtException( event.reason );
+} );
+```

src/assert.js

@@ -4,7 +4,7 @@ import { internalStop, resetTestTimeout } from "./test";
 import Logger from "./logger";
 
 import config from "./core/config";
-import { objectType, objectValues } from "./core/utilities";
+import { objectType, objectValues, errorString } from "./core/utilities";
 import { sourceFromStacktrace } from "./core/stacktrace";
 import { clearTimeout } from "./globals";
 
@@ -491,25 +491,4 @@ class Assert {
 // eslint-disable-next-line dot-notation
 Assert.prototype.raises = Assert.prototype[ "throws" ];
 
-/**
- * Converts an error into a simple string for comparisons.
- *
- * @param {Error|Object} error
- * @return {string}
- */
-function errorString( error ) {
-	const resultErrorString = error.toString();
-
-	// If the error wasn't a subclass of Error but something like
-	// an object literal with name and message properties...
-	if ( resultErrorString.slice( 0, 7 ) === "[object" ) {
-
-		// Based on https://es5.github.com/#x15.11.4.4
-		const name = error.name ? String( error.name ) : "Error";
-		return error.message ? `${name}: ${error.message}` : name;
-	} else {
-		return resultErrorString;
-	}
-}
-
 export default Assert;

src/cli/run.js

@@ -89,28 +89,34 @@ async function run( args, options ) {
 				}
 			}
 		} catch ( e ) {
-
-			// eslint-disable-next-line no-loop-func
-			QUnit.module( files[ i ], function() {
-				const loadFailureMessage = `Failed to load the test file with error:\n${e.stack}`;
-				QUnit.test( loadFailureMessage, function( assert ) {
-					assert.true( false, "should be able to load file" );
-				} );
-			} );
+			const error = new Error( `Failed to load file ${files[ i ]}\n${e.name}: ${e.message}` );
+			error.stack = e.stack;
+			QUnit.onUncaughtException( error );
 		}
 	}
 
-	let running = true;
-
-	// Listen for unhandled rejections, and call QUnit.onUnhandledRejection
-	process.on( "unhandledRejection", function( reason ) {
-		QUnit.onUnhandledRejection( reason );
+	// The below handlers set exitCode directly, to make sure it is set even if the
+	// uncaught exception happens after the last test (shortly before "runEnd", or
+	// asynchronously after "runEnd" if the process is still running).
+	//
+	// The variable makes sure that if the uncaught exception is before "runEnd",
+	// or from another "runEnd" callback, it also won't turn the error code
+	// back into a success.
+	let uncaught = false;
+
+	// Handle the unhandled
+	process.on( "unhandledRejection", ( reason, _promise ) => {
+		QUnit.onUncaughtException( reason );
+		process.exitCode = 1;
+		uncaught = true;
 	} );
-
-	process.on( "uncaughtException", function( error ) {
-		QUnit.onError( error );
+	process.on( "uncaughtException", ( error, _origin ) => {
+		QUnit.onUncaughtException( error );
+		process.exitCode = 1;
+		uncaught = true;
 	} );
 
+	let running = true;
 	process.on( "exit", function() {
 		if ( running ) {
 			console.error( "Error: Process exited before tests finished running" );
@@ -130,7 +136,7 @@ async function run( args, options ) {
 	QUnit.on( "runEnd", function setExitCode( data ) {
 		running = false;
 
-		if ( data.testCounts.failed ) {
+		if ( data.testCounts.failed || uncaught ) {
 			process.exitCode = 1;
 		} else {
 			process.exitCode = 0;

src/core.js

@@ -18,8 +18,8 @@ import ProcessingQueue from "./core/processing-queue";
 import SuiteReport from "./reports/suite";
 
 import { on, emit } from "./events";
-import onError from "./core/onerror";
-import onUnhandledRejection from "./core/on-unhandled-rejection";
+import onWindowError from "./core/onerror";
+import onUncaughtException from "./core/on-uncaught-exception";
 
 const QUnit = {};
 export const globalSuite = new SuiteReport();
@@ -47,8 +47,8 @@ extend( QUnit, {
 	is,
 	objectType,
 	on,
-	onError,
-	onUnhandledRejection,
+	onError: onWindowError,
+	onUncaughtException,
 	pushFailure,
 
 	assert: Assert.prototype,
@@ -99,6 +99,12 @@ extend( QUnit, {
 
 	},
 
+	onUnhandledRejection: function( reason ) {
+		Logger.warn( "QUnit.onUnhandledRejection is deprecated and will be removed in QUnit 3.0." +
+			" Please use QUnit.onUncaughtException instead." );
+		onUncaughtException( reason );
+	},
+
 	extend: function( ...args ) {
 		Logger.warn( "QUnit.extend is deprecated and will be removed in QUnit 3.0." +
 			" Please use Object.assign instead." );

src/core/on-uncaught-exception.js

@@ -0,0 +1,52 @@
+import config from "./config";
+import ProcessingQueue from "./processing-queue";
+import { sourceFromStacktrace } from "./stacktrace";
+import { extend, errorString } from "./utilities";
+import Logger from "../logger";
+import { test } from "../test";
+
+/**
+ * Handle a global error that should result in a failed test run.
+ *
+ * Summary:
+ *
+ * - If there is a current test, it becomes a failed assertion.
+ * - If there is a current module, it becomes a failed test (and bypassing filters).
+ *   Note that if we're before any other test or module, it will naturally
+ *   become a global test.
+ * - If the overall test run has ended, the error is printed to `console.warn()`.
+ *
+ * @since 2.17.0
+ * @param {Error|any} error
+ */
+export default function onUncaughtException( error ) {
+	const message = errorString( error );
+
+	// We could let callers specify an extra offset to add to the number passed to
+	// sourceFromStacktrace, in case they are a wrapper further away from the error
+	// handler, and thus reduce some noise in the stack trace. However, we're not
+	// doing this right now because it would almost never be used in practice given
+	// the vast majority of error values will be an Error object, and thus have a
+	// clean stack trace already.
+	const source = error.stack || sourceFromStacktrace( 2 );
+
+	if ( config.current ) {
+		config.current.assert.pushResult( {
+			result: false,
+			message: `global failure: ${message}`,
+			source
+		} );
+	} else if ( !ProcessingQueue.finished ) {
+		test( "global failure", extend( function( assert ) {
+			assert.pushResult( { result: false, message, source } );
+		}, { validTest: true } ) );
+	} else {
+
+		// TODO: Once supported in js-reporters and QUnit, use a TAP "bail" event.
+		// The CLI runner can use this to ensure a non-zero exit code, even if
+		// emitted after "runEnd" and before the process exits.
+		// The HTML Reporter can use this to renmder it on the page in a test-like
+		// block for easy discovery.
+		Logger.warn( `${message}\n${source}` );
+	}
+}

src/core/onerror.js

@@ -1,31 +1,33 @@
-import { pushFailure, test } from "../test";
-
 import config from "./config";
-import { extend } from "./utilities";
+import onUncaughtException from "./on-uncaught-exception";
 
-// Handle an unhandled exception. By convention, returns true if further
-// error handling should be suppressed and false otherwise.
-// In this case, we will only suppress further error handling if the
-// "ignoreGlobalErrors" configuration option is enabled.
-export default function onError( error, ...args ) {
-	if ( config.current ) {
-		if ( config.current.ignoreGlobalErrors ) {
-			return true;
-		}
-		pushFailure(
-			error.message,
-			error.stacktrace || error.fileName + ":" + error.lineNumber,
-			...args
-		);
-	} else {
-		test( "global failure", extend( function() {
-			pushFailure(
-				error.message,
-				error.stacktrace || error.fileName + ":" + error.lineNumber,
-				...args
-			);
-		}, { validTest: true } ) );
+/**
+ * Handle a window.onerror error.
+ *
+ * If there is a current test that sets the internal `ignoreGlobalErrors` field
+ * (such as during `assert.throws()`), then the error is ignored and native
+ * error reporting is suppressed as well. This is because in browsers, an error
+ * can sometimes end up in `window.onerror` instead of in the local try/catch.
+ * This ignoring of errors does not apply to our general onUncaughtException
+ * method, nor to our `unhandledRejection` handlers, as those are not meant
+ * to receive an "expected" error during `assert.throws()`.
+ *
+ * @see <https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onerror>
+ * @param {Object} details
+ * @param {string} details.message
+ * @param {string} details.fileName
+ * @param {number} details.lineNumber
+ * @param {string|undefined} [details.stacktrace]
+ * @return {bool} True if native error reporting should be suppressed.
+ */
+export default function onWindowError( details ) {
+	if ( config.current && config.current.ignoreGlobalErrors ) {
+		return true;
 	}
 
+	const err = new Error( details.message );
+	err.stack = details.stacktrace || details.fileName + ":" + details.lineNumber;
+	onUncaughtException( err );
+
 	return false;
 }

src/core/processing-queue.js

@@ -1,4 +1,5 @@
 import config from "./config";
+import onUncaughtException from "./on-uncaught-exception";
 import {
 	generateHash,
 	now
@@ -159,33 +160,36 @@ function unitSamplerGenerator( seed ) {
 function done() {
 	const storage = config.storage;
 
-	ProcessingQueue.finished = true;
-
 	const runtime = now() - config.started;
 	const passed = config.stats.all - config.stats.bad;
 
+	// We have reached the end of the processing queue, but there is one more
+	// thing we'd like to report as a possible failing test. For this to work
+	// we need to call `onUncaughtException` before closing `ProcessingQueue.finished`.
+	// However, 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 below condition should evaluate to false.
 	if ( config.stats.testCount === 0 && config.failOnZeroTests === true ) {
-
+		let error;
 		if ( config.filter && config.filter.length ) {
-			throw new Error( `No tests matched the filter "${config.filter}".` );
-		}
-
-		if ( config.module && config.module.length ) {
-			throw new Error( `No tests matched the module "${config.module}".` );
-		}
-
-		if ( config.moduleId && config.moduleId.length ) {
-			throw new Error( `No tests matched the moduleId "${config.moduleId}".` );
-		}
-
-		if ( config.testId && config.testId.length ) {
-			throw new Error( `No tests matched the testId "${config.testId}".` );
+			error = new Error( `No tests matched the filter "${config.filter}".` );
+		} else if ( config.module && config.module.length ) {
+			error = new Error( `No tests matched the module "${config.module}".` );
+		} else if ( config.moduleId && config.moduleId.length ) {
+			error = new Error( `No tests matched the moduleId "${config.moduleId}".` );
+		} else if ( config.testId && config.testId.length ) {
+			error = new Error( `No tests matched the testId "${config.testId}".` );
+		} else {
+			error = new Error( "No tests were run." );
 		}
 
-		throw new Error( "No tests were run." );
-
+		onUncaughtException( error );
+		advance();
+		return;
 	}
 
+	ProcessingQueue.finished = true;
+
 	emit( "runEnd", globalSuite.end( true ) );
 	runLoggingCallbacks( "done", {
 		passed,

src/core/utilities.js

@@ -157,3 +157,25 @@ export function generateHash( module, testName ) {
 
 	return hex.slice( -8 );
 }
+
+/**
+ * Converts an error into a simple string for comparisons.
+ *
+ * @param {Error|any} error
+ * @return {string}
+ */
+export function errorString( error ) {
+
+	// Use String() instead of toString() to handle non-object values like undefined or null.
+	const resultErrorString = String( error );
+
+	// If the error wasn't a subclass of Error but something like
+	// an object literal with name and message properties...
+	if ( resultErrorString.slice( 0, 7 ) === "[object" ) {
+
+		// Based on https://es5.github.io/#x15.11.4.4
+		return ( error.name || "Error" ) + ( error.message ? `: ${error.message}` : "" );
+	} else {
+		return resultErrorString;
+	}
+}

src/html-reporter/html.js

@@ -1088,8 +1088,7 @@ export function escapeText( s ) {
 		return ret;
 	};
 
-	// Listen for unhandled rejections, and call QUnit.onUnhandledRejection
 	window.addEventListener( "unhandledrejection", function( event ) {
-		QUnit.onUnhandledRejection( event.reason );
+		QUnit.onUncaughtException( event.reason );
 	} );
 }() );