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

Implement support for multiple reporters #1681

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion .mailmap
Original file line number Diff line number Diff line change
@@ -1 +1,11 @@
TJ Holowaychuk <tj@vision-media.ca>
TJ Holowaychuk <tj@vision-media.ca>
Travis Jeffery <tj@travisjeffery.com> <travisjeffery@gmail.com>
Travis Jeffery <tj@travisjeffery.com> Dr. Travis Jeffery <tj@travisjeffery.com>
Christopher Hiller <boneskull@boneskull.com> Christopher Hiller <chiller@badwing.com>
David da Silva Contín <dasilvacontin@gmail.com> David da Silva <daviddasilvacontin@me.com>
David da Silva Contín <dasilvacontin@gmail.com> David da Silva <dasilvacontin@gmail.com>
Ariel Mashraki <ariel@mashraki.co.il> Ariel Mashraki <ariel@codeoasis.com>
Ariel Mashraki <ariel@mashraki.co.il> Ariel Mashraki <ariel.mashraki@ironsrc.com>
Forbes Lindesay <forbes@lindesay.co.uk> Forbes Lindesay <fpfl2@cam.ac.uk>
Ben Bradley <ben@bradleyit.com> Ben Bradley <[ben.bradley@cigna.com|mailto:ben.bradley@cigna.com]>
Glen Mailer <glenjamin@gmail.com> Glen Mailer <glen.mailer@bskyb.com>
61 changes: 48 additions & 13 deletions bin/_mocha
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ program
.option('-C, --no-colors', 'force disabling of colors')
.option('-G, --growl', 'enable growl notification support')
.option('-O, --reporter-options <k=v,k2=v2,...>', 'reporter-specific options')
.option('-R, --reporter <name>', 'specify the reporter to use', 'spec')
.option('-R, --reporter <name>,...', 'specify the reporters to use', list, ['spec'])
.option('-S, --sort', "sort test files")
.option('-b, --bail', "bail after first test failure")
.option('-d, --debug', "enable node's debugger, synonym for node --debug")
Expand Down Expand Up @@ -193,13 +193,44 @@ Error.stackTraceLimit = Infinity; // TODO: config

var reporterOptions = {};
if (program.reporterOptions !== undefined) {
program.reporterOptions.split(",").forEach(function(opt) {
var L = opt.split("=");
if (L.length != 2) {
throw new Error("invalid reporter option '" + opt + "'");

program.reporterOptions.split(",").forEach(

// Two possible formats ...
/\w+{(\w+:\w+)+}/.test(program.reporterOptions) ?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dasilvacontin – Is there an existing test for bin/_mocha that I've missed? If not, how would you recommend I test these changes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benvinegar You could add an integration test: https://github.com/mochajs/mocha/tree/master/test/integration The helpers allow you to invoke the mocha binary (and hence _mocha as well) https://github.com/mochajs/mocha/blob/master/test/integration/helpers.js#L25

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielstjules I notice those files are on master, but not on v3.0.0.

Should I merge v3.0.0 up w/ master (or someone else)? Should I be targeting master instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely missed what branch this was targeting. I haven't been involved with the 3.0.0 branch, so I'm gonna defer this to @boneskull :) He'll be able to answer that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice those files are on master, but not on v3.0.0.

Indeed, master commits should be in v3.0.0.

Why target v3.0.0? Is this PR breaking backwards compatibility?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, the new syntax for --reporter-options has been added. Sorry, I missed that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original syntax still works, but if multiple reporters are present, it will apply that configuration to "all" active reporters.

Then it's not breaking. (?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why target v3.0.0? Is this PR breaking backwards compatibility?

Somebody in the last PR asked me to. I will close and switch it back (sigh).


// per-reporter config:
// spec{a:1,b:2},dot{c:3,d:4}
function(opt) {
var m = opt.match(/(\w+){(\w+:\w+)+}/);
if (m.length !== 3) {
throw new("invalid reporter option '" + opt + "'");
}

var reporterName = m[1]
, reporterOptStr = m[2];

reporterOptStr.split(",").forEach(function (reporterOpt) {
var L = reporterOpt.split(':');
if (L.length !== 2) {
throw new Error("invalid reporter option '" + opt + "'");
}
reporterOptions[L[0]] = L[1];
});
reporterOptions[reporterName] = reporterOptions[reporterName] || {};
reporterOptions[reporterName][L[0]] = L[1];
});
} :

// single reporter config:
// a=1,b=2
function(opt) {
var L = opt.split("=");
if (L.length != 2) {
throw new Error("invalid reporter option '" + opt + "'");
}
reporterOptions._default = reporterOptions._default || {};
reporterOptions._default[L[0]] = L[1];
}
);
}

