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

== Background ==

When creating two async pauses in a test, it was possible for a test
to pass by invoking one of them twice, and the other not at all.

Easy scenario (though perhaps not realistic):

> Use `assert.async()` twice, assigned as done1 and done2 in the same
> `QUnit.test()` case, and then simulate the failure scenario such that
> you wrongly call done1 two times, and forget to call done2.

Complex scenario across `QUnit.test()` and "afterEach" hooks, since
these previously shared a single semaphore:

> Use `assert.async()` once in a simple test, and schedule the resume
> call in the future, but then fail with an uncaught error. The uncaught
> error is found and `Test.run()` would internally kill the pause by
> resetting the semaphore to zero (this make sense since we shouldn't
> wait for the release once the test is known to have failed).
> After this reset, we proceed to the "afterEach" hook. Suppose this
> hook is also async, and during its execution, the originally scheduled
> resume call happens. This would effectively end up releasing the
> afterEach's async pause despite not being finished yet, and then we
> proceed to the next test. That test would then fail when the afterEach's
> own release call happens, failing as "release during a different test".

This is the scenario of #1432.

Fix this and numerous other edge cases by making the returned
callbacks from `assert.async()` strict about which locks they release.

Each lock now adds a unique token to a map, and invoking the release
function decrements/removes this token from the map.

== Notes ==

* es6-map.js assigns the fallback in all browsers.
  This is a bug, to be fixed later.

* The `isNaN(semaphore)` logic was originally added in 2015 by ea3e350.
  At the time, the internal resume function was public, and NaN could
  emerge through `QUnit.start("bla")` as result of `semaphore += "bla"`.
  This has not been possible for a while. During PR #1590, I did not
  trace the origin of this code, and thus did not realize that it was
  already obsolete (the semaphore itself is not publicly supported).

* The "during different test" error is now almost impossible to
  trigger since we now kill pending locks during test failures and
  tolerate all late calls equally. This meant the `drooling-done.js`
  test case now fails in a more limited way.

  I added a new test case for coverage, that reproduces it still, but
  it's a lot more obscure – it requires the original test to pass and
  then also have an unexpected call during a different test.

* I considered using the phrase "async lock" in the public-facing
  error messages, but found this perhaps too internal/technical
  when coming from the perspective of `var done = assert.async();`.

  In order to keep the code shared between handling of async-await,
  Promise, and assert.async, but remain friendly and understandable,
  I went for the phrase "async pause".

Fixes #1432.

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.asyncPauses.size ) {
 				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.asyncNextPauseId = 1;
+	this.asyncPauses = new Map();
 	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.asyncPauses.size > 0 ) {
 				pushFailure(
 					"Test did not finish synchronously even though assert.timeout( 0 ) was used.",
 					sourceFromStacktrace( 2 )
@@ -810,22 +811,106 @@ 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 `killed = true`. The killed flag 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 is simply throws an error, after which uncaught error handling picks it up
+//   and processing resumes.
+export function internalStop( test, requiredCalls = 1 ) {
 	config.blocking = true;
 
-	// Set a recovery timeout, if so configured.
+	const pauseId = `#${test.asyncNextPauseId++}`;
+	const pause = {
+		killed: false,
+		remaining: requiredCalls
+	};
+	test.asyncPauses.set( pauseId, pause );
+
+	function release() {
+		if ( pause.killed ) {
+			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.asyncPauses.delete( pauseId );
+		}
+
+		internalStart( test );
+	}
+
 	if ( setTimeout ) {
 		let timeoutDuration;
-
 		if ( typeof test.timeout === "number" ) {
 			timeoutDuration = test.timeout;
 		} else if ( typeof config.testTimeout === "number" ) {
 			timeoutDuration = config.testTimeout;
 		}
 
+		// Set a recovery timeout, if so configured.
 		if ( typeof timeoutDuration === "number" && timeoutDuration > 0 ) {
 			config.timeoutHandler = function( timeout ) {
 				return function() {
@@ -834,8 +919,10 @@ export function internalStop( test ) {
 						`Test took longer than ${timeout}ms; test timed out.`,
 						sourceFromStacktrace( 2 )
 					);
-					released = true;
-					internalRecover( test );
+
+					pause.killed = true;
+					test.asyncPauses.delete( pauseId );
+					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.asyncPauses.forEach( pause => {
+		pause.killed = true;
+	} );
+	test.asyncPauses.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.asyncPauses.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.asyncPauses.size > 0 ) {
 				return;
 			}
 

test/cli/fixtures/drooling-done.js

@@ -10,5 +10,5 @@ QUnit.test( "Test A", assert => {
 
 QUnit.test( "Test B", assert => {
 	assert.ok( true );
-	done();
+	done(); // Silently ignored since "Test A" already failed and we killed its async pauses.
 } );

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();
+} );