Core: Optimise module allocation, hoist some functions, clarify code

Two micro optimisations:

* Move the declaration of the hooks object to where the internal module
  is created more generally. Adding the property later seems confusing
  and was less optimal.

* Move two utility functions from within processModule() to outside
  of it as they didn't use any of its scope and thus were needlessly
  being re-allocated for every registered test module.

Clarity:

* Move `module.hooks` and `module.stats` property definitions to
  the module object literal rather than creating them sometime later
  as dynamic properties.

* Clarify that the unnamed module is not (and should not become)
  a "global module". I was thinking of doing this as a way to
  implement the upcoming global QUnit.hooks feature, but then
  remembered that these are two distinct concepts that I think are
  best kept separate.

This is preparation for #1475,
ref https://github.com/qunitjs/qunitjs.com/issues/161.

src/core.js

@@ -22,12 +22,15 @@ import onWindowError from "./core/onerror";
 import onUncaughtException from "./core/on-uncaught-exception";
 
 const QUnit = {};
-export const globalSuite = new SuiteReport();
+export const runSuite = new SuiteReport();
 
-// The initial "currentModule" represents the global (or top-level) module that
-// is not explicitly defined by the user, therefore we add the "globalSuite" to
-// it since each module has a suiteReport associated with it.
-config.currentModule.suiteReport = globalSuite;
+// The "currentModule" object would ideally be defined using the createModule()
+// function. Since it isn't, add the missing suiteReport property to it now that
+// we have loaded all source code required to do so.
+//
+// TODO: Consider defining currentModule in core.js or module.js in its entirely
+// rather than partly in config.js and partly here.
+config.currentModule.suiteReport = runSuite;
 
 let globalStartCalled = false;
 let runStarted = false;
@@ -187,7 +190,7 @@ export function begin() {
 	}
 
 	// The test run is officially beginning now
