Core: New error event for bailing on uncaught errors

Details at #1638.

Fixes #1446.
Fixes #1633.

Gruntfile.js

@@ -90,7 +90,6 @@ module.exports = function( grunt ) {
 						"test/reporter-urlparams-hidepassed.html",
 						"test/moduleId.html",
 						"test/onerror/inside-test.html",
-						"test/onerror/outside-test.html",
 						"test/seed.html",
 						"test/overload.html",
 						"test/preconfigured.html",
@@ -125,9 +124,9 @@ module.exports = function( grunt ) {
 				"test/main/deepEqual.js",
 				"test/main/stack.js",
 				"test/main/utilities.js",
+				"test/main/onUncaughtException.js",
 				"test/events-in-test.js",
 				"test/onerror/inside-test.js",
-				"test/onerror/outside-test.js",
 				"test/setTimeout.js",
 				"test/node/storage-1.js",
 				"test/node/storage-2.js",

src/cli/run.js

@@ -95,25 +95,12 @@ async function run( args, options ) {
 		}
 	}
 
-	// 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", ( error, _origin ) => {
 		QUnit.onUncaughtException( error );
-		process.exitCode = 1;
-		uncaught = true;
 	} );
 
 	let running = true;
@@ -131,17 +118,24 @@ async function run( args, options ) {
 		}
 	} );
 
