Core: Count tests run so that 'suiteEnd' emits correctly with active filters

Previously, the `suiteEnd` event and the `moduleDone` callback
were fired when we've run all the tests, based on counting the
number of registered tests in a module and the number of tests
actually run.

This logic does not hold up when there are active module/test filters
used in the runner, because then some of the registered tests will
never run.

The added tests demonstrate the issue, as they will fail without
the src patch. Several of the `suiteEnd` events would not fire.

The impact of this bug was that the `suiteEnd` event and `moduleDone`
callback don't fire if there were active filters such as `--filters`
in the CLI or via the "Filter" field in the HTML Reporter.

This in turn meant that reporters such as TAP would sometimes
reflect the structure of the test suite in an odd way due to
it sometimes missing an "end" event for a group of tests.

Fixes #1416.
Closes #1417.

Author: step2yeung

Gruntfile.js

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

src/core/config.js

@@ -46,7 +46,7 @@ const config = {
 		tests: [],
 		childModules: [],
 		testsRun: 0,
-		unskippedTestsRun: 0,
+		testsIgnored: 0,
 		hooks: {
 			before: [],
 			beforeEach: [],

src/module.js

@@ -28,7 +28,7 @@ function createModule( name, testEnvironment, modifiers ) {
 		tests: [],
 		moduleId: generateHash( moduleName ),
 		testsRun: 0,
-		unskippedTestsRun: 0,
+		testsIgnored: 0,
 		childModules: [],
 		suiteReport: new SuiteReport( name, parentSuite ),
 

src/test.js

@@ -206,7 +206,7 @@ Test.prototype = {
 
 		const runHook = () => {
 			if ( hookName === "before" ) {
-				if ( hookOwner.unskippedTestsRun !== 0 ) {
+				if ( hookOwner.testsRun !== 0 ) {
 					return;
 				}
 
@@ -216,7 +216,7 @@ Test.prototype = {
 			// The 'after' hook should only execute when there are not tests left and
 			// when the 'after' and 'finish' tasks are the only tasks left to process
 			if ( hookName === "after" &&
-				hookOwner.unskippedTestsRun !== numberOfUnskippedTests( hookOwner ) - 1 &&
+				!lastTestWithinModuleExecuted( hookOwner ) &&
 				( config.queue.length > 0 || ProcessingQueue.taskCount() > 2 ) ) {
 				return;
 			}
@@ -308,7 +308,11 @@ Test.prototype = {
 			}
 		}
 
-		notifyTestsRan( module, skipped );
+		if ( skipped ) {
+			incrementTestsIgnored( module );
+		} else {
+			incrementTestsRun( module );
+		}
 
 		// Store result when possible
 		if ( storage ) {
@@ -343,13 +347,13 @@ Test.prototype = {
 			// generating stack trace is expensive, so using a getter will help defer this until we need it
 			get source() { return test.stack; }
 		} ).then( function() {
-			if ( module.testsRun === numberOfTests( module ) ) {
+			if ( allTestsExecuted( module ) ) {
 				const completedModules = [ module ];
 
 				// Check if the parent modules, iteratively, are done. If that the case,
 				// we emit the `suiteEnd` event and trigger `moduleDone` callback.
 				let parent = module.parentModule;
-				while ( parent && parent.testsRun === numberOfTests( parent ) ) {
+				while ( parent && allTestsExecuted( parent ) ) {
 					completedModules.push( parent );
 					parent = parent.parentModule;
 				}
@@ -393,6 +397,7 @@ Test.prototype = {
 		const test = this;
 
 		if ( !this.valid() ) {
+			incrementTestsIgnored( this.module );
 			return;
 		}
 
@@ -852,23 +857,32 @@ function collectTests( module ) {
 	return tests;
 }
 
-function numberOfTests( module ) {
-	return collectTests( module ).length;
+// This returns true after all executable and skippable tests
+// in a module have been proccessed, and informs 'suiteEnd'
+// and moduleDone().
+function allTestsExecuted( module ) {
+	return module.testsRun + module.testsIgnored === collectTests( module ).length;
 }
 
-function numberOfUnskippedTests( module ) {
-	return collectTests( module ).filter( test => !test.skip ).length;
+// This returns true during the last executable non-skipped test
+// within a module, and informs the running of the 'after' hook
+// for a given module. This runs only once for a given module,
+// but must run during the last non-skipped test. When it runs,
+// there may be non-zero skipped tests left.
+function lastTestWithinModuleExecuted( module ) {
+	return module.testsRun === collectTests( module ).filter( test => !test.skip ).length - 1;
 }
 
-function notifyTestsRan( module, skipped ) {
+function incrementTestsRun( module ) {
 	module.testsRun++;
-	if ( !skipped ) {
-		module.unskippedTestsRun++;
-	}
 	while ( ( module = module.parentModule ) ) {
 		module.testsRun++;
-		if ( !skipped ) {
-			module.unskippedTestsRun++;
-		}
+	}
+}
+
+function incrementTestsIgnored( module ) {
+	module.testsIgnored++;
+	while ( ( module = module.parentModule ) ) {
+		module.testsIgnored++;
 	}
 }

test/events-filters.html

@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<html>
+<head>
+	<meta charset="UTF-8">
+	<title>Events Filters</title>
+	<link rel="stylesheet" href="../dist/qunit.css">
+	<script src="../dist/qunit.js"></script>
+	<script src="events-filters.js"></script>
+</head>
+<body>
+	<div id="qunit"></div>
+	<div id="qunit-fixture"></div>
+</body>
+</html>

test/events-filters.js

@@ -0,0 +1,106 @@
+/**
+ * This test file verifies the execution order of events emitted
+ * by QUnit when there are active module filters and test filters.
+ */
+
+// These are "module1", "module3", "module4" and "verify"
+QUnit.config.moduleId = [ "1cf055b9", "46b9bb3b", "db9e6dfc", "c056c5ed" ];
+QUnit.config.filter = "!SKIPME";
+QUnit.config.reorder = false;
+
+var invokedHooks = [],
+	done = false;
+
+function callback( name ) {
+	return function( details ) {
+		if ( done ) {
+			return;
+		}
+
+		invokedHooks.push( name + ": " + details.name );
+	};
+}
+
+QUnit.on( "suiteStart", callback( "suiteStart" ) );
+QUnit.on( "testStart", callback( "testStart" ) );
+QUnit.on( "testEnd", callback( "testEnd" ) );
+QUnit.on( "suiteEnd", callback( "suiteEnd" ) );
+
+QUnit.on( "suiteEnd", function( details ) {
+	if ( !done && details.name === "module4" ) {
+		done = true;
+	}
+} );
+
+
+// matches module filter
+QUnit.module( "module1", function() {
+	QUnit.test( "test 1.1", function( assert ) {
+		assert.true( true );
+	} );
+	QUnit.test( "test 1.2", function( assert ) {
+		assert.true( true );
+	} );
+} );
+
+// skipped by module filter
+QUnit.module( "module2", function() {
+	QUnit.test( "test 2.1", function( assert ) {
+		assert.true( false );
+	} );
+	QUnit.test( "test 2.2", function( assert ) {
+		assert.true( false );
+	} );
+} );
+
+// matches module filter
+QUnit.module( "module3", function() {
+	QUnit.test( "test 3.1", function( assert ) {
+		assert.true( true );
+	} );
+
+	// skipped by test filter
+	QUnit.test( "test 3.2 SKIPME", function( assert ) {
+		assert.true( false );
+	} );
+} );
+
+// matches module filter
+QUnit.module( "module4", function() {
+	QUnit.module( "module4-inner", function() {
+		QUnit.test( "test 4.1", function( assert ) {
+			assert.true( true );
+		} );
+
+		// skipped by test filter
+		QUnit.test( "test 4.2 SKIPME", function( assert ) {
+			assert.true( false );
+		} );
+	} );
+} );
+
+// matches module filter
+QUnit.module( "verify", function() {
+	QUnit.test( "events with active filters", function( assert ) {
+		assert.deepEqual( invokedHooks, [
+			"suiteStart: module1",
+			"testStart: test 1.1",
+			"testEnd: test 1.1",
+			"testStart: test 1.2",
+			"testEnd: test 1.2",
+			"suiteEnd: module1",
+
+			"suiteStart: module3",
+			"testStart: test 3.1",
+			"testEnd: test 3.1",
+			"suiteEnd: module3",
+
+			"suiteStart: module4",
+			"suiteStart: module4-inner",
+			"testStart: test 4.1",
+			"testEnd: test 4.1",
+			"suiteEnd: module4-inner",
+			"suiteEnd: module4"
+		] );
+	} );
+} );