-	emit( "runStart", globalSuite.start( true ) );
+	emit( "runStart", runSuite.start( true ) );
 	runLoggingCallbacks( "begin", {
 		totalTests: Test.count,
 		modules: modulesLog

src/core/config.js

@@ -47,6 +47,38 @@ const config = {
 	modules: [],
 
 	// The first unnamed module
+	//
+	// By being defined as the intial value for currentModule, it is the
+	// receptacle and implied parent for any global tests. It is as if we
+	// called `QUnit.module( "" );` before any other tests were defined.
+	//
+	// If we reach begin() and no tests were put in it, we dequeue it as if it
+	// never existed, and in that case never expose it to the events and
+	// callbacks API.
+	//
+	// When global tests are defined, then this unnamed module will execute
+	// as any other module, including moduleStart/moduleDone events etc.
+	//
+	// Since this module isn't explicitly created by the user, they have no
+	// access to add hooks for it. The hooks object is defined to comply
+	// with internal expectations of test.js, but they will be empty.
+	// To apply hooks, place tests explicitly in a QUnit.module(), and use
+	// its hooks accordingly.
+	//
+	// For global hooks that apply to all tests and all modules, use QUnit.hooks.
+	//
+	// NOTE: This is *not* a "global module". It is not an ancestor of all modules
+	// and tests. It is merely the parent for any tests defined globally,
+	// before the first QUnit.module(). As such, the events for this unnamed
+	// module will fire as normal, right after its last test, and *not* at
+	// the end of the test run.
+	//
+	// NOTE: This also should probably also not become a global module, unless
+	// we keep it out of the public API. For example, it would likely not
+	// improve the user interface and plugin behaviour if all modules became
+	// wrapped between the start and end events of this module, and thus
+	// needlessly add indentation, indirection, or other visible noise.
+	// Unit tests for the callbacks API would detect that as a regression.
 	currentModule: {
 		name: "",
 		tests: [],

src/core/on-uncaught-exception.js

@@ -1,5 +1,5 @@
 import config from "./config";
-import { globalSuite } from "../core";
+import { runSuite } from "../core";
 import { sourceFromStacktrace } from "./stacktrace";
 import { errorString } from "./utilities";
 import { emit } from "../events";
@@ -43,7 +43,7 @@ export default function onUncaughtException( error ) {
 		// Increase "bad assertion" stats despite no longer pushing an assertion in this case.
 		// This ensures "runEnd" and "QUnit.done()" handlers behave as expected, since the "bad"
 		// count is typically how reporters decide on the boolean outcome of the test run.
-		globalSuite.globalFailureCount++;
+		runSuite.globalFailureCount++;
 		config.stats.bad++;
 		config.stats.all++;
 		emit( "error", error );

src/core/processing-queue.js

@@ -13,7 +13,7 @@ import {
 	test
 } from "../test";
 import {
-	globalSuite
+	runSuite
 } from "../core";
 import {
 	emit
@@ -201,7 +201,7 @@ function done() {
 
 	ProcessingQueue.finished = true;
 
-	emit( "runEnd", globalSuite.end( true ) );
+	emit( "runEnd", runSuite.end( true ) );
 	runLoggingCallbacks( "done", {
 		passed,
 		failed: config.stats.bad,

src/module.js

@@ -5,7 +5,7 @@ import config from "./core/config";
 import SuiteReport from "./reports/suite";
 
 import { extend, objectType, generateHash } from "./core/utilities";
-import { globalSuite } from "./core";
+import { runSuite } from "./core";
 
 const moduleStack = [];
 
@@ -19,21 +19,38 @@ function isParentModuleInQueue() {
 function createModule( name, testEnvironment, modifiers ) {
 	const parentModule = moduleStack.length ? moduleStack.slice( -1 )[ 0 ] : null;
 	const moduleName = parentModule !== null ? [ parentModule.name, name ].join( " > " ) : name;
-	const parentSuite = parentModule ? parentModule.suiteReport : globalSuite;
+	const parentSuite = parentModule ? parentModule.suiteReport : runSuite;
 
 	const skip = parentModule !== null && parentModule.skip || modifiers.skip;
 	const todo = parentModule !== null && parentModule.todo || modifiers.todo;
 
+	const env = {};
+	if ( parentModule ) {
+		extend( env, parentModule.testEnvironment );
+	}
+	extend( env, testEnvironment );
+
 	const module = {
 		name: moduleName,
 		parentModule: parentModule,
+		hooks: {
+			before: [],
+			beforeEach: [],
+			afterEach: [],
+			after: []
+		},
+		testEnvironment: env,
 		tests: [],
 		moduleId: generateHash( moduleName ),
 		testsRun: 0,
 		testsIgnored: 0,
 		childModules: [],
 		suiteReport: new SuiteReport( name, parentSuite ),
 
+		// Initialised by test.js when the module start executing,
+		// i.e. before the first test in this module (or a child).
+		stats: null,
+
 		// Pass along `skip` and `todo` properties from parent module, in case
 		// there is one, to childs. And use own otherwise.
 		// This property will be used to mark own tests and tests of child suites
@@ -43,18 +60,35 @@ function createModule( name, testEnvironment, modifiers ) {
 		ignored: modifiers.ignored || false
 	};
 
-	const env = {};
 	if ( parentModule ) {
 		parentModule.childModules.push( module );
-		extend( env, parentModule.testEnvironment );
 	}
-	extend( env, testEnvironment );
-	module.testEnvironment = env;
 
 	config.modules.push( module );
 	return module;
 }
 
+function setHookFromEnvironment( hooks, environment, name ) {
+	const potentialHook = environment[ name ];
+	if ( typeof potentialHook === "function" ) {
+		hooks[ name ].push( potentialHook );
+	}
+	delete environment[ name ];
+}
+
+function makeSetHook( module, hookName ) {
+	return function setHook( callback ) {
+		if ( config.currentModule !== module ) {
+			Logger.warn( "The `" + hookName + "` hook was called inside the wrong module (`" +
+				config.currentModule.name + "`). " +
+				"Instead, use hooks provided by the callback to the containing module (`" +
+				module.name + "`). " +
+				"This will become an error in QUnit 3.0." );
+		}
+		module.hooks[ hookName ].push( callback );
+	};
+}
+
 function processModule( name, options, executeNow, modifiers = {} ) {
 	if ( objectType( options ) === "function" ) {
 		executeNow = options;
@@ -63,20 +97,20 @@ function processModule( name, options, executeNow, modifiers = {} ) {
 
 	const module = createModule( name, options, modifiers );
 
-	// Move any hooks to a 'hooks' object
+	// Transfer any initial hooks from the options object to the 'hooks' object
 	const testEnvironment = module.testEnvironment;
-	const hooks = module.hooks = {};
+	const hooks = module.hooks;
 
 	setHookFromEnvironment( hooks, testEnvironment, "before" );
 	setHookFromEnvironment( hooks, testEnvironment, "beforeEach" );
 	setHookFromEnvironment( hooks, testEnvironment, "afterEach" );
 	setHookFromEnvironment( hooks, testEnvironment, "after" );
 
 	const moduleFns = {
-		before: setHookFunction( module, "before" ),
-		beforeEach: setHookFunction( module, "beforeEach" ),
-		afterEach: setHookFunction( module, "afterEach" ),
-		after: setHookFunction( module, "after" )
+		before: makeSetHook( module, "before" ),
+		beforeEach: makeSetHook( module, "beforeEach" ),
+		afterEach: makeSetHook( module, "afterEach" ),
+		after: makeSetHook( module, "after" )
 	};
 
 	const prevModule = config.currentModule;
@@ -102,25 +136,6 @@ function processModule( name, options, executeNow, modifiers = {} ) {
 			config.currentModule = module.parentModule || prevModule;
 		}
 	}
-
-	function setHookFromEnvironment( hooks, environment, name ) {
-		const potentialHook = environment[ name ];
-		hooks[ name ] = typeof potentialHook === "function" ? [ potentialHook ] : [];
-		delete environment[ name ];
-	}
-
-	function setHookFunction( module, hookName ) {
-		return function setHook( callback ) {
-			if ( config.currentModule !== module ) {
-				Logger.warn( "The `" + hookName + "` hook was called inside the wrong module (`" +
-					config.currentModule.name + "`). " +
-					"Instead, use hooks provided by the callback to the containing module (`" +
-					module.name + "`). " +
-					"This will become an error in QUnit 3.0." );
-			}
-			module.hooks[ hookName ].push( callback );
-		};
-	}
 }
 
 let focused = false; // indicates that the "only" filter was used

src/reporters/TapReporter.js

@@ -199,7 +199,7 @@ export default class TapReporter {
 		return new TapReporter( runner, options );
 	}
 
-	onRunStart( _globalSuite ) {
+	onRunStart( _runSuite ) {
 		this.log( "TAP version 13" );
 	}
 
@@ -246,14 +246,14 @@ export default class TapReporter {
 		}
 	}
 
-	onRunEnd( globalSuite ) {
+	onRunEnd( runSuite ) {
 		this.ended = true;
 
-		this.log( `1..${globalSuite.testCounts.total}` );
-		this.log( `# pass ${globalSuite.testCounts.passed}` );
-		this.log( kleur.yellow( `# skip ${globalSuite.testCounts.skipped}` ) );
-		this.log( kleur.cyan( `# todo ${globalSuite.testCounts.todo}` ) );
-		this.log( kleur.red( `# fail ${globalSuite.testCounts.failed}` ) );
+		this.log( `1..${runSuite.testCounts.total}` );
+		this.log( `# pass ${runSuite.testCounts.passed}` );
+		this.log( kleur.yellow( `# skip ${runSuite.testCounts.skipped}` ) );
+		this.log( kleur.cyan( `# todo ${runSuite.testCounts.todo}` ) );
+		this.log( kleur.red( `# fail ${runSuite.testCounts.failed}` ) );
 	}
 
 	logAssertion( error, severity ) {

src/reports/suite.js

@@ -6,8 +6,8 @@ export default class SuiteReport {
 		this.fullName = parentSuite ? parentSuite.fullName.concat( name ) : [];
 
 		// When an "error" event is emitted from onUncaughtException(), the
-		// "runEnd" event should report the status as failed.
-		// The "runEnd" event data is made by this class (as "globalSuite").
+		// "runEnd" event should report the status as failed. The "runEnd" event data
+		// is tracked through this property (via the "runSuite" instance).
 		this.globalFailureCount = 0;
 
 		this.tests = [];