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.

Sign up for GitHub

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

Jump to bottom

add Root Hook Plugins #4244

Merged
merged 1 commit into from May 18, 2020
Merged

add Root Hook Plugins #4244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
60 changes: 50 additions & 10 deletions lib/cli/run-helpers.js
View file
Open in desktop
Expand Up @@ -12,10 +12,9 @@ const path = require('path');
const debug = require('debug')('mocha:cli:run:helpers');
const watchRun = require('./watch-run');
const collectFiles = require('./collect-files');
const {type} = require('../utils');
const {format} = require('util');

const cwd = (exports.cwd = process.cwd());
const {createInvalidPluginError} = require('../errors');
const {createInvalidPluginError, createUnsupportedError} = require('../errors');

/**
* Exits Mocha when tests + code under test has finished execution (default)
Expand Down Expand Up @@ -75,20 +74,60 @@ exports.list = str =>
Array.isArray(str) ? exports.list(str.join(',')) : str.split(/ *, */);

/**
* `require()` the modules as required by `--require <require>`
* `require()` the modules as required by `--require <require>`.
*
* Returns array of `mochaHooks` exports, if any.
* @param {string[]} requires - Modules to require
* @returns {Promise<MochaRootHookObject|MochaRootHookFunction>} Any root hooks
* @private
*/
exports.handleRequires = (requires = []) => {
requires.forEach(mod => {
exports.handleRequires = async (requires = []) =>
requires.reduce((acc, mod) => {
let modpath = mod;
if (fs.existsSync(mod, {cwd}) || fs.existsSync(`${mod}.js`, {cwd})) {
// this is relative to cwd
if (fs.existsSync(mod) || fs.existsSync(`${mod}.js`)) {
modpath = path.resolve(mod);
debug('resolved %s to %s', mod, modpath);
debug('resolved required file %s to %s', mod, modpath);
}
const requiredModule = require(modpath);
if (type(requiredModule) === 'object' && requiredModule.mochaHooks) {
const mochaHooksType = type(requiredModule.mochaHooks);
if (/function$/.test(mochaHooksType) || mochaHooksType === 'object') {
debug('found root hooks in required file %s', mod);
acc.push(requiredModule.mochaHooks);
} else {
throw createUnsupportedError(
'mochaHooks must be an object or a function returning (or fulfilling with) an object'
);
}
}
require(modpath);
debug('loaded required module "%s"', mod);
});
return acc;
}, []);

/**
* Loads root hooks as exported via `mochaHooks` from required files.
* These can be sync/async functions returning objects, or just objects.
* Flattens to a single object.
* @param {Array<MochaRootHookObject|MochaRootHookFunction>} rootHooks - Array of root hooks
* @private
* @returns {MochaRootHookObject}
*/
exports.loadRootHooks = async rootHooks => {
const rootHookObjects = await Promise.all(
rootHooks.map(async hook => (/function$/.test(type(hook)) ? hook() : hook))
);

return rootHookObjects.reduce(
(acc, hook) => {
acc.beforeAll = acc.beforeAll.concat(hook.beforeAll || []);
acc.beforeEach = acc.beforeEach.concat(hook.beforeEach || []);
acc.afterAll = acc.afterAll.concat(hook.afterAll || []);
acc.afterEach = acc.afterEach.concat(hook.afterEach || []);
return acc;
},
{beforeAll: [], beforeEach: [], afterAll: [], afterEach: []}
);
};

/**
Expand All @@ -106,6 +145,7 @@ const singleRun = async (mocha, {exit}, fileCollectParams) => {
debug('single run with %d file(s)', files.length);
mocha.files = files;

// handles ESM modules
await mocha.loadFilesAsync();
return mocha.run(exit ? exitMocha : exitMochaLater);
};
Expand Down
10 changes: 8 additions & 2 deletions lib/cli/run.js
View file
Open in desktop
Expand Up @@ -18,6 +18,7 @@ const {
list,
handleRequires,
validatePlugin,
loadRootHooks,
runMocha
} = require('./run-helpers');
const {ONE_AND_DONES, ONE_AND_DONE_ARGS} = require('./one-and-dones');
Expand Down Expand Up @@ -285,12 +286,17 @@ exports.builder = yargs =>
);
}

return true;
})
.middleware(async argv => {
// load requires first, because it can impact "plugin" validation
handleRequires(argv.require);
const rawRootHooks = await handleRequires(argv.require);
validatePlugin(argv, 'reporter', Mocha.reporters);
validatePlugin(argv, 'ui', Mocha.interfaces);

return true;
if (rawRootHooks.length) {
argv.rootHooks = await loadRootHooks(rawRootHooks);
}
})
.array(types.array)
.boolean(types.boolean)
Expand Down
49 changes: 49 additions & 0 deletions lib/mocha.js
View file
Open in desktop
Expand Up @@ -118,6 +118,8 @@ exports.Test = require('./test');
* @param {number} [options.slow] - Slow threshold value.
* @param {number|string} [options.timeout] - Timeout threshold value.
* @param {string} [options.ui] - Interface name.
* @param {MochaRootHookObject} [options.rootHooks] - Hooks to bootstrap the root
* suite with
*/
function Mocha(options) {
options = utils.assign({}, mocharc, options || {});
Expand Down Expand Up @@ -165,6 +167,10 @@ function Mocha(options) {
this[opt]();
}
}, this);

if (options.rootHooks) {
this.rootHooks(options.rootHooks);
}
}

