From 5812597b7f086e6afafef947ebff5231c0011f6b Mon Sep 17 00:00:00 2001
From: Timo Tijhof <krinkle@fastmail.com>
Date: Sun, 28 Jul 2024 20:30:40 +0100
Subject: [PATCH] Core: Make initial "unnamed" QUnit.config.currentModule
 complete

Before this change, the unnamed module object was not created by
`createModule()`. Instead, it was declared in config.js as the first
value for `config.currentModule`, with only some of the properties.

It had enough to satisfy internal execution requirements, but some
publicly observable stuff like `moduleId` and `testEnvironment` were
not defined.

Misc changes and optimisations:

* `suiteReport` is now injected instead of overridden afterwards.

* `moduleStack` state is moved to config along with other internal
  state. This removes one of the last remaining state that wasn't
  resettable.

* When creating child modulese, avoid allocating unused `env = {}`.

* When creating module objects, avoid needless copying of the
  moduleStack via Array slice().

* In test.js, simplify how we fill `completedModules` by using the
  same loop for both parenet modules and the current module alike,
  thus removing duplicate code.
---
 src/core/browser/browser-runner.js |  2 +-
 src/core/config.js                 | 73 +++++++++---------------------
 src/core/core.js                   | 11 +----
 src/core/module.js                 | 65 +++++++++++++++++---------
 src/core/on-uncaught-exception.js  |  4 +-
 src/core/processing-queue.js       |  6 +--
 src/core/reporters/TapReporter.js  | 12 ++---
 src/core/reports/suite.js          |  2 +-
 src/core/stacktrace.js             |  8 ++--
 src/core/start.js                  | 20 ++++----
 src/core/test.js                   | 53 ++++++++++------------
 11 files changed, 121 insertions(+), 135 deletions(-)

