Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🗣️ Discussion: Plugin notes #4116

Closed
boneskull opened this issue Dec 3, 2019 · 5 comments
Closed

🗣️ Discussion: Plugin notes #4116

boneskull opened this issue Dec 3, 2019 · 5 comments
Labels
type: discussion debates, philosophy, navel-gazing, etc. type: feature enhancement proposal

Comments

@boneskull
Copy link
Contributor

I've done some thinking about the plugin system (or lack thereof) recently, as I was exploring adding native support for snapshot tests.

(I'm not sure adding native support for snapshot tests is a great idea, because e.g. snap-shot-it already exists and works fine; this is not the right issue in which to discuss it)

Here's a breakdown of the way things currently work:

  1. You can load an arbitrary script with --require before the main Mocha object is instantiated with any user-supplied options (and loads any test files)
  2. If such a module sets a prop in require('mocha').reporters or require('mocha').interfaces, e.g., my-reporter or my-interface, the custom reporter or interface implementation will be able to be referenced by --reporter my-reporter or --ui my-interface, respectively (because "registration" comes before instantiation of Mocha)
  3. At this point, Mocha will begin loading test files in whatever order is defined by the user. Using --file will allow the user to be explicit about this; anything else will not make guarantees. Thus, a plugin loaded with --file will have access to the root suite, describe, it, hooks, etc. Not every file "run" by Mocha needs to be a "test" file. If a global setup/teardown needs to happen, use --file <./path/to/harness.js>; if it's a 3rd-party package, --file <packagename> works identically.

Some shortcomings of this (there are many, but here are a few), in order of (IMO) "least problematic" to "most problematic":

  1. The way in which reporters and interfaces are "registered" is awkward.
  2. --file <packagename> is rather unintuitive and cumbersome; it's not really designed for arbitrary plugins that need to operate at the root-suite level, but it works.
  3. --require'd files which are not reporters nor interfaces cannot access any settings or options short of monkeypatching require('mocha').prototype.
  4. Arbitrary plugins (packages) loaded with --file or otherwise via require() cannot access individual suites, hooks, nor test objects; all they can do is work with what Mocha injects into the global context, the same as test files (describe(), it(), etc.). They cannot access the Mocha instance, nor the Runner instance, nor reporters, etc.

Some ways in which we can solve these issues:

Required Module Execution

Expand --require to look at the "return type" of a module. If it has a return type matching "a Mocha plugin", then save a reference to it, instantiate Mocha, and pass this Mocha instance to each module we've saved a reference to.

A "Mocha plugin" cannot just be module.exports = function(mocha) {}, because existing modules/package export a function, like @babel/register. this would break stuff and cause bad weirdness

What this would allow plugins to do (keep in mind this would be before mocha begins even loading files):

  • Inject hooks into the root suite by modifying mocha.suite
  • Listen on events emitted by the root suite and/or any other suite once Mocha starts loading test files. This would allow a plugin to do something when, e.g., a new suite is created, or a new test/hook/suite is created within a suite.
  • Modify options before run
  • Add files to the list of files Mocha attempts to load (other plugins, harnesses, tests??)
  • etc

Drawbacks of this approach:

  • Exposes events which may be previously considered internal
  • Because event listeners run synchronously, prevents async code from being run with any sort of guarantees. A workaround may be to set the delay option programmatically and prepend a file into the list which executes Mocha's global.run(). Otherwise, there may be many breaking changes necessary to support "async listeners". On the plus side, it'd probably give us async suites "for free!"

Probably best to define a more explicit plugin API surface where something loaded via --require would return some metadata and handler(s); maybe auto-loading of plugins could be a thing too. This would ideally provide a more "official" interface for defining custom interfaces and reporters, as well. That's... more work.

Expose Events via Singleton

Given:

  • test file a.spec.js which contains require('mocha-plugin')
  • package mocha-plugin which has a main module that contains const Mocha = require('mocha') (Mocha is the main class found in lib/mocha.js)

We could expose Mocha.events (or whatever), which is a singleton EE that listens on events emitted from Mocha's Runner instance, and re-emits them with two parameters: the current Mocha instance and whatever else the original event emits. For example:

// mocha-plugin main 
const {events, Runner} = require('mocha');
const {EVENT_HOOK_BEGIN} = Runner.constants;
events.on(EVENT_HOOK_BEGIN, (mocha, hook) => {
  // display the full hook title whenever a hook is about to run
  console.error(`running hook ${hook.fullTitle()`)
});

This suffers from the same problem as before, where these listeners must be synchronous. Again, a "proper" API surface would be better, so we can avoid exposing more internals. The mocha instance is passed along because while we can reasonably be sure that we only have one mocha instance, we cannot always guarantee it.

It would also be a good idea to allow packages loaded with --require to listen to these events. We don't even have a Runner instance at this point, so we'd need to do something to work around that.


It'd be ideal to allow plugins to function at both "setup time" and "run time", and have a similar API for each. The latter would necessarily have some restrictions on what it can do.

I don't think EE's are the right solution, as much of a time-saver they might be.

This will be very helpful to tooling authors trying to extend Mocha. But whatever we do, we're stuck with it.

cc @mochajs/core

@boneskull boneskull added type: feature enhancement proposal type: discussion debates, philosophy, navel-gazing, etc. labels Dec 3, 2019
@boneskull
Copy link
Contributor Author

cc @papandreou and @sunesimonsen who I discussed this sort of thing with

@papandreou
Copy link
Contributor

Thanks for writing this down! First thing that popped up in my head: Would it work roughly the same way in the browser? I assume you(/plugins) could interact with the mocha global before kicking off the show with mocha.run();?

@boneskull
Copy link
Contributor Author

@papandreou You can interact with the mocha global, yes--I mean, you can do this now. It's a monkeypatched instance of Mocha.

Now that I wrote that, I'm somewhat less hesitant to introduce a mocha global in Node.js, since it's already in the browser...

@boneskull
Copy link
Contributor Author

a mocha global, assuming it's a singleton, would solve the "expose events" question. still... I don't really want to make private APIs public.

@JoshuaKGoldberg JoshuaKGoldberg changed the title plugin notes 🗣️ Discussion: Plugin notes Dec 27, 2023
@JoshuaKGoldberg
Copy link
Member

These are very good notes. Cross-linking them with #1457 and closing this issue to keep our queue small.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: discussion debates, philosophy, navel-gazing, etc. type: feature enhancement proposal
Projects
None yet
Development

No branches or pull requests

3 participants