/**
Expand Down Expand Up @@ -959,3 +965,46 @@ Mocha.prototype.run = function(fn) {

return runner.run(done);
};

/**
* Assigns hooks to the root suite
* @param {MochaRootHookObject} [hooks] - Hooks to assign to root suite
boneskull marked this conversation as resolved.
Show resolved Hide resolved
* @chainable
*/
Mocha.prototype.rootHooks = function rootHooks(hooks) {
if (utils.type(hooks) === 'object') {
var beforeAll = [].concat(hooks.beforeAll || []);
var beforeEach = [].concat(hooks.beforeEach || []);
var afterAll = [].concat(hooks.afterAll || []);
var afterEach = [].concat(hooks.afterEach || []);
Copy link
Member

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?

Copy link
Member Author

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.

var rootSuite = this.suite;
beforeAll.forEach(function(hook) {
rootSuite.beforeAll(hook);
});
beforeEach.forEach(function(hook) {
rootSuite.beforeEach(hook);
});
afterAll.forEach(function(hook) {
rootSuite.afterAll(hook);
});
afterEach.forEach(function(hook) {
rootSuite.afterEach(hook);
});
}
return this;
};

/**
* An alternative way to define root hooks that works with parallel runs.
* @typedef {Object} MochaRootHookObject
Copy link
Member

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.

Copy link
Member Author

@boneskull boneskull Apr 23, 2020 •
edited

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?

Copy link
Member Author

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.

* @property {Function|Function[]} [beforeAll] - "Before all" hook(s)
* @property {Function|Function[]} [beforeEach] - "Before each" hook(s)
* @property {Function|Function[]} [afterAll] - "After all" hook(s)
* @property {Function|Function[]} [afterEach] - "After each" hook(s)
*/

/**
* An function that returns a {@link MochaRootHookObject}, either sync or async.
* @callback MochaRootHookFunction
* @returns {MochaRootHookObject|Promise<MochaRootHookObject>}
*/
5 changes: 3 additions & 2 deletions lib/utils.js
View file
Open in desktop
Expand Up @@ -63,8 +63,9 @@ exports.isString = function(obj) {
exports.slug = function(str) {
return str
.toLowerCase()
.replace(/ +/g, '-')
.replace(/[^-\w]/g, '');
.replace(/\s+/g, '-')
.replace(/[^-\w]/g, '')
.replace(/-{2,}/g, '-');
};

/**
Expand Down
16 changes: 16 additions & 0 deletions test/integration/fixtures/options/require/root-hook-defs-a.fixture.js
View file
Open in desktop
@@ -0,0 +1,16 @@
'use strict';

exports.mochaHooks = {
beforeAll() {
console.log('beforeAll');
},
beforeEach() {
console.log('beforeEach');
},
afterAll() {
console.log('afterAll');
},
afterEach() {
console.log('afterEach');
}
};
36 changes: 36 additions & 0 deletions test/integration/fixtures/options/require/root-hook-defs-b.fixture.js
View file
Open in desktop
@@ -0,0 +1,36 @@
'use strict';

exports.mochaHooks = {
beforeAll: [
function() {
console.log('beforeAll array 1');
},
function() {
console.log('beforeAll array 2');
}
],
beforeEach: [
function() {
console.log('beforeEach array 1');
},
function() {
console.log('beforeEach array 2');
}
],
afterAll: [
function() {
console.log('afterAll array 1');
},
function() {
console.log('afterAll array 2');
}
],
afterEach: [
function() {
console.log('afterEach array 1');
},
function() {
console.log('afterEach array 2');
boneskull marked this conversation as resolved.
Show resolved Hide resolved
}
]
};
16 changes: 16 additions & 0 deletions test/integration/fixtures/options/require/root-hook-defs-c.fixture.js
View file
Open in desktop
@@ -0,0 +1,16 @@
'use strict';

exports.mochaHooks = async () => ({
beforeAll() {
console.log('beforeAll');
},
beforeEach() {
console.log('beforeEach');
},
afterAll() {
console.log('afterAll');
},
afterEach() {
console.log('afterEach');
}
});
36 changes: 36 additions & 0 deletions test/integration/fixtures/options/require/root-hook-defs-d.fixture.js
View file
Open in desktop
@@ -0,0 +1,36 @@
'use strict';

exports.mochaHooks = async() => ({
beforeAll: [
function() {
console.log('beforeAll array 1');
},
function() {
console.log('beforeAll array 2');
}
],
beforeEach: [
function() {
console.log('beforeEach array 1');
},
function() {
console.log('beforeEach array 2');
}
],
afterAll: [
function() {
console.log('afterAll array 1');
},
function() {
console.log('afterAll array 2');
}
],
afterEach: [
function() {
console.log('afterEach array 1');
},
function() {
console.log('afterEach array 2');
}
]
});
6 changes: 6 additions & 0 deletions test/integration/fixtures/options/require/root-hook-test-2.fixture.js
View file
Open in desktop
@@ -0,0 +1,6 @@
// run with --require root-hook-defs-a.fixture.js --require
// root-hook-defs-b.fixture.js

it('should also have some root hooks', function() {
// test
});
6 changes: 6 additions & 0 deletions test/integration/fixtures/options/require/root-hook-test.fixture.js
View file
Open in desktop
@@ -0,0 +1,6 @@
// run with --require root-hook-defs-a.fixture.js --require
// root-hook-defs-b.fixture.js

it('should have some root hooks', function() {
// test
});