Skip to content

implementation of global setup/teardown; closes #4308 - #4360

Merged
boneskull merged 2 commits into
masterfrom
boneskull/issue/4308-global-setup-teardown
Sep 8, 2020
Merged

boneskull merged 2 commits into
masterfrom
boneskull/issue/4308-global-setup-teardown

Conversation

@boneskull

@boneskull boneskull commented Jul 1, 2020 •

Copy link
Copy Markdown
Member

This PR adds mochaGlobalSetup and mochaGlobalTeardown fixtures, loaded like mochaHooks via --require. the function(s) defined here are guaranteed to run once and only once, in serial, parallel, serial-watch or parallel-watch modes. this should be the preferred way of running global setup/teardown code, and I'll need to update the docs accordingly. the setup and teardown functions will share a single context object, and will only run in the main process.

more:

  • adds a generic "plugin loader" which handles root hooks, global setup/teardown, future. these "plugin definitions" (of which there are three: root hooks, global setup, and global teardown) adhere to a rough "interface" at this point, but are not entirely self-contained--functionality is still tightly coupled to the Mocha class, which needs instance methods to support each. I'm choosing to defer any decisions on this design for now
  • renamed validatePlugin to validateLegacyPlugin. reporters/interfaces could/should eventually be refactored to use the same system. this would make it less awkward to define a custom reporter, for instance.
  • soft-deprecates errors.createPluginError; directs consumers to use errors.createLegacyPluginError instead

@boneskull boneskull added type: feature enhancement proposal semver-minor implementation requires increase of "minor" version number; "features" labels Jul 1, 2020
@boneskull boneskull self-assigned this Jul 1, 2020
@boneskull

Copy link
Copy Markdown
Member Author

this will get rebased onto master once #4293 lands. just can't be bothered to write ES5

@boneskull

boneskull commented Jul 1, 2020 •

Copy link
Copy Markdown
Member Author

the design here is up for discussion btw. we have a new file, lib/plugin.js which knows about "the special stuff you can export from a --require'd file" (e.g., mochaHooks, mochaGlobalSetup, etc.) and has validation/processing functionality thrown in there. maybe each "special thing" should have its own module. I dunno.

was thinking of taking it a step further and moving all of it into some sort of system that, say, uses event listeners to define functionality. so there'd be some implementation of mochaGlobalSetup, mochaGlobalTeardown, and mochaHooks which would be completely decoupled from lib/mocha.js, lib/runner.js, etc. so the implementation for global setup & teardown, which currently lives in Runner#run, could just listen for the EVENT_RUN_BEGIN event and then handle its business.

