Skip to content

Commit

Permalink
Core: Always define globalThis.QUnit
Browse files Browse the repository at this point in the history
== Background ==

In QUnit 2.x, we always set the QUnit global in browser contexts, but
outside browsers, we only set the global if CJS/AMD wasn't detected.
Except in the QUnit CLI, where we do set the global anyway, because
that's how most QUnit tests are written, incliuding for Node.js.
So really the only case where the QUnit global is missing is custom
test runners that:

* run in Node.js,
* and require/import qunit.js directly,
* and don't export it as a global.

I'm aware a growing proportion of developers import 'qunit' directly
in each test file for improved type support. That's a great way
to avoid needing to rely on globals. But, it's not a requirement,
and is not always an option, especially for simple no-build-step
and browser-facing projects.

== Why ==

1. Improve portability between test runners.

Remove the last edge case where the QUnit global can be undefined.
Make it QUnit's responsiblity to define this reliably, as indeed
it almost always already does. Remove this as undocumented requirement
for specific test runners to patch up on their end.

In light of Karma deprecation, and emergence of more general purpose
TAP runners, I'd like to remove this as factor that might make/break
QUnit support in one of those.

2. Prepare for native ESM build.

We don't natively support ESM exports yet, but once we do this will
become a problem. To prevent split-brain problems with mixed use
(e.g. in test registry and other state) standardise internally on
which ever globalThis.QUnit was defined first, and then reliably
export that to any importers.

Ref #1551.
  • Loading branch information
Krinkle committed Jun 24, 2024
1 parent 57a543b commit c78d475
Show file tree
Hide file tree
Showing 8 changed files with 71 additions and 69 deletions.
9 changes: 4 additions & 5 deletions build/tasks/test-on-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@ module.exports = function (grunt) {
grunt.log.ok(`Ran ${totals.files} files`);
}

// Refresh the QUnit global to be used in other tests
global.QUnit = requireFresh('../../qunit/qunit.js');
// Reset for anything after this task
delete globalThis.QUnit;
delete require.cache[require.resolve('../../qunit/qunit.js')];

