add Root Hook Plugins

[boneskull]

(documentation will be in another PR; this depends on #4237)

Adds "root hook plugins", a system to define root hooks via files loaded with --require.

This enables root hooks to work in parallel mode. Because parallel mode runs files in a non-deterministic order, and files do not share a Mocha instance, it is not possible to share these hooks with other test files. This change also works well with third-party libraries for Mocha which need the behavior; these can now be trivially consumed by adding --require or require: 'some-library' in Mocha's config file.

The way it works is:

1. When a file is loaded via --require, we check to see if that file exports a property named mochaHooks (can be multiple files).
2. If it does, we save a reference to the property.
3. After Yargs' validation phase, we use async middleware to execute root hook plugin functions--or if they are objects, just collect them--and we flatten all hooks found into four buckets corresponding to the four hook types.
4. Once Mocha is instantiated, if it is given a rootHooks option, those hooks are applied to the root suite.

This works with parallel tests because we can save a reference to the flattened hooks in each worker process, and a new Mocha instance is created with them for each test file.

---

Tangential:

• Because a root hook plugin can be defined as an async function, I noticed that utils.type() does not return function for async functions; it returns asyncfunction. I've added a (Node-specific, for now) test for this.
• handleRequires is now async, since it will need to be anyway to support ESM and calls to import().
• fixed incorrect call to fs.existsSync()

Ref: #4198

[coveralls]

Coverage increased (+0.05%) to 93.275% when pulling 0cdb978 on boneskull/issue/2839-root-hook-plugins into 68eec9e on master.

[juergba]

4. Once Mocha is instantiated, if it is given a rootHooks option, those hooks are applied to the root suite.

We have one Mocha instance and one root suite per each child-process, right?
Isn't it possible to use one mocha over all child-processes?

[juergba]

Users may want to run root hooks only once for all child-processes (not for each). It has to be in the first file running. Is there a way to achieve this?

Does it work for async hooks with callbacks? Because of the hook.async property.

[juergba]

Do we need this typedef for typescript? Because this file is part of our bundled browser mocha.js.

[boneskull]

I can move it elsewhere. It's not intended to be in the public API at this point anyway. VSCode's TS language server understands it (somewhat). I just did this instead of doing it inline elsewhere, as describing more than the most trivial values in a @param is awkward.

Given that this functionality is intended to be specific to Node.js, maybe I should move it out? That would be poor encapsulation, though. I really dislike the points in the codebase where object A is mucking with the internals of object B, and that's exactly what would have to happen if Mocha#rootHooks() were moved elsewhere; we'd have a Mocha instance, then some other method would need to reach into Mocha#suite and call methods there.

While this is intended to be used with Node.js, it could be used in the browser. There's no reason why a browser can't call Mocha#rootHooks(), but the consumer would need to pass an object adhering to the MochaRootHookObject shape, which is awkward.

Another way may be to do away with MochaRootHookObject entirely, and instead expose four new chainable methods: Mocha#rootBeforeAll(), Mocha#rootBeforeEach(), Mocha#rootAfterAll(), and Mocha#rootAfterEach(). Each would accept a single hook function. This would probably be a more ergonomic API for a consumer (which I had not originally considered).

What do you think?

[boneskull]

I mean, there's no reason why the browser couldn't consume rootHooks(). I think it's probably the right place for it for now.

[juergba]

I haven't understood this part. Why you pass an object, not a function.
Suite.prototype.beforeAll = function(title, fn) expects a function, but here it gets an object.
If you run those hooks with Runnable#run, how can this work? The hook has already been executed and contains the return value, not the function anymore?

[boneskull]

hooks is a collection of all four types of hook, and has properties beforeAll, beforeEach, afterAll, afterEach. At this point in the code, if any of these property is truthy, they are expected to be arrays of functions. I think I could be more specific about the shape of the object; before we reach this point, loadHooks() (in run-helpers.js, iirc) does some pre-processing.

Below, we loop thru the properties (again, which are arrays of functions) and assign them to the root suite via the appropriate method in Suite.

[boneskull]

@juergba

Once Mocha is instantiated, if it is given a rootHooks option, those hooks are applied to the root suite.

We have one Mocha instance and one root suite per each child-process, right?

This is correct.

Isn't it possible to use one mocha over all child-processes?

It isn't possible to share a single mocha instance across multiple child processes, no. There's no way to share an object reference via IPC (afaik).

It's not impossible for a single child process to reuse a Mocha instance. In fact, PR #4245 could take advantage of @nicojs' PR #4234 to do so. I haven't looked closely at that PR, so I'm unsure if the root suite would be reset or not. If we were to use that, root beforeAll and afterAll hooks would only be called once per child process. In the current state of #4245, these are called for each file.

I realize this doesn't work for people who need to run, e.g., beforeAll once and only once. If beforeAll is modifying the test context, there's no way to share object references between child
processes, so it would have to be run for each child process anyway. If it's doing some sort of I/O and must be run once, then presently users should not use parallel mode for now. Bikeshedding, the way forward for this use case would be support of async ESM via --require and/or adding another convention, e.g., exports.mochaSetup = async () => { /* .. */ }, which would be called in the main process, but would necessarily run before the instantiation of Mocha.

[boneskull]

somehow accidentally closed this

[boneskull]

@juergba waiting for response from you in the comments, and also pls @mochajs/core take a look.

I want to make sure

1. If we add an API like this, we are comfortable living with it for awhile, and
2. We understand why it should be a thing
3. We understand the limitations (there's currently no way, when running tests in parallel, to run a root "before all"/"after all" hook once and only once; they happen once per test file)
4. I'm not sure if we should wait on the other PR which allows better "resetting" of Mocha; this would allow us to reduce these hooks to run once per child process instead of once per test file

[craigtaub]

What about adding a new argument --setup or --setup-file or --global-setup?

Sounds sensible to me. Could see this adding to the user confusion around whether to use file or require. Hopefully no issues running once per child process + still use rootHooks.

Assuming waiting for more feedback before adding docs, which is fine.

[boneskull]

@craigtaub docs are in #4246

[boneskull]

re: --setup. I'm not entirely opposed, but we run the risk of just adding more and more flags if we want to support some other kind of plugin.

this is why I wanted to use --require + a convention. this would position mocha to define more such conventions in the future to expose more kinds of plugins.

if we use --setup, it can only really be used for this one thing, and if we want to do something else, then the precedent is to create yet another option.

things that could be supported via --require + convention include, but are not limited to:

1. easier interface definition
2. easier reporter definition
3. custom Runners (e.g., a worker-thread based runner)
4. custom stats collecting
5. snapshot test hooks
6. tighter integration with assertion & mocking libraries

Could see this adding to the user confusion around whether to use file or require

If we merged this, I think we would want to be explicit: do not use --file for root-level hooks. --file is intended to describe a specific order in which test files run, not for root hook definition.

[boneskull]

I note that you could do this, with minimal changes:

// .mocharc.js

module.exports = {
  require: [
    'source-map-support',
    'something-else',
    {
      mochaHooks: {
        beforeEach() {
          this.foo()
        }
      }
    }
  ]
};

[boneskull]

@nicojs @craigtaub What do you think of my argument above? Convinced or no?

[nicojs]

Sure, that makes sense. Didn't know about other plugins yet. If you need a plugin system, this makes sense.

module​.​exports​ ​=​ ​{​
  ​require​: ​[​
    ​'source-map-support'​,​
    ​'something-else'​,​
    ​{​
      ​mochaHooks​: ​{​
        ​beforeEach​(​)​ ​{​
          ​this​.​foo​(​)​
        ​}​
      ​}​
    ​}​
  ​]​
​}​;

This would work, except not for --concurrent mode, right?

[boneskull]

@nicojs

module​.​exports​ ​=​ ​{​
  ​require​: ​[​
    ​'source-map-support'​,​
    ​'something-else'​,​
    ​{​
      ​mochaHooks​: ​{​
        ​beforeEach​(​)​ ​{​
          ​this​.​foo​(​)​
        ​}​
      ​}​
    ​}​
  ​]​
​}​;

I'm not sure I understand. The above would theoretically work as a .mocharc.js with parallel: true or not. I just need to modify the codebase to know what to do with a require value that isn't a string. (see below)

[boneskull]

(rather, you're right, I'd need to re-load the config file for each child process, because even if I did serialize and eval the damn thing it'd be missing the enclosing scope)

[boneskull]

anyway, not gonna do that right now. still: a --require foo.js where foo.js exports a mochaHooks prop will work with --parallel or without.

[nicojs]

I've created a small serialization library you might want to look at for communicating between child process and main process: https://www.npmjs.com/package/surrial
We're using it in Stryker for the same reason.

Anyway, it won't solve the enclosing scope issue. You cannot simply pick up a function and serialize / deseriaze it and expect every scenario to work.

You could, however, load the config files in child processes for this reason.

That's all totally out of scope for this PR. And off topic.

[craigtaub]

What do you think of my argument above? Convinced or no?

Yep good point, plugin system sounds useful.

[boneskull]

@nicojs Thanks. There are a lot of serialization libs out there--I just picked one.

You're welcome to try to replace the adhoc serialization stuff with surrial, but we have to be careful about a) performance, b) transmitting exceptions, and b) self-referential objects--e.g., I'm not serializing the entirety of a Suite for performance reasons.

[boneskull]

something is weird with github. the build (on travis anyway) has passed

[boneskull]

still waiting on appveyor.

[boneskull]

travis passed.