Core: ensure process-queue advance with rejected promises

Author: step2yeung

Gruntfile.js

@@ -151,6 +151,7 @@ module.exports = function( grunt ) {
 						"test/reorderError2.html",
 						"test/callbacks.html",
 						"test/callbacks-promises.html",
+						"test/callbacks-rejected-promises.html",
 						"test/events.html",
 						"test/events-in-test.html",
 						"test/logs.html",

reporter/html.js

@@ -858,16 +858,15 @@ export function escapeText( s ) {
 	} );
 
 	QUnit.testDone( function( details ) {
-		var testTitle, time, testItem, assertList, status,
+		var testTitle, time, assertList, status,
 			good, bad, testCounts, skipped, sourceName,
-			tests = id( "qunit-tests" );
+			tests = id( "qunit-tests" ),
+			testItem = id( "qunit-test-output-" + details.testId );
 
-		if ( !tests ) {
+		if ( !tests || !testItem ) {
 			return;
 		}
 
-		testItem = id( "qunit-test-output-" + details.testId );
-
 		removeClass( testItem, "running" );
 
 		if ( details.failed > 0 ) {

src/core.js

@@ -176,7 +176,10 @@ export function begin() {
 		runLoggingCallbacks( "begin", {
 			totalTests: Test.count,
 			modules: modulesLog
-		} ).then( unblockAndAdvanceQueue );
+		} ).then( unblockAndAdvanceQueue, function( err ) {
+			setTimeout( unblockAndAdvanceQueue );
+			throw err;
+		} );
 	} else {
 		unblockAndAdvanceQueue();
 	}

src/core/logging.js

@@ -1,27 +1,42 @@
 import config from "./config";
 import { objectType } from "./utilities";
 import Promise from "../promise";
+import {
+	setTimeout
+} from "../globals";
+
+function _promisfyCallbacksSequentially( callbacks, args ) {
+	return callbacks.reduce( ( promiseChain, callback ) => {
+		const executeCallback = () => callback( args );
+		return promiseChain.then(
+			executeCallback,
+			( err ) => {
+				setTimeout( executeCallback );
+				throw err;
+			}
+		);
+	}, Promise.resolve( [] ) );
+}
+
+function _registerLoggingCallback( key ) {
+	const loggingCallback = ( callback ) => {
+		if ( objectType( callback ) !== "function" ) {
+			throw new Error(
+				"QUnit logging methods require a callback function as their first parameters."
+			);
+		}
+
+		config.callbacks[ key ].push( callback );
+	};
+
+	return loggingCallback;
+}
 
-// Register logging callbacks
 export function registerLoggingCallbacks( obj ) {
 	var i, l, key,
 		callbackNames = [ "begin", "done", "log", "testStart", "testDone",
 			"moduleStart", "moduleDone" ];
 
-	function registerLoggingCallback( key ) {
-		var loggingCallback = function( callback ) {
-			if ( objectType( callback ) !== "function" ) {
-				throw new Error(
-					"QUnit logging methods require a callback function as their first parameters."
-				);
-			}
-
-			config.callbacks[ key ].push( callback );
-		};
-
-		return loggingCallback;
-	}
-
 	for ( i = 0, l = callbackNames.length; i < l; i++ ) {
 		key = callbackNames[ i ];
 
@@ -30,7 +45,7 @@ export function registerLoggingCallbacks( obj ) {
 			config.callbacks[ key ] = [];
 		}
 
-		obj[ key ] = registerLoggingCallback( key );
+		obj[ key ] = _registerLoggingCallback( key );
 	}
 }
 
@@ -46,10 +61,5 @@ export function runLoggingCallbacks( key, args ) {
 		return;
 	}
 
-	// ensure that each callback is executed serially
-	return callbacks.reduce( ( promiseChain, callback ) => {
-		return promiseChain.then( () => {
-			return Promise.resolve( callback( args ) );
-		} );
-	}, Promise.resolve( [] ) );
+	return _promisfyCallbacksSequentially( callbacks, args );
 }

src/core/processing-queue.js

@@ -61,13 +61,23 @@ function processTaskQueue( start ) {
 
 		if ( !defined.setTimeout || config.updateRate <= 0 || elapsedTime < config.updateRate ) {
 			const task = taskQueue.shift();
-			Promise.resolve( task() ).then( function() {
+			const processNextTaskOrAdvance = () => {
 				if ( !taskQueue.length ) {
 					advance();
 				} else {
 					processTaskQueue( start );
 				}
-			} );
+			};
+			const throwAndAdvance = ( err ) => {
+				setTimeout( advance );
+				throw err;
+			};
+
+			// Without throwAndAdvance, qunit does not continue advanceing the processing queue if
+			// task() throws an error
+			Promise.resolve( task() )
+				.then( processNextTaskOrAdvance, throwAndAdvance )
+				.catch( throwAndAdvance );
 		} else {
 			setTimeout( advance );
 		}
@@ -153,13 +163,25 @@ function unitSamplerGenerator( seed ) {
 	};
 }
 
+// Clear own storage items when tests completes
+function cleanStorage() {
+	const storage = config.storage;
+	if ( storage && config.stats.bad === 0 ) {
+		for ( let i = storage.length - 1; i >= 0; i-- ) {
+			const key = storage.key( i );
+
+			if ( key.indexOf( "qunit-test-" ) === 0 ) {
+				storage.removeItem( key );
+			}
+		}
+	}
+}
+
 /**
  * This function is called when the ProcessingQueue is done processing all
  * items. It handles emitting the final run events.
  */
 function done() {
-	const storage = config.storage;
-
 	ProcessingQueue.finished = true;
 
 	const runtime = now() - config.started;
@@ -193,18 +215,9 @@ function done() {
 		failed: config.stats.bad,
 		total: config.stats.all,
 		runtime
-	} ).then( () => {
-
-		// Clear own storage items if all tests passed
-		if ( storage && config.stats.bad === 0 ) {
-			for ( let i = storage.length - 1; i >= 0; i-- ) {
-				const key = storage.key( i );
-
-				if ( key.indexOf( "qunit-test-" ) === 0 ) {
-					storage.removeItem( key );
-				}
-			}
-		}
+	} ).then( cleanStorage, function( err ) {
+		cleanStorage();
+		throw err;
 	} );
 }
 