@boneskull
boneskull force-pushed the boneskull/issue/4308-global-setup-teardown branch 3 times, most recently from fc0e78f to 8b38307 Compare July 7, 2020 23:30
Comment thread lib/cli/run-helpers.js
const acc = [];
for (const mod of requires) {
const pluginLoader = PluginLoader.create();
for await (const mod of requires) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this may not be strictly necessary, but I think it's going to help reason about the order in which requires are loaded.

Comment thread lib/plugin.js Outdated
const PLUGIN_NAMES = new Set(Object.values(constants));

exports.PluginLoader = class PluginLoader {
constructor({

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overriding the defaults here is just for testability at this point.

Comment thread lib/plugin.js Outdated
}

async finalize() {
const mochaHooks = this.pluginMap.get(PLUGIN_ROOT_HOOKS);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should add this in a code comment, but the idea here is that we call load() on all required modules, which fishes the special named exports out of those modules, and then calling finalize() aggregates/flattens everything into a single data structure, which is suitable for passing into Mocha constructor

Comment thread lib/plugin.js Outdated
}
};

const PluginValidators = {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dunno, these are some validator functions to check that the named exports e.g. mochaHooks are of the correct shape. the createFunctionArrayValidator one above looks for a function or array thereof. if we're going to be in the business of validating a bunch of code this way, maybe we should use a suitable library (e.g., ajv or joi or something) instead of this.

Comment thread lib/plugin.js Outdated
* @private
* @returns {MochaRootHookObject}
*/
const aggregateRootHooks = (exports.aggregateRootHooks = async (

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved from run-helpers module

Comment thread lib/runner.js Resolved
@boneskull

Copy link
Copy Markdown
Member Author

What I mean by "reporters and interfaces should use this plugin system" above is that it's currently awkward to implement a third-party reporter or interface. You have to essentially set properties on Mocha.interfaces or Mocha.reporters. This new system could detect the presence of a reporter/interface in a --require'd file via named exports and do the plumbing for you. We'd also have a chance, if e.g., --require my-reporter was used, to enable the reporter automatically if no other --reporter was given (and this would need to be limited to a single reporter, since we don't have multiple reporter support).

@boneskull

Copy link
Copy Markdown
Member Author

rebasing onto branch boneskull/rollup-umd

@boneskull
boneskull force-pushed the boneskull/issue/4308-global-setup-teardown branch 2 times, most recently from 34e9a2b to 20dd278 Compare July 16, 2020 00:49
@coveralls

coveralls commented Jul 16, 2020 •

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.1%) to 93.959% when pulling 91eaaf0 on boneskull/issue/4308-global-setup-teardown into c1db2a6 on master.

@boneskull
boneskull force-pushed the boneskull/issue/4308-global-setup-teardown branch from fae370f to c8c81fa Compare July 17, 2020 21:46
@boneskull

Copy link
Copy Markdown
Member Author

hey, green build finally.

looks like it needs some more tests though.

@boneskull
boneskull force-pushed the boneskull/issue/4308-global-setup-teardown branch from 06e3d33 to 699016f Compare July 28, 2020 21:23
@boneskull
boneskull marked this pull request as ready for review July 28, 2020 21:23
@boneskull boneskull changed the title WIP: initial implementation of global setup/teardown; see #4308 implementation of global setup/teardown; closes #4308 Jul 28, 2020
Comment thread lib/cli/run-helpers.js
throw createUnsupportedError(
'mochaHooks must be an object or a function returning (or fulfilling with) an object'
);
if (requiredModule && typeof requiredModule === 'object') {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the logic here is "look in the module we've loaded via --require for any known named exports and figure out what to do with them". since these could be multiple and spread out over several files, on L102 below, we call pluginLoader.finalize() which allows aggregation of these into a single object, suitable for passing into the Mocha constructor.

for example, we might have mochaHooks in a.js and mochaHooks in b.js; the pluginLoader.load() call will collect these, and we mash them all together into an object (containing props for each hook type, and arrays of hook callback functions) in pluginLoader.finalize().

Comment thread lib/cli/run.js
const plugins = await handleRequires(argv.require);
validateLegacyPlugin(argv, 'reporter', Mocha.reporters);
validateLegacyPlugin(argv, 'ui', Mocha.interfaces);
Object.assign(argv, plugins);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of just a rootHooks prop on the options object for Mocha, the plugins object will have several properties which we want to merge in.

Comment thread test/unit/plugin-loader.spec.js Resolved
@boneskull
boneskull force-pushed the boneskull/issue/4308-global-setup-teardown branch 3 times, most recently from bab73dd to fadff9a Compare August 25, 2020 21:30
@boneskull

Copy link
Copy Markdown
Member Author

this has gotten too unwieldy and I'm going to try to break it up

Comment thread lib/runner.js
* @param {Object} [opts] - Options for {@link Runner#run}
* @returns {Promise<number>} Failure count
*/
Runner.prototype.runAsync = async function runAsync(opts = {}) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this! 😍

@nicojs nicojs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cleans up the code nicely and is an excellent feature.

@boneskull
boneskull force-pushed the boneskull/issue/4308-global-setup-teardown branch from e677219 to ee72712 Compare September 2, 2020 19:14
@boneskull
boneskull force-pushed the boneskull/issue/4308-global-setup-teardown branch from ee72712 to eeb7dae Compare September 2, 2020 19:22
@boneskull

Copy link
Copy Markdown
Member Author

I am not going to attempt to break this up, because that's a pain in the ass. suffice to say there are things in here that could technically be in separate PRs. I am sorry.

This avoids a circular dependency which arises when Mocha is bundled.

These are private APIs.
@boneskull
boneskull force-pushed the boneskull/issue/4308-global-setup-teardown branch from 94a2055 to 91eaaf0 Compare September 2, 2020 23:10
@boneskull

Copy link
Copy Markdown
Member Author

I'm going to assume we're OK with moving deprecate() and warn() into lib/errors from lib/utils. It is not ideal, but it is problematic to attempt to bundle a cyclic dependency, as utils needed errors and vice-versa.

@boneskull boneskull added this to the v8.2.0 milestone Sep 2, 2020
@boneskull

Copy link
Copy Markdown
Member Author

when AppVeyor passes I'm going to merge this, finally.

@boneskull
boneskull merged commit 12db9db into master Sep 8, 2020
@boneskull

Copy link
Copy Markdown
Member Author

I have some ideas around documenting this further, but I can do it in a subsequent PR.

@wbt

wbt commented Jan 10, 2022

Copy link
Copy Markdown

FYI as feedback to help you calibrate semver choices for the future, this was a breaking change for reasons summarized here, and should've gone out in 9.0.0.
Thanks for all your work on this project!

lennonnikolas pushed a commit to lennonnikolas/mocha that referenced this pull request Jan 24, 2026
…#4360)

* implementation of global setup/teardown; closes mochajs#4308

* move deprecate() and warn() from utils into errors module

This avoids a circular dependency which arises when Mocha is bundled.

These are private APIs.
voxpelli added a commit that referenced this pull request Mar 31, 2026
Deprecated for 6 years, since: #4360
voxpelli added a commit that referenced this pull request Mar 31, 2026
- `warn()` is no longer used anywhere
- `createInvalidPluginError()` is no longer used anywhere and has been deprecated since #4360, for 6 years now
- `deprecate()` was only used by `createInvalidPluginError()`
- `emitWarning()` was only used by `warn()` and `deprecate()` and contained an unnecessary polyfill for `process.emitWarning`, a function that has existed since Node 6.0.0 with us now requiring at least Node 20.19.0

Ref: #5357 (v12 Release Plan)
voxpelli added a commit that referenced this pull request Mar 31, 2026
- `warn()` is no longer used anywhere
- `createInvalidPluginError()` is no longer used anywhere and has been deprecated since #4360, for 6 years now
- `deprecate()` was only used by `createInvalidPluginError()`
- `emitWarning()` was only used by `warn()` and `deprecate()` and contained an unnecessary polyfill for `process.emitWarning`, a function that has existed since Node 6.0.0 with us now requiring at least Node 20.19.0

Ref: #5357 (v12 Release Plan)
voxpelli added a commit that referenced this pull request Mar 31, 2026
- `warn()` is no longer used anywhere
- `createInvalidPluginError()` is no longer used anywhere and has been deprecated since #4360, for 6 years now
- `deprecate()` was only used by `createInvalidPluginError()`
- `emitWarning()` was only used by `warn()` and `deprecate()` and contained an unnecessary polyfill for `process.emitWarning`, a function that has existed since Node 6.0.0 with us now requiring at least Node 20.19.0

Ref: #5357 (v12 Release Plan)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-minor implementation requires increase of "minor" version number; "features" type: feature enhancement proposal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants