Assert: Improve detection of bad calls to assert.async() callbacks

src/assert.js

@@ -72,47 +72,10 @@ class Assert {
 		}
 	}
 
-	// Put a hold on processing and return a function that will release it a maximum of once.
+	// Create a new async pause and return a new function that can release the pause.
 	async( count ) {
-		const test = this.test;
-
-		let popped = false,
-			acceptCallCount = count;
-
-		if ( typeof acceptCallCount === "undefined" ) {
-			acceptCallCount = 1;
-		}
-
-		const resume = internalStop( test );
-
-		return function done() {
-
-			if ( config.current === undefined ) {
-				throw new Error( "`assert.async` callback from test \"" +
-					test.testName + "\" called after tests finished." );
-			}
-
-			if ( config.current !== test ) {
-				config.current.pushFailure(
-					"`assert.async` callback from test \"" +
-					test.testName + "\" was called during this test." );
-				return;
-			}
-
-			if ( popped ) {
-				test.pushFailure( "Too many calls to the `assert.async` callback",
-					sourceFromStacktrace( 2 ) );
-				return;
-			}
-
-			acceptCallCount -= 1;
-			if ( acceptCallCount > 0 ) {
-				return;
-			}
-
-			popped = true;
-			resume();
-		};
+		const requiredCalls = count === undefined ? 1 : count;
+		return internalStop( this.test, requiredCalls );
 	}
 
 	// Exports test.push() to the user API

src/cli/run.js