-	QUnit.start();
+	QUnit.on( "error", function( _error ) {
+
+		// Set exitCode directly, to make sure it is set to fail even if "runEnd" will never be
+		// reached, or if "runEnd" was already fired in the past and the process crashed later.
+		process.exitCode = 1;
+	} );
 
 	QUnit.on( "runEnd", function setExitCode( data ) {
 		running = false;
 
-		if ( data.testCounts.failed || uncaught ) {
+		if ( data.testCounts.failed ) {
 			process.exitCode = 1;
 		} else {
 			process.exitCode = 0;
 		}
 	} );
+
+	QUnit.start();
 }
 
 run.restart = function( args ) {

src/core.js

@@ -118,7 +118,6 @@ extend( QUnit, {
 
 		// Initialize the configuration options
 		extend( config, {
-			stats: { all: 0, bad: 0, testCount: 0 },
 			started: 0,
 			updateRate: 1000,
 			autostart: true,

src/core/config.js

@@ -11,6 +11,8 @@ const config = {
 	// The queue of tests to run
 	queue: [],
 
+	stats: { all: 0, bad: 0, testCount: 0 },
+
 	// Block until document ready
 	blocking: true,
 

src/core/on-uncaught-exception.js

@@ -1,55 +1,51 @@
 import config from "./config";
-import ProcessingQueue from "./processing-queue";
+import { globalSuite } from "../core";
 import { sourceFromStacktrace } from "./stacktrace";
-import { extend, errorString } from "./utilities";
-import Logger from "../logger";
-import { test } from "../test";
+import { errorString } from "./utilities";
+import { emit } from "../events";
 
 /**
  * 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()`.
+ * - If we're strictly inside a test (or one if its module hooks), the exception
+ *   becomes a failed assertion.
+ *
+ *   This has the important side-effect that uncaught exceptions (such as
+ *   calling an undefined function) during a "todo" test do NOT result in
+ *   a failed test run.
+ *
+ * - If we're anywhere outside a test (be it in early event callbacks, or
+ *   internally between tests, or somewhere after "runEnd" if the process is
+ *   still alive for some reason), then send an "error" event to the reporters.
  *
  * @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
+			message: `global failure: ${errorString( error )}`,
+
+			// We could let callers specify an offset to subtract a number of frames via
+			// 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 Error objects, and thus have their
+			// own stack trace already.
+			source: ( error && error.stack ) || sourceFromStacktrace( 2 )
 		} );
-	} 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.
-		//
-		// Avoid printing "Error: foo" twice if the environment's native stack trace
-		// already includes that in its format.
-		Logger.warn( source.indexOf( source ) !== -1 ? source : `${message}\n${source}` );
+		// The "error" event was added in QUnit 2.17.
+		// Increase "bad assertion" stats despite no longer pushing an assertion in this case.
+		// This ensures "runEnd" and "QUnit.done()" handlers behave as expected, since the "bad"
+		// count is typically how reporters decide on the boolean outcome of the test run.
+		globalSuite.globalFailureCount++;
+		config.stats.bad++;
+		config.stats.all++;
+		emit( "error", error );
 	}
 }

src/core/onerror.js

@@ -1,3 +1,4 @@
+import Logger from "../logger";
 import config from "./config";
 import onUncaughtException from "./on-uncaught-exception";
 
@@ -13,6 +14,7 @@ import onUncaughtException from "./on-uncaught-exception";
  * to receive an "expected" error during `assert.throws()`.
  *
  * @see <https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onerror>
+ * @deprecated since 2.17.0 Use QUnit.onUncaughtException instead.
  * @param {Object} details
  * @param {string} details.message
  * @param {string} details.fileName
@@ -21,6 +23,9 @@ import onUncaughtException from "./on-uncaught-exception";
  * @return {bool} True if native error reporting should be suppressed.
  */
 export default function onWindowError( details ) {
+	Logger.warn( "QUnit.onError is deprecated and will be removed in QUnit 3.0." +
+			" Please use QUnit.onUncaughtException instead." );
+
 	if ( config.current && config.current.ignoreGlobalErrors ) {
 		return true;
 	}

src/core/processing-queue.js

@@ -163,12 +163,11 @@ function done() {
 	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.
+	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.
 	if ( config.stats.testCount === 0 && config.failOnZeroTests === true ) {
 		let error;
 		if ( config.filter && config.filter.length ) {
@@ -184,12 +183,8 @@ function done() {
 		}
 
 		onUncaughtException( error );
-		advance();
-		return;
 	}
 
-	ProcessingQueue.finished = true;
-
 	emit( "runEnd", globalSuite.end( true ) );
 	runLoggingCallbacks( "done", {
 		passed,

src/events.js

@@ -2,6 +2,7 @@ import { objectType, inArray } from "./core/utilities";
 
 const LISTENERS = Object.create( null );
 const SUPPORTED_EVENTS = [
+	"error",
 	"runStart",
 	"suiteStart",
 	"testStart",

src/html-reporter/html.js

@@ -1,11 +1,12 @@
 import QUnit from "../core";
+import Logger from "../logger";
 import Test from "../test";
-import { extractStacktrace } from "../core/stacktrace";
-import { now, extend } from "../core/utilities";
+import { now, extend, errorString } from "../core/utilities";
 import { window, document, navigator, console } from "../globals";
 import "./urlparams";
 import fuzzysort from "fuzzysort";
 
+// TODO: Remove adhoc counting in favour of stats from QUnit.done() or "runEnd" event.
 const stats = {
 	passedTests: 0,
 	failedTests: 0,
@@ -654,21 +655,27 @@ export function escapeText( s ) {
 		title = document.createElement( "strong" );
 		title.innerHTML = getNameHtml( name, moduleName );
 
-		rerunTrigger = document.createElement( "a" );
-		rerunTrigger.innerHTML = "Rerun";
-		rerunTrigger.href = setUrl( { testId: testId } );
-
 		testBlock = document.createElement( "li" );
 		testBlock.appendChild( title );
-		testBlock.appendChild( rerunTrigger );
-		testBlock.id = "qunit-test-output-" + testId;
+
+		// No ID or rerun link for "global failure" blocks
+		if ( testId !== undefined ) {
+			rerunTrigger = document.createElement( "a" );
+			rerunTrigger.innerHTML = "Rerun";
+			rerunTrigger.href = setUrl( { testId: testId } );
+
+			testBlock.id = "qunit-test-output-" + testId;
+			testBlock.appendChild( rerunTrigger );
+		}
 
 		assertList = document.createElement( "ol" );
 		assertList.className = "qunit-assert-list";
 
 		testBlock.appendChild( assertList );
 
 		tests.appendChild( testBlock );
+
+		return testBlock;
 	}
 
 	// HTML Reporter initialization and load
@@ -1027,6 +1034,34 @@ export function escapeText( s ) {
 		}
 	} );
 
+	QUnit.on( "error", ( error ) => {
+		stats.failedTests++;
+
+		const testItem = appendTest( "global failure" );
+		if ( !testItem ) {
+
+			// HTML Reporter is probably disabled or not yet initialized.
+			Logger.warn( "global failure" );
+			Logger.warn( error );
+			return;
+		}
+
+		// Render similar to a failed assertion (see above QUnit.log callback)
+		let message = escapeText( errorString( error ) );
+		message = "<span class='test-message'>" + message + "</span>";
+		if ( error && error.stack ) {
+			message += "<table>" +
+				"<tr class='test-source'><th>Source: </th><td><pre>" +
+				escapeText( error.stack ) + "</pre></td></tr>" +
+				"</table>";
+		}
+		const assertList = testItem.getElementsByTagName( "ol" )[ 0 ];
+		const assertLi = document.createElement( "li" );
+		assertLi.className = "fail";
+		assertLi.innerHTML = message;
+		assertList.appendChild( assertLi );
+	} );
+
 	// Avoid readyState issue with phantomjs
 	// Ref: #818
 	var usingPhantom = ( function( p ) {
@@ -1068,21 +1103,27 @@ export function escapeText( s ) {
 		// Treat return value as window.onerror itself does,
 		// Only do our handling if not suppressed.
 		if ( ret !== true ) {
-			const error = {
-				message,
-				fileName,
-				lineNumber
-			};
+
+			// 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()`.
+			if ( config.current && config.current.ignoreGlobalErrors ) {
+				return true;
+			}
 
 			// According to
 			// https://blog.sentry.io/2016/01/04/client-javascript-reporting-window-onerror,
 			// most modern browsers support an errorObj argument; use that to
 			// get a full stack trace if it's available.
-			if ( errorObj && errorObj.stack ) {
-				error.stacktrace = extractStacktrace( errorObj, 0 );
+			const error = errorObj || new Error( message );
+			if ( !error.stack && fileName && lineNumber ) {
+				error.stack = `${fileName}:${lineNumber}`;
 			}
-
-			ret = QUnit.onError( error );
+			QUnit.onUncaughtException( error );
 		}
 
 		return ret;

src/reporters/ConsoleReporter.js

@@ -9,6 +9,7 @@ export default class ConsoleReporter {
 		// Support IE 9: Function#bind is supported, but no console.log.bind().
 		this.log = options.log || Function.prototype.bind.call( console.log, console );
 
+		runner.on( "error", this.onError.bind( this ) );
 		runner.on( "runStart", this.onRunStart.bind( this ) );
 		runner.on( "testStart", this.onTestStart.bind( this ) );
 		runner.on( "testEnd", this.onTestEnd.bind( this ) );
@@ -19,6 +20,10 @@ export default class ConsoleReporter {
 		return new ConsoleReporter( runner, options );
 	}
 
+	onError( error ) {
+		this.log( "error", error );
+	}
+
 	onRunStart( runStart ) {
 		this.log( "runStart", runStart );
 	}