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

@@ -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 () {

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;

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 = {

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;
     }
   }

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);

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', {

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) {

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 = [];

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),