run global setup before test files load; closes #4508 #4511
Conversation
BREAKING CHANGE `Mocha#run()` was orchestrating if and when to run global setup/teardown fixtures, but `Mocha#run()` must be called after test files have been loaded. This is a problem, because: 1. `--delay` may expect something created in a fixture to be accessible, but it won't be 2. Any future support for async suites is similarly negatively impacted 3. It was inconsistent between "watch" and "single run" mode; "watch" mode already has this behavior! This change causes setup fixtures to run _before_ any test files have been loaded. In Node.js, this happens in `lib/cli/run-helpers`. - Added two functions to `Mocha`; `Mocha#globalSetupEnabled()` and `Mocha#globalTeardownEnabled()` which both return booleans - Correct order of operations is asserted in `test/integration/global-fixtures.spec.js` - Removed low-value (and newly broken) unit tests where `Mocha#run` called `runGlobalSetup`/`runGlobalTeardown` - Add a note to `browser-entry.js` about global fixtures This is breaking because: 1. Browser users now must run setup/teardown fixtures manually. 2. Somebody's test harness might expect test files to be loaded (and suites run) _before_ global setup fixtures execute.
boneskull
added
type: feature
|
@boneskull Is there any chance of a merge in the near future? This issue is really a pain point for us. |
|
@juergba Could you take a look into this? It's still a pain |
|
What is the status of this one? It would be nice if it gets prioritized |
|
Can you please commit this |
There was a problem hiding this comment.
Looks good to me. Shoutout to 2020-era @boneskull for writing clear, maintainable code. 😄
Since this is a semver-major we'll definitely want reviews from the rest of @mochajs/maintenance-crew. Note that per #5027, even when we have approvals, we're still ramping up into bigger changes. This will take a while longer to get merged.
JoshuaKGoldberg
added
the
status: blocked
| await mocha.runGlobalTeardown(context); | ||
| } | ||
| done(...args); | ||
| }); |
There was a problem hiding this comment.
Aside: https://github.com/mochajs/mocha/pull/4771/files#r1511173111 also talks about refactoring these shared wrappers. If I were to end up taking ownership over this PR then I'd probably apply that refactor. Food for thought.
|
So glad to see this PR being resurrected 🎉 this bug is a pain in the ass 😄 |
There was a problem hiding this comment.
Pull Request Overview
This PR changes Mocha’s execution flow so that global setup fixtures run before loading test files (both in serial and parallel CLI modes), introduces new public flags to query fixture enablement, and updates related integration tests and documentation.
- Introduced
Mocha#globalSetupEnabled()andMocha#globalTeardownEnabled()methods - Moved fixture orchestration into
lib/cli/run-helpers.jsto run setup → load → tests → teardown - Updated integration tests to verify fixture ordering and added a sample fixture file
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/unit/mocha.spec.js | Removed outdated unit tests around Mocha.run fixture behavior |
| test/integration/plugins/global-fixtures.spec.js | Switched to new fixture path constant and tightened expectations to include the test fixture load order |
| test/integration/fixtures/plugins/global-fixtures/global.fixture.js | Added a sample test fixture that logs loading and defines a passing suite |
| lib/plugin-loader.js | Swapped Array.from for spread syntax when iterating export names |
| lib/mocha.js | Removed built-in fixture calls from Mocha.run and added two new public enablement methods |
| lib/cli/run-helpers.js | Updated single-run and parallel-run helpers to run global setup/teardown around file loading |
| browser-entry.js | Noted that global fixtures must be run manually in the browser and linked APIs |
Comments suppressed due to low confidence (2)
test/unit/mocha.spec.js:854
- Add unit tests for the newly introduced
Mocha#globalSetupEnabled()andMocha#globalTeardownEnabled()methods to validate their behavior based on variousenableGlobalSetupandenableGlobalTeardownoption states.
describe('Mocha', function() {
lib/plugin-loader.js:225
- [nitpick] Consider using
Array.from(this.knownExportNames)instead of the spread operator for consistency with other parts of the codebase that useArray.from()to convert sets to arrays.
return [...this.knownExportNames].reduce(
| * @public | ||
| * @returns {boolean} | ||
| */ | ||
| Mocha.prototype.globalTeardownEnabled = function globalSetupEnabled() { |
There was a problem hiding this comment.
The function assigned to globalTeardownEnabled is incorrectly named globalSetupEnabled; this misnaming could lead to confusion in stack traces and debugging. Rename the function to globalTeardownEnabled.
| Mocha.prototype.globalTeardownEnabled = function globalSetupEnabled() { | |
| Mocha.prototype.globalTeardownEnabled = function globalTeardownEnabled() { |
Copilot uses AI. Check for mistakes.
BREAKING CHANGE
Mocha#run()was orchestrating if and when to run global setup/teardown fixtures, butMocha#run()must be called after test files have been loaded. This is a problem, because:--delaymay expect something created in a fixture to be accessible, but it won't beThis change causes setup fixtures to run before any test files have been loaded. In Node.js, this happens in
lib/cli/run-helpers.Mocha;Mocha#globalSetupEnabled()andMocha#globalTeardownEnabled()which both return booleanstest/integration/global-fixtures.spec.jsMocha#runcalledrunGlobalSetup/runGlobalTeardownbrowser-entry.jsabout global fixturesThis is breaking because:
We should probably merge this, even though I'm loathe to break it--the original design will cause major headaches in future development. My bad!
Closes #4508