diff --git a/src/core/browser/browser-runner.js b/src/core/browser/browser-runner.js
index d6545f84a..5e0f3b501 100644
--- a/src/core/browser/browser-runner.js
+++ b/src/core/browser/browser-runner.js
@@ -83,7 +83,7 @@ export function initBrowser (QUnit, window, document) {
   //   as otherwise it will miss the first few or even all synchronous events.
   //
   // Priot to QUnit 3.0, the reporter was initialised here, between error handler (above),
-  // and start (below). As of QUnit 3.0, reporters are initialized by doBegin() within
+  // and start (below). As of QUnit 3.0, reporters are initialized by doStart() within
   // QUnit.start(), which is logically the same place, but decoupled from initBrowser().
 
   function autostart () {
diff --git a/src/core/config.js b/src/core/config.js
index 75b6fb198..c85c71ce6 100644
--- a/src/core/config.js
+++ b/src/core/config.js
@@ -97,58 +97,29 @@ const config = {
   // List of defined modules (read-only).
   modules: [],
 
-  // Internal: 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: [],
-    childModules: [],
-    testsRun: 0,
-    testsIgnored: 0,
-    hooks: {
-      before: [],
-      beforeEach: [],
-      afterEach: [],
-      after: []
-    }
-  },
-
   // Semi-internal state.
   //
   // These are undocumented but defacto stable for certain limited use cases,
   // in order to maintain ecosystem compat with popular QUnit 2.x plugins and integrations.
   //
+  // - currentModule: This object represents the most recent `QUnit.module()` call,
+  //   and is used by functions like `QUnit.test()` to determine their module parent.
+  //   It is also referred to from `config.modules` and `config._moduleStack`.
+  //
+  //   This starts out with an unnamed placeholder module to hold any "global" tests.
+  //   The unnamed module was introduced in QUnit 1.16. When we reach doStart() in start.js,
+  //   if no global tests exist, the unnamed module will be removed `config.modules`, as if
+  //   it never existed, and thus never exposed to the events and callbacks API.
+  //
+  //   Note that this unnamed initial module is not a "root" module, it is not an ancestor
+  //   to any other modules. Doing so would negatively impact developer experience by ading
+  //   needless indentation, indirection, and other visible noise in test results (or require
+  //   workarounds to prevent that). Since the unnamed module is a regular module, it will
+  //   "end" after the last global test (i.e. before the first named module), and not e.g.
+  //   at the end of the test run.
+  //   To set global hooks, use `QUnit.hooks` instead.
+  //   To listen for the end of the run, handle the "runEnd" event from `QUnit.on()`.
+  //
   // - blocking: Whether new tests will be defined and queued, or executed immediately.
   //   In other words, whether QUnit.start() has been called yet.
   //
@@ -160,6 +131,8 @@ const config = {
   // - stats: Internal assertion counts. Use `QUnit.on('runEnd')` instead.
   //   These are discouraged per the notice at https://qunitjs.com/api/callbacks/QUnit.done/.
   //   https://qunitjs.com/api/callbacks/QUnit.on/#the-runend-event
+  //
+  currentModule: null, // initial unnamed module for "global tests" assigned in core.js.
   blocking: true,
   started: 0,
   callbacks: {},
@@ -168,8 +141,9 @@ const config = {
 
   // Internal state, exposed to ease in-process resets
   // Ref https://github.com/qunitjs/qunit/pull/1598
+  _moduleStack: [],
   _globalHooks: {},
-  _pq: null, // ProcessingQueue singleton, assigned in /src/core.js
+  _pq: null, // ProcessingQueue singleton, assigned in core.js
   _runStarted: false,
   _event_listeners: Object.create(null),
   _event_memory: {}
@@ -271,7 +245,4 @@ if (urlParams.seed === true) {
   readFlatPreconfigString(urlParams.seed, 'seed');
 }
 
-// Push a loose unnamed module to the modules collection
-config.modules.push(config.currentModule);
-
 export default config;
diff --git a/src/core/core.js b/src/core/core.js
index 17ea04be8..f60f97501 100644
--- a/src/core/core.js
+++ b/src/core/core.js
@@ -1,7 +1,7 @@
 import { window } from './globals.js';
 import equiv from './equiv.js';
 import dump from './dump.js';
-import { runSuite, module } from './module.js';
+import { unnamedModule, module } from './module.js';
 import Assert from './assert.js';
 import { test, pushFailure } from './test.js';
 import reporters from './reporters.js';
@@ -18,14 +18,7 @@ import diff from './diff.js';
 import version from './version.js';
 import { createStartFunction } from './start.js';
 
-// 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;
-
+config.currentModule = unnamedModule;
 config._pq = new ProcessingQueue();
 
 const QUnit = {
diff --git a/src/core/module.js b/src/core/module.js
index e195eebe9..bb5ced1fa 100644
--- a/src/core/module.js
+++ b/src/core/module.js
@@ -2,31 +2,45 @@ import config from './config.js';
 import SuiteReport from './reports/suite.js';
 import { extend, generateHash, isAsyncFunction } from './utilities.js';
 
-const moduleStack = [];
+export const globalSuiteReport = new SuiteReport();
 
-export const runSuite = new SuiteReport();
+// This also pushes the unnamed module to config.modules.
+// It is important that we not push this into the moduleStack,
+// since it is not meant to be a parent to any other modules.
+export const unnamedModule = createModule('', {}, {}, globalSuiteReport);
 
 function isParentModuleInQueue () {
   const modulesInQueue = config.modules
     .filter(module => !module.ignored)
     .map(module => module.moduleId);
-  return moduleStack.some(module => modulesInQueue.includes(module.moduleId));
+  return config._moduleStack.some(module => modulesInQueue.includes(module.moduleId));
 }
 
-function createModule (name, testEnvironment, modifiers) {
-  const parentModule = moduleStack.length ? moduleStack.slice(-1)[0] : null;
-  const parentReport = parentModule ? parentModule.suiteReport : runSuite;
+/**
+ * This does:
+ * - Create a module object
+ * - Link it to and from a parent module (if one is on the stack)
+ * - Link it to a parent SuiteReport (if one is on the stack)
+ * - Push it to `config.modules`
+ *
+ * It does NOT push it to config._moduleStack. That's only relevant for
+ * scoped modules, and is the responsibility of processModule().
+ *
+ * @param {string} name
+ * @param {Object} testEnvironment
+ * @param {Object} modifiers
+ * @param {SuiteReport} suiteReport Force the report, for use by the initial unnamedModule
+ * @return {Object}
+ */
+function createModule (name, testEnvironment, modifiers, suiteReport) {
+  const parentModule = config._moduleStack.length
+    ? config._moduleStack[config._moduleStack.length - 1]
+    : null;
   const moduleName = parentModule !== null ? [parentModule.name, name].join(' > ') : name;
 
   const skip = (parentModule !== null && parentModule.skip) || modifiers.skip;
   const todo = (parentModule !== null && parentModule.todo) || modifiers.todo;
 
-  let env = {};
-  if (parentModule) {
-    env = Object.create(parentModule.testEnvironment || {});
-  }
-  extend(env, testEnvironment);
-
   const module = {
     name: moduleName,
     parentModule: parentModule,
@@ -36,13 +50,20 @@ function createModule (name, testEnvironment, modifiers) {
       afterEach: [],
       after: []
     },
-    testEnvironment: env,
+    testEnvironment: extend(
+      // Live inheritence as of QUnit 3. https://github.com/qunitjs/qunit/pull/1762
+      (parentModule ? Object.create(parentModule.testEnvironment || {}) : {}),
+      testEnvironment
+    ),
     tests: [],
     moduleId: generateHash(moduleName),
     testsRun: 0,
     testsIgnored: 0,
     childModules: [],
-    suiteReport: new SuiteReport(name, parentReport),
+    suiteReport: suiteReport || new SuiteReport(
+      name,
+      parentModule ? parentModule.suiteReport : globalSuiteReport
+    ),
 
     // Initialised by test.js when the module start executing,
     // i.e. before the first test in this module (or a child).
@@ -62,10 +83,11 @@ function createModule (name, testEnvironment, modifiers) {
   }
 
   config.modules.push(module);
+
   return module;
 }
 
-function setHookFromEnvironment (hooks, environment, name) {
+function setHookFromEnvironment (environment, hooks, name) {
   const potentialHook = environment[name];
   if (typeof potentialHook === 'function') {
     hooks[name].push(potentialHook);
@@ -97,11 +119,10 @@ function processModule (name, options, scope, modifiers = {}) {
   // Transfer any initial hooks from the options object to the 'hooks' object
   const testEnvironment = module.testEnvironment;
   const hooks = module.hooks;
-
-  setHookFromEnvironment(hooks, testEnvironment, 'before');
-  setHookFromEnvironment(hooks, testEnvironment, 'beforeEach');
-  setHookFromEnvironment(hooks, testEnvironment, 'afterEach');
-  setHookFromEnvironment(hooks, testEnvironment, 'after');
+  setHookFromEnvironment(testEnvironment, hooks, 'before');
+  setHookFromEnvironment(testEnvironment, hooks, 'beforeEach');
+  setHookFromEnvironment(testEnvironment, hooks, 'afterEach');
+  setHookFromEnvironment(testEnvironment, hooks, 'after');
 
   const moduleFns = {
     before: makeSetHook(module, 'before'),
@@ -114,7 +135,7 @@ function processModule (name, options, scope, modifiers = {}) {
   config.currentModule = module;
 
   if (typeof scope === 'function') {
-    moduleStack.push(module);
+    config._moduleStack.push(module);
 
     try {
       const cbReturnValue = scope.call(module.testEnvironment, moduleFns);
@@ -126,7 +147,7 @@ function processModule (name, options, scope, modifiers = {}) {
       // we let this bubble up to global error handlers. But, not until after
       // we teardown internal state to ensure correct module nesting.
       // Ref https://github.com/qunitjs/qunit/issues/1478.
-      moduleStack.pop();
+      config._moduleStack.pop();
       config.currentModule = module.parentModule || prevModule;
     }
   }
diff --git a/src/core/on-uncaught-exception.js b/src/core/on-uncaught-exception.js
index cf6f89cd4..21f6744f2 100644
--- a/src/core/on-uncaught-exception.js
+++ b/src/core/on-uncaught-exception.js
@@ -1,5 +1,5 @@
 import config from './config.js';
-import { runSuite } from './module.js';
+import { globalSuiteReport } from './module.js';
 import { sourceFromStacktrace } from './stacktrace.js';
 import { errorString } from './utilities.js';
 import { emit } from './events.js';
@@ -42,7 +42,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.
-    runSuite.globalFailureCount++;
+    globalSuiteReport.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 fb7eceae3..e107bf3a1 100644
--- a/src/core/processing-queue.js
+++ b/src/core/processing-queue.js
@@ -2,7 +2,7 @@ import config from './config.js';
 import { generateHash, performance } from './utilities.js';
 import { runLoggingCallbacks } from './callbacks.js';
 import Promise from './promise.js';
-import { runSuite } from './module.js';
+import { globalSuiteReport } from './module.js';
 import onUncaughtException from './on-uncaught-exception.js';
 import { emit } from './events.js';
 import { setTimeout } from './globals.js';
@@ -67,7 +67,7 @@ class ProcessingQueue {
 
   /**
    * Process the first task on the taskQueue as a promise.
-   * Each task is a function added by Test#queue() in /src/test.js
+   * Each task is a function added by Test#queue() in test.js
    */
   processTaskQueue (start) {
     if (this.taskQueue.length && !config.blocking) {
@@ -187,7 +187,7 @@ class ProcessingQueue {
 
     this.finished = true;
 
-    const runEnd = runSuite.end(true);
+    const runEnd = globalSuiteReport.end(true);
     emit('runEnd', runEnd);
 
     runLoggingCallbacks('done', {
diff --git a/src/core/reporters/TapReporter.js b/src/core/reporters/TapReporter.js
index b819bd37f..19b620345 100644
--- a/src/core/reporters/TapReporter.js
+++ b/src/core/reporters/TapReporter.js
@@ -244,14 +244,14 @@ export default class TapReporter {
     }
   }
 
-  onRunEnd (runSuite) {
+  onRunEnd (runEnd) {
     this.ended = true;
 
-    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}`));
+    this.log(`1..${runEnd.testCounts.total}`);
+    this.log(`# pass ${runEnd.testCounts.passed}`);
+    this.log(kleur.yellow(`# skip ${runEnd.testCounts.skipped}`));
+    this.log(kleur.cyan(`# todo ${runEnd.testCounts.todo}`));
+    this.log(kleur.red(`# fail ${runEnd.testCounts.failed}`));
   }
 
   logAssertion (error, severity) {
diff --git a/src/core/reports/suite.js b/src/core/reports/suite.js
index ae461d3b1..f17c9d993 100644
--- a/src/core/reports/suite.js
+++ b/src/core/reports/suite.js
@@ -7,7 +7,7 @@ export default class SuiteReport {
 
     // When an "error" event is emitted from onUncaughtException(), the
     // "runEnd" event should report the status as failed. The "runEnd" event data
-    // is tracked through this property (via the "runSuite" instance).
+    // is tracked through this property (via the "globalSuiteReport" instance).
     this.globalFailureCount = 0;
 
     this.tests = [];
diff --git a/src/core/stacktrace.js b/src/core/stacktrace.js
index 588a8c5cb..67ce75218 100644
--- a/src/core/stacktrace.js
+++ b/src/core/stacktrace.js
@@ -2,8 +2,8 @@
 //
 // This should reduce a raw stack trace like this:
 //
-// > foo.broken()@/src/foo.js
-// > Bar@/src/bar.js
+// > foo.broken()@/example/foo.js
+// > Bar@/example/bar.js
 // > @/test/bar.test.js
 // > @/lib/qunit.js:500:12
 // > @/lib/qunit.js:100:28
@@ -13,8 +13,8 @@
 //
 // and shorten it to show up until the end of the user's bar.test.js code.
 //
-// > foo.broken()@/src/foo.js
-// > Bar@/src/bar.js
+// > foo.broken()@/example/foo.js
+// > Bar@/example/bar.js
 // > @/test/bar.test.js
 //
 // QUnit will obtain one example trace (once per process/pageload suffices),
diff --git a/src/core/start.js b/src/core/start.js
index ad15ed2f3..9d718f8b0 100644
--- a/src/core/start.js
+++ b/src/core/start.js
@@ -2,7 +2,7 @@ import { runLoggingCallbacks } from './callbacks.js';
 import config from './config.js';
 import { emit } from './events.js';
 import { window, document, setTimeout } from './globals.js';
-import { runSuite } from './module.js';
+import { globalSuiteReport } from './module.js';
 import Test from './test.js';
 import reporters from './reporters.js';
 import { performance } from './utilities.js';
@@ -14,7 +14,7 @@ function unblockAndAdvanceQueue () {
 
 // Inject the complete QUnit API for use by reporters
 export function createStartFunction (QUnit) {
-  function doBegin () {
+  function doStart () {
     if (config.started) {
       unblockAndAdvanceQueue();
       return;
@@ -39,14 +39,18 @@ export function createStartFunction (QUnit) {
     // Record the time of the test run's beginning
     config.started = performance.now();
 
-    // Delete the loose unnamed module if unused.
+    // Delete the unnamed module if no global tests were defined (see config.js)
     if (config.modules[0].name === '' && config.modules[0].tests.length === 0) {
       config.modules.shift();
     }
 
+    // Create a list of simplified and independent module descriptor objects for
+    // the QUnit.begin callbacks. This prevents plugins from relying on reading
+    // from (or writing!) to internal state.
     const modulesLog = [];
     for (let i = 0; i < config.modules.length; i++) {
-      // Don't expose the unnamed global test module to plugins.
+      // Always omit the unnamed module from the list of module names
+      // for UI plugins, even if there were glboal tests defined.
       if (config.modules[i].name !== '') {
         modulesLog.push({
           name: config.modules[i].name,
@@ -56,7 +60,7 @@ export function createStartFunction (QUnit) {
     }
 
     // The test run is officially beginning now
-    emit('runStart', runSuite.start(true));
+    emit('runStart', globalSuiteReport.start(true));
     runLoggingCallbacks('begin', {
       totalTests: Test.count,
       modules: modulesLog
@@ -82,15 +86,15 @@ export function createStartFunction (QUnit) {
       // still wait for DOM ready to ensure reliable integration of reporters.
       window.addEventListener('load', function () {
         setTimeout(function () {
-          doBegin();
+          doStart();
         });
       });
     } else if (setTimeout) {
       setTimeout(function () {
-        doBegin();
+        doStart();
       });
     } else {
-      doBegin();
+      doStart();
     }
   };
 }
diff --git a/src/core/test.js b/src/core/test.js
index 05cb638b0..d90816701 100644
--- a/src/core/test.js
+++ b/src/core/test.js
@@ -111,8 +111,8 @@ export default function Test (settings) {
 
 Test.count = 0;
 
-function getNotStartedModules (startModule) {
-  let module = startModule;
+function getModulesForStartEvent (startingModule) {
+  let module = startingModule;
   const modules = [];
 
   while (module && module.testsRun === 0) {
@@ -120,8 +120,8 @@ function getNotStartedModules (startModule) {
     module = module.parentModule;
   }
 
-  // The above push modules from the child to the parent
-  // return a reversed order with the top being the top most parent module
+  // The above starts from the child and moves up to the parent.
+  // Return this in reversed order, such that we start with top-most parent.
   return modules.reverse();
 }
 
@@ -136,7 +136,7 @@ Test.prototype = {
 
   before: function () {
     const module = this.module;
-    const notStartedModules = getNotStartedModules(module);
+    const notStartedModules = getModulesForStartEvent(module);
 
     // ensure the callbacks are executed serially for each module
     let moduleStartChain = Promise.resolve();
@@ -243,7 +243,7 @@ Test.prototype = {
         return;
       }
 
-      // The 'after' hook should only execute when there are not tests left and
+      // The 'after' hook should only execute when there are no tests left and
       // when the 'after' and 'finish' tasks are the only tasks left to process
       if (hookName === 'after' &&
         !lastTestWithinModuleExecuted(hookOwner) &&
@@ -417,39 +417,36 @@ 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 (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 && allTestsExecuted(parent)) {
-          completedModules.push(parent);
-          parent = parent.parentModule;
-        }
+      // Emit the `suiteEnd` event and `moduleDone` callbacks for modules
+      // that are completed as of now.
+      const completedModules = [];
+      let parent = module;
+      while (parent && allTestsExecuted(parent)) {
+        completedModules.push(parent);
+        parent = parent.parentModule;
+      }
 
-        let moduleDoneChain = Promise.resolve();
-        completedModules.forEach(completedModule => {
-          moduleDoneChain = moduleDoneChain.then(() => {
-            return logSuiteEnd(completedModule);
-          });
+      let moduleDoneChain = Promise.resolve();
+      completedModules.forEach(completedModule => {
+        moduleDoneChain = moduleDoneChain.then(() => {
+          return logSuiteEnd(completedModule);
         });
-        return moduleDoneChain;
-      }
+      });
+      return moduleDoneChain;
     }).then(function () {
       config.current = undefined;
     });
 
     function logSuiteEnd (module) {
-      // Reset `module.hooks` to ensure that anything referenced in these hooks
+      // Empty `module.hooks` to ensure that anything referenced in these hooks
       // has been released to be garbage collected. Descendant modules that were
       // entirely skipped, e.g. due to filtering, will never have this method
-      // called for them, but might have hooks with references pinning data in
-      // memory (even if the hooks weren't actually executed), so we reset the
+      // called for them, but might have hooks with references that hold data in
+      // memory (even if the hooks were never executed), so we empty the
       // hooks on all descendant modules here as well. This is safe because we
       // will never call this as long as any descendant modules still have tests
-      // to run. This also means that in multi-tiered nesting scenarios we might
-      // reset the hooks multiple times on some modules, but that's harmless.
+      // to run. This also means that for deeply nested modules, we might empty
+      // the hooks on completed child modules multiple times. That's harmless.
       const modules = [module];
       while (modules.length) {
         const nextModule = modules.shift();