src/test.js

@@ -118,19 +118,25 @@ Test.prototype = {
 
 		// ensure the callbacks are executed serially for each module
 		var callbackPromises = notStartedModules.reduce( ( promiseChain, startModule ) => {
-			return promiseChain.then( () => {
+			const moduleStartCallback = () => {
 				startModule.stats = { all: 0, bad: 0, started: now() };
 				emit( "suiteStart", startModule.suiteReport.start( true ) );
 				return runLoggingCallbacks( "moduleStart", {
 					name: startModule.name,
 					tests: startModule.tests
 				} );
-			} );
+			};
+
+			return promiseChain.then( moduleStartCallback, moduleStartCallback );
 		}, Promise.resolve( [] ) );
 
-		return callbackPromises.then( () => {
+		const testStartHandler = () => {
 			config.current = this;
-
+			const testStartResolvedHandler = () => {
+				if ( !config.pollution ) {
+					saveGlobal();
+				}
+			};
 			this.testEnvironment = extend( {}, module.testEnvironment );
 
 			this.started = now();
@@ -140,10 +146,16 @@ Test.prototype = {
 				module: module.name,
 				testId: this.testId,
 				previousFailure: this.previousFailure
-			} ).then( () => {
-				if ( !config.pollution ) {
-					saveGlobal();
-				}
+			} ).then( testStartResolvedHandler, function( err ) {
+				setTimeout( testStartResolvedHandler );
+				throw err;
+			} );
+		};
+
+		return callbackPromises.then( testStartHandler, ( err ) => {
+			return Promise.reject( err ).catch( ( err ) => {
+				setTimeout( testStartHandler );
+				throw err;
 			} );
 		} );
 	},
@@ -319,23 +331,7 @@ Test.prototype = {
 		emit( "testEnd", this.testReport.end( true ) );
 		this.testReport.slimAssertions();
 
-		return runLoggingCallbacks( "testDone", {
-			name: testName,
-			module: moduleName,
-			skipped: skipped,
-			todo: todo,
-			failed: bad,
-			passed: this.assertions.length - bad,
-			total: this.assertions.length,
-			runtime: skipped ? 0 : this.runtime,
-
-			// HTML Reporter use
-			assertions: this.assertions,
-			testId: this.testId,
-
-			// Source of Test
-			source: this.stack
-		} ).then( function() {
+		const testDoneResolvedHandler = function() {
 			if ( module.testsRun === numberOfTests( module ) ) {
 				const completedModules = [ module ];
 
@@ -353,8 +349,35 @@ Test.prototype = {
 					} );
 				}, Promise.resolve( [] ) );
 			}
-		} ).then( function() {
+		};
+
+		return runLoggingCallbacks( "testDone", {
+			name: testName,
+			module: moduleName,
+			skipped: skipped,
+			todo: todo,
+			failed: bad,
+			passed: this.assertions.length - bad,
+			total: this.assertions.length,
+			runtime: skipped ? 0 : this.runtime,
+
+			// HTML Reporter use
+			assertions: this.assertions,
+			testId: this.testId,
+
+			// Source of Test
+			source: this.stack
+		} ).then(
+			testDoneResolvedHandler,
+			function( err ) {
+				setTimeout( testDoneResolvedHandler );
+				throw err;
+			}
+		).then( function() {
+			config.current = undefined;
+		}, function( err ) {
 			config.current = undefined;
+			throw err;
 		} );
 
 		function logSuiteEnd( module ) {
@@ -660,7 +683,6 @@ function checkPollution() {
 	if ( newGlobals.length > 0 ) {
 		pushFailure( "Introduced global variable(s): " + newGlobals.join( ", " ) );
 	}
-
 	deletedGlobals = diff( old, config.pollution );
 	if ( deletedGlobals.length > 0 ) {
 		pushFailure( "Deleted global variable(s): " + deletedGlobals.join( ", " ) );

test/callbacks-rejected-promises.html

@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html>
+<head>
+	<meta charset="UTF-8">
+	<title>QUnit Callbacks Test Suite</title>
+	<link rel="stylesheet" href="../dist/qunit.css">
+	<script src="../dist/qunit.js"></script>
+	<script src="callbacks-rejected-promises.js"></script>
+</head>
+<body>
+	<div id="qunit"></div>
+</body>
+</html>