// reporter
Expand All @@ -212,15 +243,19 @@ mocha.ui(program.ui);

// load reporter

try {
Reporter = require('../lib/reporters/' + program.reporter);
} catch (err) {
program.reporter.forEach(function (reporterConfig) {
var reporterName;
try {
Reporter = require(program.reporter);
reporterName = reporterConfig.split(':')[0];
Reporter = require('../lib/reporters/' + reporterName);
} catch (err) {
throw new Error('reporter "' + program.reporter + '" does not exist');
try {
Reporter = require(reporterName);
} catch (err) {
throw new Error('reporter "' + reporterName + '" does not exist');
}
}
}
});

// --no-colors

Expand Down
107 changes: 80 additions & 27 deletions lib/mocha.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,32 +131,63 @@ Mocha.prototype.addFile = function(file){
};

/**
* Set reporter to `reporter`, defaults to "spec".
* Set reporters to `reporters`, defaults to "spec".
*
* @param {String|Function} reporter name or constructor
* @param {Object} reporterOptions optional options
* @param {String|Function|Array of strings|Array of functions} reporter name as string,
* reporter constructor, or array of constructors or reporter names as strings.
* @param {Object} reporterOptions optional options TODO FIXME
* @api public
*/
Mocha.prototype.reporter = function(reporter, reporterOptions){
if ('function' == typeof reporter) {
this._reporter = reporter;
} else {
reporter = reporter || 'spec';
var _reporter;
try { _reporter = require('./reporters/' + reporter); } catch (err) {}
if (!_reporter) try { _reporter = require(reporter); } catch (err) {
err.message.indexOf('Cannot find module') !== -1
? console.warn('"' + reporter + '" reporter not found')
: console.warn('"' + reporter + '" reporter blew up with error:\n' + err.stack);

Mocha.prototype.reporter = function(reporters, reporterOptions){
// if no reporter is given as input, default to spec reporter
reporters = reporters || ['spec'];
reporterOptions = reporterOptions || {};

// If reporters argument is not a list, turn it into a list of reporter names
// or constructors that we'll iterate on right after to initialize them
if (!Array.isArray(reporters)) {
if (('string' == typeof reporters) ||
('function' == typeof reporters)) {
reporters = [reporters];
}
if (!_reporter && reporter === 'teamcity')
console.warn('The Teamcity reporter was moved to a package named ' +
'mocha-teamcity-reporter ' +
'(https://npmjs.org/package/mocha-teamcity-reporter).');
if (!_reporter) throw new Error('invalid reporter "' + reporter + '"');
this._reporter = _reporter;
}
this.options.reporterOptions = reporterOptions;

// Load each reporter
this._reporters = reporters.map(function (reporterConfig) {
if ('function' == typeof reporterConfig) {
return {
fn: reporterConfig,
options: reporterOptions
};
} else {
var reporterName
, path;

reporterName = reporterConfig.split(':');
if (reporterName.length > 1) {
path = reporterName[1];
reporterName = reporterName[0];
}
try { _reporter = require('./reporters/' + reporterName); } catch (err) {};
if (!_reporter) try { _reporter = require(reporterName); } catch (err) {
err.message.indexOf('Cannot find module') !== -1
? console.warn('"' + reporterName + '" reporter not found')
: console.warn('"' + reporterName + '" reporter blew up with error:\n' + err.stack);
}
if (!_reporter && reporterName === 'teamcity')
console.warn('The Teamcity reporter was moved to a package named ' +
'mocha-teamcity-reporter ' +
'(https://npmjs.org/package/mocha-teamcity-reporter).');
if (!_reporter) throw new Error('invalid reporter "' + reporterName + '"');
return {
fn: _reporter,
path: path,
options: reporterOptions[reporterName] || reporterOptions._default || {}
};
}
}, this);

return this;
};

Expand Down Expand Up @@ -424,23 +455,45 @@ Mocha.prototype.run = function(fn){
var options = this.options;
options.files = this.files;
var runner = new exports.Runner(suite, options.delay);
var reporter = new this._reporter(runner, options);
// For each loaded reporter constructor, create
// an instance and initialize it with the runner
var reporters = this._reporters.map(function (reporterConfig) {
var _reporter = reporterConfig.fn
, path = reporterConfig.path;

options.reporterOptions = reporterConfig.options;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of gross (changing the value of this.options per-reporter), but I did this to keep backwards compatibility with external reporters that have been using this.options.reporterOptions. For example, this Slack reporter.

Open to alternative suggestions.

return new _reporter(runner, options, path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In following commits you will be passing a reporterOptions object instead of mocha's options, I suppose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

});
runner.ignoreLeaks = false !== options.ignoreLeaks;
runner.fullStackTrace = options.fullStackTrace;
runner.asyncOnly = options.asyncOnly;
if (options.grep) runner.grep(options.grep, options.invert);
if (options.globals) runner.globals(options.globals);
if (options.growl) this._growl(runner, reporter);

// Use only the first reporter for growl, since we don't want to
// send several notifications for the same test suite
if (options.growl) this._growl(runner, reporters[0]);

if (options.useColors !== undefined) {
exports.reporters.Base.useColors = options.useColors;
}

exports.reporters.Base.inlineDiffs = options.useInlineDiffs;

function done(failures) {
function runnerDone(failures) {
var remain = reporters.length
, reporterDone = function(failures) {
if (--remain === 0) fn && fn(failures);
};

reporters.forEach(function (reporter) {
if (reporter.done) {
reporter.done(failures, fn);
} else fn && fn(failures);
reporter.done(failures, reporterDone);
} else {
reporterDone(failures);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's not because of your code, just discussing it, but I don't see clear why the reporter could/can modify the failures object that will be passed to the run's callback. I couldn't find any of our bundled reporters with a done method either. Any thoughts @mochajs/mocha?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find any of our bundled reporters with a done method either.

Never mind that; atom/find-and-replace didn't work in a way I expected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi, @benvinegar is closing the streams at Base.done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd refactor this using a new function that contains (--remain === 0) fn && fn(failures). dry and cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

});
}

return runner.run(done);
return runner.run(runnerDone);
};
39 changes: 38 additions & 1 deletion lib/reporters/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/

var tty = require('tty')
, fs = require('fs')
, diff = require('diff')
, ms = require('../ms')
, utils = require('../utils')
Expand Down Expand Up @@ -225,11 +226,21 @@ exports.list = function(failures){
* @api public
*/

function Base(runner) {
function Base(runner, options, path) {
var self = this
, stats = this.stats = { suites: 0, tests: 0, passes: 0, pending: 0, failures: 0 }
, failures = this.failures = [];

this.options = options;
this.path = path;

if (path) {
if (!fs.createWriteStream) {
throw new Error('file output not supported in browser');
}
this.fileStream = fs.createWriteStream(path);
}

if (!runner) return;
this.runner = runner;

Expand Down Expand Up @@ -323,6 +334,32 @@ Base.prototype.epilogue = function(){
console.log();
};


/**
* Write to reporter output stream
*/
Base.prototype.write = function(str){
if (this.fileStream) {
this.fileStream.write(str);
} else {
process.stdout.write(str);
}
};

Base.prototype.writeLine = function(line) {
this.write(line + '\n');
};

Base.prototype.done = function(failures, fn) {
if (this.fileStream) {
this.fileStream.end(function() {
fn(failures);
});
} else {
fn(failures);
}
};

/**
* Pad the given `str` to `len`.
*
Expand Down
4 changes: 2 additions & 2 deletions lib/reporters/json.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ exports = module.exports = JSONReporter;
* @api public
*/

function JSONReporter(runner) {
function JSONReporter(runner, options, path) {
var self = this;
Base.call(this, runner);
Base.call(this, runner, options, path);

var tests = []
, pending = []
Expand Down
Loading