done(!totals.failed);
});
Expand All @@ -31,11 +32,9 @@ module.exports = function (grunt) {
function runQUnit (file, runEnd) {
// Resolve current QUnit path and remove it from the require cache
// to avoid stacking the QUnit logs.
delete globalThis.QUnit;
let QUnit = requireFresh('../../qunit/qunit.js');

// Expose QUnit to the global scope to be seen on the other tests.
global.QUnit = QUnit;

QUnit.config.autostart = false;
requireFresh('../../' + file);
registerEvents(QUnit, file, runEnd);
Expand Down
9 changes: 2 additions & 7 deletions src/cli/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,6 @@ async function run (args, options) {
console.log(`Running tests with seed: ${QUnit.config.seed}`);
}

// TODO: Enable mode where QUnit is not auto-injected, but other setup is
// still done automatically.
global.QUnit = QUnit;

options.requires.forEach(requireFromCWD);

findReporter(options.reporter, QUnit.reporters).init(QUnit);
Expand Down Expand Up @@ -176,7 +172,7 @@ function abort (callback) {
process.off('exit', onExit);
running = false;

delete global.QUnit;
delete globalThis.QUnit;
QUnit = null;
if (callback) {
callback();
Expand All @@ -196,8 +192,7 @@ run.watch = function watch (args, options) {
const watch = require('node-watch');
const baseDir = process.cwd();

QUnit = requireQUnit();
global.QUnit = QUnit;
requireQUnit();
options.requires.forEach(requireFromCWD);

// Include TypeScript when in use (automatically via require.extensions),
Expand Down
3 changes: 0 additions & 3 deletions src/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import dump from './dump';
import { runSuite, module } from './module';
import Assert from './assert';
import Test, { test, pushFailure } from './test';
import exportQUnit from './export';
import reporters from './reporters';

import config from './core/config';
Expand Down Expand Up @@ -141,6 +140,4 @@ function begin () {
}).then(unblockAndAdvanceQueue);
}

exportQUnit(QUnit);

export default QUnit;
2 changes: 1 addition & 1 deletion src/core/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ readFlatPreconfig(globalThis);
// Apply a predefined QUnit.config object
//
// Ignore QUnit.config if it is a QUnit distribution instead of preconfig.
// That means QUnit was loaded twice! (Use the same approach as export.js)
// This should use the same heuristic as the export in qunit.js.
const preConfig = globalThis && globalThis.QUnit && !globalThis.QUnit.version && globalThis.QUnit.config;
if (preConfig) {
extend(config, preConfig);
Expand Down
40 changes: 0 additions & 40 deletions src/export.js

This file was deleted.

61 changes: 57 additions & 4 deletions src/qunit.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,60 @@
import QUnit from './core';
/* global module, exports */

import core from './core';
import { initBrowser } from './browser/browser-runner';
import { window, document } from './globals';
import { globalThis, window, document } from './globals';

/**
* Available exports:
*
* globalThis:
* - browser (globalThis === window)
* - Web Worker (globalThis === self)
* - Node.js
* - SpiderMonkey (mozjs)
* - Rhino 7.14+
* - any other embedded JS engine
*
* CommonJS module.exports (commonjs2):
* - Node.js
*
* CommonJS exports (commonjs, https://wiki.commonjs.org/wiki/Modules):
* - Rhino
*/

// If a real QUnit global was already defined, then replace our reference
// with that one, and export that instead. Skip initBrowser() to avoid
// doubling the user interface.
//
// Since QUnit 3.0, we no longer throw an error if QUnit is loaded twice.
// This enables mixed use of ESM import and CJS require() in a project,
// without split-brain problems for defining tests, setting config, registering
// reporters, etc.
//
// Note that a placeholder QUnit global may exist for preconfiguration.
// Such placeholder is recognised by not having QUnit.version (it should
// only contain QUnit.config), and will be upgraded to the real QUnit.
let QUnit;
if (globalThis.QUnit && globalThis.QUnit.version) {
QUnit = globalThis.QUnit;
} else {
QUnit = core;
globalThis.QUnit = QUnit;

if (window && document) {
initBrowser(QUnit, window, document);
}
}

// For Node.js
if (typeof module !== 'undefined' && module && module.exports) {
module.exports = QUnit;

// For consistency with CommonJS environments' exports
module.exports.QUnit = QUnit;
}

if (window && document) {
initBrowser(QUnit, window, document);
// For CommonJS with exports, but without module.exports, like Rhino
if (typeof exports !== 'undefined' && exports) {
exports.QUnit = QUnit;
}
2 changes: 1 addition & 1 deletion test/benchmark/index-node.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* eslint-env node */

global.QUnit = require('qunit');
require('qunit');

if (process.argv[2] === 'fast-deep-equal') {
const fde = require('fast-deep-equal/es6');
Expand Down
14 changes: 6 additions & 8 deletions test/overload.html
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,15 @@
};
</script>
<script src="../qunit/qunit.js"></script>
<!-- Load twice -->
<script>
var _qunitFirst = window.QUnit;
</script>
<script src="../qunit/qunit.js"></script>
<script>
QUnit.module('overload', function () {
QUnit.test('detect', function (assert) {
assert.true(
// In Firefox the string starts with "Error: "
// In Chrome the string starts with "Uncaught Error: "
_globalError.includes('QUnit has already been defined.'),
'error is thrown if QUnit global was overloaded'
);
QUnit.test('verify', function (assert) {
assert.strictEqual(_globalError, undefined, 'no error');
assert.strictEqual(QUnit, _qunitFirst, 'single identity');
});
});
</script>
Expand Down

0 comments on commit c78d475

Please sign in to comment.