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.

Fixes #1432.

docs/assert/async.md

@@ -12,17 +12,17 @@ redirect_from:
 version_added: "1.16.0"
 ---
 
-`async( [ acceptCallCount = 1 ] )`
+`async( [ count = 1 ] )`
 
 Instruct QUnit to wait for an asynchronous operation.
 
 | name | description |
 |------|-------------|
-| `acceptCallCount` (number) | Number of expected callbacks before the test is done. Defaults to `1`. |
+| `count` (number) | Number of expected calls before the tests will resume. Defaults to `1`. |
 
 ### Description
 
-`assert.async()` returns a callback function and pauses test processing until the callback function is invoked the specified number of times. The callback will throw an `Error` if it is invoked more often than the accepted call count.
+`assert.async()` returns a callback function and pauses test processing until the callback function is called. The callback will throw an `Error` if it is invoked more often than the required call count.
 
 This replaces functionality previously provided by `QUnit.stop()` and [`QUnit.start()`](../QUnit/start.md).
 
@@ -69,7 +69,7 @@ QUnit.test( "two async calls", assert => {
 
 ##### Example: Require multiple calls
 
-The `acceptCallCount` parameter can be used to require multiple calls to the same callback. In the below example, the test passes after exactly three calls.
+The `count` parameter can be used to require multiple calls to the same callback. In the below example, the test passes after exactly three calls.
 
 ```js
 function uploadBatch(batch, notify, complete) {
@@ -80,7 +80,7 @@ function uploadBatch(batch, notify, complete) {
   complete(null)
 }
 
-QUnit.test( "acceptCallCount example", assert => {
+QUnit.test( "multiple calls example", assert => {
   assert.timeout( 1000 );
 
   const notify = assert.async( 3 );

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,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 on processing and return a new function that can release the pause.
+//
+// This mechanism is used internally 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.
+//
+// The `requiredCalls` parameter exists to support `assert.async(count)`.
+//
+// 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 forcibly empties the list of
+//   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 confusing "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 then also causes an uncaught error. The uncaught error means we will no longer
+//   wait for the release (as it might never arrive), and move on to a different test.
+//
+// * Pause is never released, or called an insufficient number of times.
+//
+//   Our timeout handler will kill the pause and resume test processing, basically the same
+//   way as internalRecover() but for just one pause instead of any/all.
+//
+//   Similarly, any late calls to resume() are silently ignored to avoid erorrs during
+//   different tests. We tolerate this since the original test will have already been
+//   marked as failure.
+//
+//   TODO: QUnit 3 will provide a default timeout <https://github.com/qunitjs/qunit/issues/1483>,
+//   but right now a test will hang indefinitely if async pauses are not released, unless
+//   the test suite explicitly enables them via  QUnit.config.testTimeout or assert.timeout().
+//
+// * Pause is spontaneously released during a different test, or when no test is currently running.
+//   This should be impossible since the previous two scenarious should cover
+//   all ways to exit a test in failure (and thus ignore late calls).
+//
+// * Pause release() is called, during the same test, after it was already released.
+//   This is an easy mistake and simply throws an error, which then results the uncaught error
+//   handling and resume processing.
+//
+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}]` );
+		}
+
+		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 +913,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 +927,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();
+} );