-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
add Root Hook Plugins #4244
add Root Hook Plugins #4244
Conversation
371a70f
to
93f5bf9
Compare
93f5bf9
to
50e9a7a
Compare
We have one Mocha instance and one root suite per each child-process, right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
/** | ||
* An alternative way to define root hooks that works with parallel runs. | ||
* @typedef {Object} MochaRootHookObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this typedef for typescript? Because this file is part of our bundled browser mocha.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
var beforeAll = [].concat(hooks.beforeAll || []); | ||
var beforeEach = [].concat(hooks.beforeEach || []); | ||
var afterAll = [].concat(hooks.afterAll || []); | ||
var afterEach = [].concat(hooks.afterEach || []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
This is correct.
It isn't possible to share a single It's not impossible for a single child process to reuse a I realize this doesn't work for people who need to run, e.g., |
2642081
to
74cf0ef
Compare
somehow accidentally closed this |
@juergba waiting for response from you in the comments, and also pls @mochajs/core take a look. I want to make sure
|
@craigtaub docs are in #4246 |
re: this is why I wanted to use if we use things that could be supported via
If we merged this, I think we would want to be explicit: do not use |
I note that you could do this, with minimal changes: // .mocharc.js
module.exports = {
require: [
'source-map-support',
'something-else',
{
mochaHooks: {
beforeEach() {
this.foo()
}
}
}
]
}; |
e0aaff2
to
f70bb78
Compare
@nicojs @craigtaub What do you think of my argument above? Convinced or no? |
Sure, that makes sense. Didn't know about other plugins yet. If you need a plugin system, this makes sense.
This would work, except not for |
f70bb78
to
61d2389
Compare
module.exports = {
require: [
'source-map-support',
'something-else',
{
mochaHooks: {
beforeEach() {
this.foo()
}
}
}
]
};
|
(rather, you're right, I'd need to re-load the config file for each child process, because even if I did serialize and |
anyway, not gonna do that right now. still: a |
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 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. |
Yep good point, plugin system sounds useful. |
61d2389
to
4d0f301
Compare
@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 |
(documentation will be in another PR) 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). 1. If it does, we save a reference to the property. 1. 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. 1. 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
4d0f301
to
0cdb978
Compare
something is weird with github. the build (on travis anyway) has passed |
still waiting on appveyor. |
travis passed. |
* Support mocha@<9 * Add support for [rootHooks](mochajs/mocha#4244) * Ignore `--parallel` flag (stryker will handle concurrency).
* Support peer dependency range mocha@<9 * Add support for [rootHooks](mochajs/mocha#4244) * Ignore `--parallel` flag (stryker will handle concurrency).
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
orrequire: 'some-library'
in Mocha's config file.The way it works is:
--require
, we check to see if that file exports a property namedmochaHooks
(can be multiple files).Mocha
is instantiated, if it is given arootHooks
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:
async
function, I noticed thatutils.type()
does not returnfunction
for async functions; it returnsasyncfunction
. I've added a (Node-specific, for now) test for this.handleRequires
is nowasync
, since it will need to be anyway to support ESM and calls toimport()
.fs.existsSync()
Ref: #4198