@@ -109,7 +109,7 @@ async function run( args, options ) {
 			console.error( "Error: Process exited before tests finished running" );
 
 			const currentTest = QUnit.config.current;
-			if ( currentTest && currentTest.semaphore ) {
+			if ( currentTest && currentTest.pauses.size > 0 ) {
 				const name = currentTest.testName;
 				console.error( "Last test to run (" + name + ") has an async hold. " +
 					"Ensure all assert.async() callbacks are invoked and Promises resolve. " +

src/html-reporter/es6-map.js

@@ -1,14 +1,35 @@
-// Support IE 9-10, PhantomJS: Fallback for fuzzysort.js used by ./html.js
+// Support IE 9-10, Safari 7, PhantomJS: Partial Map fallback.
+// Used by html.js (via fuzzysort.js), and test.js.
+//
+// FIXME: This check is broken. This file is embedded in the qunit.js closure,
+// thus the Map var is hoisted in that scope, and starts undefined (not a function).
 var Map = typeof Map === "function" ? Map : function StringMap() {
 	var store = Object.create( null );
+	var hasOwn = Object.prototype.hasOwnProperty;
 	this.get = function( strKey ) {
 		return store[ strKey ];
 	};
 	this.set = function( strKey, val ) {
+		if ( !hasOwn.call( store, strKey ) ) {
+			this.size++;
+		}
 		store[ strKey ] = val;
 		return this;
 	};
+	this.delete = function( strKey ) {
+		if ( hasOwn.call( store, strKey ) ) {
+			delete store[ strKey ];
+			this.size--;
+		}
+	};
+	this.forEach = function( callback ) {
+		for ( var strKey in store ) {
+			callback( store[ strKey ], strKey );
+		}
+	};
 	this.clear = function() {
 		store = Object.create( null );
+		this.size = 0;
 	};
+	this.size = 0;
 };

src/test.js

@@ -25,12 +25,13 @@ import TestReport from "./reports/test";
 export default function Test( settings ) {
 	this.expected = null;
 	this.assertions = [];
-	this.semaphore = 0;
 	this.module = config.currentModule;
 	this.steps = [];
 	this.timeout = undefined;
 	this.data = undefined;
 	this.withData = false;
+	this.pauses = new Map();
+	this.nextPauseId = 1;
 	extend( this, settings );
 
 	// If a module is skipped, all its tests and the tests of the child suites
@@ -201,9 +202,9 @@ Test.prototype = {
 			}
 			test.resolvePromise( promise );
 
-			// If the test has a "lock" on it, but the timeout is 0, then we push a
+			// If the test has an async "pause" on it, but the timeout is 0, then we push a
 			// failure as the test should be synchronous.
-			if ( test.timeout === 0 && test.semaphore !== 0 ) {
+			if ( test.timeout === 0 && test.pauses.size > 0 ) {
 				pushFailure(
 					"Test did not finish synchronously even though assert.timeout( 0 ) was used.",
 					sourceFromStacktrace( 2 )
@@ -810,16 +811,100 @@ export function resetTestTimeout( timeoutDuration ) {
 	config.timeout = setTimeout( config.timeoutHandler( timeoutDuration ), timeoutDuration );
 }
 
-// Put a hold on processing and return a function that will release it.
-export function internalStop( test ) {
-	let released = false;
-	test.semaphore += 1;
+// Create a new async pause and return a new function that can release the pause.
+//
+// This mechanism is internally used by:
+//
+// * explicit async pauses, created by calling `assert.async()`,
+// * implicit async pauses, created when `QUnit.test()` or module hook callbacks
+//   use async-await or otherwise return a Promise.
+//
+// Happy scenario:
+//
+// * Pause is created by calling internalStop().
+//
+//   Pause is released normally by invoking release() during the same test.
+//
+//   The release() callback lets internal processing resume.
+//
+// Failure scenarios:
+//
+// * The test fails due to an uncaught exception.
+//
+//   In this case, Test.run() will call internalRecover() which empties the clears all
+//   async pauses and sets the cancelled flag, which means we silently ignore any
+//   late calls to the resume() callback, as we will have moved on to a different
+//   test by then, and we don't want to cause an extra "release during a different test"
+//   errors that the developer isn't really responsible for. This can happen when a test
+//   correctly schedules a call to release(), but also causes an uncaught error. The
+//   uncaught error means we will no longer wait for the release (as it might not arrive).
+//
+// * Pause is never released, or called an insufficient number of times.
+//
+//   Our timeout handler will kill the pause and resume test processing, basically
+//   like internalRecover(), but for one pause instead of any/all.
+//
+//   Here, too, any late calls to resume() will be silently ignored to avoid
+//   extra errors. We tolerate this since the original test will have already been
+//   marked as failure.
+//
+//   TODO: QUnit 3 will enable timeouts by default <https://github.com/qunitjs/qunit/issues/1483>,
+//   but right now a test will hang indefinitely if async pauses are not released,
+//   unless QUnit.config.testTimeout or assert.timeout() is used.
+//
+// * Pause is spontaneously released during a different test,
+//   or when no test is currently running.
+//
+//   This is close to impossible because this error only happens if the original test
+//   succesfully finished first (since other failure scenarios kill pauses and ignore
+//   late calls). It can happen if a test ended exactly as expected, but has some
+//   external or shared state continuing to hold a reference to the release callback,
+//   and either the same test scheduled another call to it in the future, or a later test
+//   causes it to be called through some shared state.
+//
+// * Pause release() is called too often, during the same test.
+//
+//   This simply throws an error, after which uncaught error handling picks it up
+//   and processing resumes.
+export function internalStop( test, requiredCalls = 1 ) {
 	config.blocking = true;
 
+	const pauseId = test.nextPauseId++;
+	const pause = {
+		cancelled: false,
+		remaining: requiredCalls
+	};
+	test.pauses.set( pauseId, pause );
+
+	function release() {
+		if ( pause.cancelled ) {
+			return;
+		}
+		if ( config.current === undefined ) {
+			throw new Error( "Unexpected release of async pause after tests finished.\n" +
+				`> Test: ${test.testName} [async #${pauseId}]` );
+		}
+		if ( config.current !== test ) {
+			throw new Error( "Unexpected release of async pause during a different test.\n" +
+				`> Test: ${test.testName} [async #${pauseId}]` );
+		}
+		if ( pause.remaining <= 0 ) {
+			throw new Error( "Tried to release async pause that was already released.\n" +
+				`> Test: ${test.testName} [async #${pauseId}]` );
+		}
+
+		// The `requiredCalls` parameter exists to support `assert.async(count)`
+		pause.remaining--;
+		if ( pause.remaining === 0 ) {
+			test.pauses.delete( pauseId );
+		}
+
+		internalStart( test );
+	}
+
 	// Set a recovery timeout, if so configured.
 	if ( setTimeout ) {
 		let timeoutDuration;
-
 		if ( typeof test.timeout === "number" ) {
 			timeoutDuration = test.timeout;
 		} else if ( typeof config.testTimeout === "number" ) {
@@ -830,12 +915,14 @@ export function internalStop( test ) {
 			config.timeoutHandler = function( timeout ) {
 				return function() {
 					config.timeout = null;
-					pushFailure(
+					pause.cancelled = true;
+					test.pauses.delete( pauseId );
+
+					test.pushFailure(
 						`Test took longer than ${timeout}ms; test timed out.`,
 						sourceFromStacktrace( 2 )
 					);
-					released = true;
-					internalRecover( test );
+					internalStart( test );
 				};
 			};
 			clearTimeout( config.timeout );
@@ -846,56 +933,31 @@ export function internalStop( test ) {
 		}
 	}
 
-	return function resume() {
-		if ( released ) {
-			return;
-		}
-
-		released = true;
-		test.semaphore -= 1;
-		internalStart( test );
-	};
+	return release;
 }
 
 // Forcefully release all processing holds.
 function internalRecover( test ) {
-	test.semaphore = 0;
+	test.pauses.forEach( pause => {
+		pause.cancelled = true;
+	} );
+	test.pauses.clear();
 	internalStart( test );
 }
 
 // Release a processing hold, scheduling a resumption attempt if no holds remain.
 function internalStart( test ) {
 
-	// If semaphore is non-numeric, throw error
-	if ( isNaN( test.semaphore ) ) {
-		test.semaphore = 0;
-
-		pushFailure(
-			"Invalid value on test.semaphore",
-			sourceFromStacktrace( 2 )
-		);
-	}
-
-	// Don't start until equal number of stop-calls
-	if ( test.semaphore > 0 ) {
+	// Ignore if other async pauses still exist.
+	if ( test.pauses.size > 0 ) {
 		return;
 	}
 
-	// Throw an Error if start is called more often than stop
-	if ( test.semaphore < 0 ) {
-		test.semaphore = 0;
-
-		pushFailure(
-			"Tried to restart test while already started (test's semaphore was 0 already)",
-			sourceFromStacktrace( 2 )
-		);
-	}
-
 	// Add a slight delay to allow more assertions etc.
 	if ( setTimeout ) {
 		clearTimeout( config.timeout );
 		config.timeout = setTimeout( function() {
-			if ( test.semaphore > 0 ) {
+			if ( test.pauses.size > 0 ) {
 				return;
 			}
 

test/cli/fixtures/drooling-done.js

@@ -10,5 +10,8 @@ QUnit.test( "Test A", assert => {
 
 QUnit.test( "Test B", assert => {
 	assert.ok( true );
+
+	// This bad call is silently ignored because "Test A" already failed
+	// and we cancelled the async pauses.
 	done();
 } );

test/cli/fixtures/drooling-extra-done-outside.js

@@ -0,0 +1,11 @@
+QUnit.test( "extra done scheduled outside any test", assert => {
+	assert.timeout( 10 );
+	const done = assert.async();
+	assert.true( true );
+
+	// Later, boom!
+	setTimeout( done, 100 );
+
+	// Passing, end of test
+	done();
+} );

test/cli/fixtures/drooling-extra-done.js

@@ -0,0 +1,18 @@
+QUnit.config.reorder = false;
+
+let done;
+
+QUnit.test( "Test A", assert => {
+	assert.ok( true );
+	done = assert.async();
+
+	// Passing.
+	done();
+} );
+
+QUnit.test( "Test B", assert => {
+	assert.ok( true );
+
+	// Boom
+	done();
+} );