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

boneskull · 2020-07-01T23:18:48Z

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 · 2020-07-01T23:20:12Z

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

boneskull · 2020-07-01T23:28:08Z

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 · 2020-07-07T23:38:15Z

lib/cli/run-helpers.js

-  const acc = [];
-  for (const mod of requires) {
+  const pluginLoader = PluginLoader.create();
+  for await (const mod of requires) {


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

boneskull · 2020-07-07T23:40:00Z

lib/plugin.js

+const PLUGIN_NAMES = new Set(Object.values(constants));
+
+exports.PluginLoader = class PluginLoader {
+  constructor({


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

boneskull · 2020-07-07T23:41:19Z

lib/plugin.js

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


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

boneskull · 2020-07-07T23:45:51Z

lib/plugin.js

+  }
+};
+
+const PluginValidators = {


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.

boneskull · 2020-07-07T23:47:59Z

lib/plugin.js

+ * @private
+ * @returns {MochaRootHookObject}
+ */
+const aggregateRootHooks = (exports.aggregateRootHooks = async (


moved from run-helpers module

lib/runner.js

boneskull · 2020-07-13T19:38:05Z

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 · 2020-07-14T19:24:04Z

rebasing onto branch boneskull/rollup-umd

coveralls · 2020-07-16T01:14:52Z

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

boneskull · 2020-07-17T23:41:16Z

hey, green build finally.

looks like it needs some more tests though.

boneskull · 2020-07-28T21:34:10Z

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


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().

boneskull · 2020-07-28T21:35:39Z

lib/cli/run.js

+        const plugins = await handleRequires(argv.require);
+        validateLegacyPlugin(argv, 'reporter', Mocha.reporters);
+        validateLegacyPlugin(argv, 'ui', Mocha.interfaces);
+        Object.assign(argv, plugins);


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.

boneskull · 2020-08-01T22:30:56Z

Thanks @craigtaub. Yeah there's really no reasonable way to share a file, db handle, socket etc across spawned worker processes. Each worker process would have its own handle, and this would be suitable for root hook plugins instead of these globals.

You can share state between global setup and teardown, but the suites, hooks, and tests themselves can not access that state.

"Fixture", I think, is not inappropriate (see wiki). we're using it to mean roughly the same thing in our test "fixture" files--it's some environment or a context we test in.

boneskull · 2020-08-17T21:04:54Z

@craigtaub Can you take a look, specifically, at the changes to the "run cycle overview" in docs/index.md? It was out-of-date with respect to parallel mode, root hooks, and this PR (and I felt like it needed further expansion)

boneskull · 2020-08-17T21:05:58Z

I'm referencing version 9 in the docs here, but now I can't remember why. I thought I was breaking something, but maybe I'm not. Need to double-check.

docs/index.md

craigtaub · 2020-08-18T21:33:22Z

docs/index.md

+
+In a browser, test files are loaded by `<script>` tags, and calling `mocha.run()` begins at step 9 [below](#serial-mode).
+
+### Serial Mode


Worth splitting into parsing and execution phases?

I'm not convinced I can make it any clearer by drawing a line there. do you have any specific suggestions on how to explain it clearly?

Not for clarity, just to codify our definition of where these phases start and what's included. Was just imagining a simple divide I.e.

--- Parsing phase 1. .... ... 4. ... --- --- Execution phase 5. .... ... ----

But they are mainly useful for internal purposes anyway so happy to drop. Leave with you.

hmmm......

I'm not sure I know what the definition is. certainly, we run our suite callbacks (I think this is what you're referring to with "parsing phase") in their entirety before our hook and test callbacks. In the changes I've made, step 8. refers to this:

The (default) bdd interface loads the test files in no particular order, which are given an interface-specific global context (this is how, e.g., describe() ends up as a global in a test file)

When a test file is loaded, Mocha executes all of its suites and finds--but does not execute--any hooks and tests therein.

Top-level hooks, tests and suites are all made members of an "invisible" root suite; there is only one root suite for the entire process

after that, we run global setup fixtures, which really has nothing to do with the suites. after that, we run the hook/test callbacks. so to me anyway, there's no clear "logical grouping" of tasks, like you've written above. it's just a sequence of steps.

now... we could make an argument that global setup fixtures should actually run before test files are loaded. that might make more sense, but it would mean that we'd have to expose APIs for programmatic users to trigger setup and teardown execution manually. maybe someone wants this, but a) you can toggle the behavior on and off anyway via the public Mocha#enableGlobalSetup() and Mocha#enableGlobalTeardown() methods, and b) it's not very friendly to make programmatic users go thru extra hoops to get standard behavior.

however, this brings up the point that Mocha#runGlobalSetup() and Mocha#runGlobalTeardown() should be public, if a programmatic users wants to fiddle with when these run:

const mocha = new Mocha(); mocha.addFile('/some/file.js'); mocha.enableGlobalSetup(false); // run the global setup early, before mocha.run() is called await mocha.runGlobalSetup(); // do something else here mocha.run();

Hmm interesting.
Sorry I should have said, my understanding is the parsing phase is just options and validation. So it's very short. I think all the loading of files and running comes under execution phase. But that's just my own mental model.

docs/index.md

craigtaub · 2020-08-18T21:42:56Z

Nice, "run cycle overview" is much improved.

test/unit/plugin-loader.spec.js

boneskull · 2020-08-26T00:16:56Z

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

nicojs · 2020-08-28T06:12:50Z

lib/runner.js

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


I like this! 😍

nicojs

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

boneskull · 2020-09-02T19:28:57Z

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 · 2020-09-02T23:50:22Z

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 · 2020-09-02T23:59:10Z

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

boneskull · 2020-09-08T19:58:05Z

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

wbt · 2022-01-10T21:41:53Z

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!

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

boneskull self-assigned this Jul 1, 2020

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

boneskull commented Jul 7, 2020

View reviewed changes

lib/runner.js Outdated Show resolved Hide resolved

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

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

boneskull force-pushed the boneskull/issue/4308-global-setup-teardown branch from 06e3d33 to 699016f Compare July 28, 2020 21:23

boneskull marked this pull request as ready for review July 28, 2020 21:23

boneskull requested review from nicojs, craigtaub, Munter and outsideris July 28, 2020 21:23

boneskull changed the title ~~WIP: initial implementation of global setup/teardown; see #4308~~ implementation of global setup/teardown; closes #4308 Jul 28, 2020

boneskull commented Jul 28, 2020

View reviewed changes

boneskull force-pushed the boneskull/issue/4308-global-setup-teardown branch from 9df3cb8 to a25fa19 Compare August 14, 2020 20:54

craigtaub reviewed Aug 18, 2020

View reviewed changes

test/unit/plugin-loader.spec.js Outdated Show resolved Hide resolved

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

nicojs reviewed Aug 28, 2020

View reviewed changes

nicojs approved these changes Aug 28, 2020

View reviewed changes

nicojs mentioned this pull request Aug 28, 2020

❓A global before in parallel mode. #4424

Closed

craigtaub approved these changes Aug 30, 2020

View reviewed changes

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

implementation of global setup/teardown; closes #4308

eeb7dae

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

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

91eaaf0

This avoids a circular dependency which arises when Mocha is bundled. These are private APIs.

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

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

boneskull merged commit 12db9db into master Sep 8, 2020

boneskull deleted the boneskull/issue/4308-global-setup-teardown branch September 8, 2020 19:58

BeeMargarida mentioned this pull request Oct 19, 2020

Support for mocha 8.2.0 validateLegacyPlugin change sysgears/mochapack#86

Merged

wbt mentioned this pull request Jan 10, 2022

Pin mocha to v8 as short-term test failure fix winstonjs/winston#2017

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

boneskull commented Jul 1, 2020 •

edited

boneskull commented Jul 1, 2020

boneskull commented Jul 1, 2020 •

edited

boneskull Jul 7, 2020

boneskull Jul 7, 2020

boneskull Jul 7, 2020

boneskull Jul 7, 2020

boneskull Jul 7, 2020

boneskull commented Jul 13, 2020

boneskull commented Jul 14, 2020

coveralls commented Jul 16, 2020 •

edited

boneskull commented Jul 17, 2020

boneskull Jul 28, 2020

boneskull Jul 28, 2020

boneskull commented Aug 1, 2020

boneskull commented Aug 17, 2020

boneskull commented Aug 17, 2020

craigtaub Aug 18, 2020

boneskull Aug 18, 2020

craigtaub Aug 19, 2020 •

edited

boneskull Aug 25, 2020

craigtaub Aug 30, 2020

craigtaub commented Aug 18, 2020 •

edited

boneskull commented Aug 26, 2020

nicojs Aug 28, 2020

nicojs left a comment

boneskull commented Sep 2, 2020

boneskull commented Sep 2, 2020

boneskull commented Sep 2, 2020

boneskull commented Sep 8, 2020

wbt commented Jan 10, 2022


		In a browser, test files are loaded by `<script>` tags, and calling `mocha.run()` begins at step 9 [below](#serial-mode).

		### Serial Mode

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

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

Conversation

boneskull commented Jul 1, 2020 • edited

boneskull commented Jul 1, 2020

boneskull commented Jul 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boneskull commented Jul 13, 2020

boneskull commented Jul 14, 2020

coveralls commented Jul 16, 2020 • edited

boneskull commented Jul 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boneskull commented Aug 1, 2020

boneskull commented Aug 17, 2020

boneskull commented Aug 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craigtaub Aug 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craigtaub commented Aug 18, 2020 • edited

boneskull commented Aug 26, 2020

Choose a reason for hiding this comment

nicojs left a comment

Choose a reason for hiding this comment

boneskull commented Sep 2, 2020

boneskull commented Sep 2, 2020

boneskull commented Sep 2, 2020

boneskull commented Sep 8, 2020

wbt commented Jan 10, 2022

boneskull commented Jul 1, 2020 •

edited

boneskull commented Jul 1, 2020 •

edited

coveralls commented Jul 16, 2020 •

edited

craigtaub Aug 19, 2020 •

edited

craigtaub commented Aug 18, 2020 •

edited