Core: Fix bug calling hooks with skipped tests (#1156)

* Core: Fix bug calling hooks with skipped tests When the first/last test in a module was skipped, it would cause the module's before()/after() hook to not be called. This ensures those hooks will always be called. * Core: address PR feedback (to be squashed) * Core: Don't store array of unskipped tests Instead of storing an array of unskipped tests, we can just set a skip flag on each test, and compute the number of unskipped tests on the fly. * Core: code review feedback
qunitjs · Apr 14, 2017 · 2ce7e91 · 2ce7e91
1 parent eee0209
commit 2ce7e91
Show file tree

Hide file tree

Showing 5 changed files with 82 additions and 18 deletions.
diff --git a/src/core.js b/src/core.js
@@ -46,6 +46,7 @@ function createModule( name, testEnvironment ) {
 		tests: [],
 		moduleId: generateHash( moduleName ),
 		testsRun: 0,
+		unskippedTestsRun: 0,
 		childModules: [],
 		suiteReport: new SuiteReport( name, parentSuite )
 	};

diff --git a/src/core/config.js b/src/core/config.js
@@ -45,7 +45,8 @@ const config = {
 		name: "",
 		tests: [],
 		childModules: [],
-		testsRun: 0
+		testsRun: 0,
+		unskippedTestsRun: 0
 	},
 
 	callbacks: {},

diff --git a/src/test.js b/src/test.js
@@ -54,7 +54,8 @@ export default function Test( settings ) {
 
 	this.module.tests.push( {
 		name: this.testName,
-		testId: this.testId
+		testId: this.testId,
+		skip: !!settings.skip
 	} );
 
 	if ( settings.skip ) {
@@ -158,15 +159,15 @@ Test.prototype = {
 			test = this;
 		return function runHook() {
 			if ( hookName === "before" ) {
-				if ( hookOwner.testsRun !== 0 ) {
+				if ( hookOwner.unskippedTestsRun !== 0 ) {
 					return;
 				}
 
 				test.preserveEnvironment = true;
 			}
 
 			if ( hookName === "after" &&
-				hookOwner.testsRun !== numberOfTests( hookOwner ) - 1 &&
+				hookOwner.unskippedTestsRun !== numberOfUnskippedTests( hookOwner ) - 1 &&
 				config.queue.length > 2 ) {
 				return;
 			}
@@ -245,7 +246,7 @@ Test.prototype = {
 			}
 		}
 
-		notifyTestsRan( module );
+		notifyTestsRan( module, skipped );
 
 		// Store result when possible
 		if ( storage ) {
@@ -737,23 +738,37 @@ function internalStart( test ) {
 	}
 }
 
-function numberOfTests( module ) {
-	let count = module.tests.length;
+function collectTests( module ) {
+	const tests = [].concat( module.tests );
 	const modules = [ ...module.childModules ];
 
 	// Do a breadth-first traversal of the child modules
 	while ( modules.length ) {
 		const nextModule =  modules.shift();
-		count += nextModule.tests.length;
+		tests.push.apply( tests, nextModule.tests );
 		modules.push( ...nextModule.childModules );
 	}
 
-	return count;
+	return tests;
+}
+
+function numberOfTests( module ) {
+	return collectTests( module ).length;
+}
+
+function numberOfUnskippedTests( module ) {
+	return collectTests( module ).filter( test => !test.skip ).length;
 }
 
-function notifyTestsRan( module ) {
+function notifyTestsRan( module, skipped ) {
 	module.testsRun++;
+	if ( !skipped ) {
+		module.unskippedTestsRun++;
+	}
 	while ( ( module = module.parentModule ) ) {
 		module.testsRun++;
+		if ( !skipped ) {
+			module.unskippedTestsRun++;
+		}
 	}
 }
diff --git a/test/logs.js b/test/logs.js
@@ -15,11 +15,13 @@ var totalTests, moduleContext, moduleDoneContext, testContext, testDoneContext,
 		tests: [
 			( module1Test1 = {
 				"name": "test1",
-				"testId": "646e9e25"
+				"testId": "646e9e25",
+				"skip": false
 			} ),
 			( module1Test2 = {
 				"name": "test2",
-				"testId": "646e9e26"
+				"testId": "646e9e26",
+				"skip": false
 			} )
 		]
 	},
@@ -28,27 +30,33 @@ var totalTests, moduleContext, moduleDoneContext, testContext, testDoneContext,
 		tests: [
 			( module2Test1 = {
 				"name": "test1",
-				"testId": "9954d966"
+				"testId": "9954d966",
+				"skip": false
 			} ),
 			( module2Test2 = {
 				"name": "test2",
-				"testId": "9954d967"
+				"testId": "9954d967",
+				"skip": false
 			} ),
 			( module2Test3 = {
 				"name": "a skipped test",
-				"testId": "3e797d3a"
+				"testId": "3e797d3a",
+				"skip": true
 			} ),
 			( module2Test4 = {
 				"name": "test the log for the skipped test",
-				"testId": "d3266148"
+				"testId": "d3266148",
+				"skip": false
 			} ),
 			( module2Test5 = {
 				"name": "a todo test",
-				"testId": "77a47174"
+				"testId": "77a47174",
+				"skip": false
 			} ),
 			( module2Test6 = {
 				"name": "test the log for the todo test",
-				"testId": "5f5ab826"
+				"testId": "5f5ab826",
+				"skip": false
 			} )
 		]
 	};

diff --git a/test/main/modules.js b/test/main/modules.js
@@ -49,6 +49,18 @@ QUnit.test( "does not run before subsequent tests", function( assert ) {
 	assert.equal( this.beforeCount, 1, "beforeCount did not increase from last test" );
 } );
 
+QUnit.module( "before (skip)", {
+	before: function( assert ) {
+		assert.ok( true, "before hook ran" );
+	}
+} );
+
+QUnit.skip( "first test in module is skipped" );
+
+QUnit.test( "runs before first unskipped test", function( assert ) {
+	assert.expect( 1 );
+} );
+
 QUnit.module( "after", {
 	after: function( assert ) {
 		assert.ok( true, "after hook ran" );
@@ -63,6 +75,33 @@ QUnit.test( "runs after final test", function( assert ) {
 	assert.expect( 1 );
 } );
 
+QUnit.module( "after (skip)", {
+	after: function( assert ) {
+		assert.ok( true, "after hook ran" );
+	}
+} );
+
+QUnit.test( "does not run after initial tests", function( assert ) {
+	assert.expect( 0 );
+} );
+
+QUnit.test( "runs after final unskipped test", function( assert ) {
+	assert.expect( 1 );
+} );
+
+QUnit.skip( "last test in module is skipped" );
+
+QUnit.module( "before/after with all tests skipped", {
+	before: function( assert ) {
+		assert.ok( false, "should not occur" );
+	},
+	after: function( assert ) {
+		assert.ok( false, "should not occur" );
+	}
+} );
+
+QUnit.skip( "verifier" );
+
 QUnit.module( "Test context object", {
 	beforeEach: function( assert ) {
 		var key,