Core: Fix late onerror handling

Gruntfile.js

@@ -75,9 +75,6 @@ module.exports = function( grunt ) {
 						"test/reorder.html",
 						"test/reorderError1.html",
 						"test/reorderError2.html",
-						"test/callbacks.html",
-						"test/callbacks-promises.html",
-						"test/events.html",
 						"test/events-filters.html",
 						"test/events-in-test.html",
 						"test/logs.html",
@@ -128,7 +125,6 @@ module.exports = function( grunt ) {
 				"test/main/deepEqual.js",
 				"test/main/stack.js",
 				"test/main/utilities.js",
-				"test/events.js",
 				"test/events-in-test.js",
 				"test/onerror/inside-test.js",
 				"test/onerror/outside-test.js",

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,