From d47240342a63f1a104eef8fef79b0fb2037e2154 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Sun, 6 Feb 2022 23:22:16 +0000 Subject: [PATCH] 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 https://github.com/qunitjs/qunit/issues/1475, ref https://github.com/qunitjs/qunitjs.com/issues/161. --- src/core.js | 15 +++--- src/core/config.js | 32 +++++++++++++ src/core/on-uncaught-exception.js | 4 +- src/core/processing-queue.js | 4 +- src/module.js | 77 ++++++++++++++++++------------- src/reporters/TapReporter.js | 14 +++--- src/reports/suite.js | 4 +- 7 files changed, 100 insertions(+), 50 deletions(-) diff --git a/src/core.js b/src/core.js index 57209978c..b6d8222d4 100644 --- a/src/core.js +++ b/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 diff --git a/src/core/config.js b/src/core/config.js index 19289b21a..3dd7b82f0 100644 --- a/src/core/config.js +++ b/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: [], diff --git a/src/core/on-uncaught-exception.js b/src/core/on-uncaught-exception.js index e32280e44..c8f0ad156 100644 --- a/src/core/on-uncaught-exception.js +++ b/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 ); diff --git a/src/core/processing-queue.js b/src/core/processing-queue.js index 17204c7bd..816059b2b 100644 --- a/src/core/processing-queue.js +++ b/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, diff --git a/src/module.js b/src/module.js index feb04412c..d065e5111 100644 --- a/src/module.js +++ b/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,14 +19,27 @@ 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, @@ -34,6 +47,10 @@ function createModule( name, testEnvironment, modifiers ) { 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,9 +97,9 @@ 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" ); @@ -73,10 +107,10 @@ function processModule( name, options, executeNow, modifiers = {} ) { 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 diff --git a/src/reporters/TapReporter.js b/src/reporters/TapReporter.js index 65c3caa6b..1ac4ca4b4 100644 --- a/src/reporters/TapReporter.js +++ b/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 ) { diff --git a/src/reports/suite.js b/src/reports/suite.js index a355a343c..973dbf9b0 100644 --- a/src/reports/suite.js +++ b/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 = [];