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

Added support for multiple reporters. #2184

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
72 changes: 51 additions & 21 deletions bin/_mocha
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,10 @@ program
.option('-c, --colors', 'force enabling of colors')
.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('-S, --sort', 'sort test files')
.option('-b, --bail', 'bail after first test failure')
.option('-O, --reporter-options <k=v,k2=v2,...>', 'reporter-specific options, use name1:{k=v,k2=v2,...},name2:{...} for multiple reporters')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not so keen on this API. would prefer something slightly more familiar, like subarg

Copy link
Author

@santiagoaguiar santiagoaguiar Jul 3, 2016

Choose a reason for hiding this comment

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

Sure, want me to add a dependency to subarg and use that instead? IIRC, This syntax was suggested on the previous PR, but I have no issues using a different one.

However, I do like the fact that is the same syntax for the reporter options when using a single reporter now, so you can just copy and paste them without the need of reformatting. e.g.:

mocha --reporter foo --reporter-options x=y,w=z
is equivalent to:
mocha --reporter foo --reporter-options foo:{x=y,w=z}

It's more an extension of the current syntax, than a new one.

In case we use subarg, what would the syntax look like? Something like this?

--reporter-options [ xunit --output=xunit.json ] --reporter-options [ foo --bar=x]

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I think that's what subarg does. adding the dep should be fine, assuming it will work in windows. when you do, please use an exact version instead of a range (~ or ^)

Copy link
Author

Choose a reason for hiding this comment

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

OK! I guess we'll need to keep support for the older syntax for a single reporter too, or otherwise we'll need to update the major version?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on using subarg.

@boneskull, do you have any argument/reason against doing a major version? I'd like to drop the old syntax. Better syntax, consistent single vs multiple reporters, less / simpler code, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I may have mentioned this before, but this format is not something people are accustomed to. can we please use the subarg format (for lack of anything better that I know of)? you'll probably want to pull in that package.

.option('-R, --reporter <name>', 'specify the reporter to use, or <name1>,<name2>,... for multiple reporters.', list, ['spec'])
.option('-S, --sort', "sort test files")
Copy link
Contributor

Choose a reason for hiding this comment

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

please revert these changes to quotes

.option('-b, --bail', "bail after first test failure")
Copy link
Contributor

Choose a reason for hiding this comment

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

and here

.option('-d, --debug', "enable node's debugger, synonym for node --debug")
.option('-g, --grep <pattern>', 'only run tests matching <pattern>')
.option('-f, --fgrep <string>', 'only run tests containing <string>')
Expand Down Expand Up @@ -196,16 +196,45 @@ 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 || L.length === 0) {
throw new Error("invalid reporter option '" + opt + "'");
} else if (L.length === 2) {
reporterOptions[L[0]] = L[1];
} else {
reporterOptions[L[0]] = true;
var reporterOptionsParser;
var multipleReporterFormat = /([^,:]+):{([^}]+)}/g;

var parseOptions = function (optionsStr) {
var options = {};
optionsStr.split(',').forEach(function (opt) {
var split = opt.split('=');
if (split.length > 2 || split.length === 0) {
throw new Error("invalid reporter option '" + opt + "'");
}

if (split.length === 2) {
options[split[0]] = split[1];
} else {
options[split[0]] = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. This wasn't supported in the previous implementation, was it? Or did I miss it?

Copy link
Author

Choose a reason for hiding this comment

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

Nope, it's on master for single reporter options, but the original PR (the one by the guys working on this originally) was written before it was supported and since it changed that area it removed support for single reporter options accidentally, and I didn't notice the change either. Now I did :).

Copy link
Contributor

Choose a reason for hiding this comment

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

This will be affected by argument parsing discussion above.

Copy link
Contributor

Choose a reason for hiding this comment

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

all of this custom parsing should be removed if using subarg

}
});

return options;
};

if (program.reporterOptions.match(multipleReporterFormat)) {
// multiple reporter config:
// spec:{a=1,b=2},dot:{c=3,d=4}
var match;
while ((match = multipleReporterFormat.exec(program.reporterOptions))) {
if (match.length !== 3) {
throw new Error("invalid multiple reporter options format '" + program.reporterOptions + "'");
}

var reporterName = match[1];
var reporterOptionStr = match[2];
reporterOptions[reporterName] = parseOptions(reporterOptionStr);
}
});
} else {
// single reporter config:
// a=1,b=2
reporterOptions._default = parseOptions(program.reporterOptions);
}
}

// reporter
Expand All @@ -214,16 +243,17 @@ mocha.reporter(program.reporter, reporterOptions);

// load reporter

var Reporter = null;
try {
Reporter = require('../lib/reporters/' + program.reporter);
} catch (err) {
program.reporter.forEach(function (reporterName) {
try {
Reporter = require(program.reporter);
} catch (err2) {
throw new Error('reporter "' + program.reporter + '" does not exist');
require('../lib/reporters/' + reporterName);
} catch (err) {
try {
require(reporterName);
} catch (err2) {
throw new Error('reporter "' + reporterName + '" does not exist');
}
}
}
});

// --no-colors

Expand Down
90 changes: 65 additions & 25 deletions lib/mocha.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

var escapeRe = require('escape-string-regexp');
var path = require('path');
var reporters = require('./reporters');
var builtInReporters = require('./reporters');
Copy link
Contributor

Choose a reason for hiding this comment

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

Appreciated change. :)

var utils = require('./utils');

/**
Expand All @@ -36,7 +36,7 @@ if (!process.browser) {

exports.utils = utils;
exports.interfaces = require('./interfaces');
exports.reporters = reporters;
exports.reporters = builtInReporters;
exports.Runnable = require('./runnable');
exports.Context = require('./context');
exports.Runner = require('./runner');
Expand Down Expand Up @@ -129,23 +129,42 @@ 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, keyed by reporter name, or
* '_default' for options to use when per-name options are not given.
* @api public
* @param {string|Function} reporter name or constructor
* @param {Object} reporterOptions optional options
*/
Mocha.prototype.reporter = function (reporter, reporterOptions) {
if (typeof reporter === 'function') {
this._reporter = reporter;
} else {
reporter = reporter || 'spec';
Mocha.prototype.reporter = function(reporters, reporterOptions) {
// if no reporter is given as input, default to spec reporter
reporters = reporters || ['spec'];
reporterOptions = reporterOptions || {};

if (!utils.isArray(reporters)) {
if ((typeof reporters === 'string')
|| (typeof reporters === 'function')) {
reporters = [reporters];
} else {
throw new Error('invalid reporter "' + reporters + '"');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, maybe throw an Error otherwise? Since it's not a type we care for.

}

// Load each reporter, only passing the options for that reporter
this._reporters = reporters.map(function(reporter) {
if (typeof reporter === 'function') {
return {
fn: reporter,
options: reporterOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it should be pointed out that when given an array of constructors as the first argument to Mocha.prototype.reporter it doesn't make sense to give an options object keyed by reporter name since we can't tell the reporter name from the constructor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise reader might wonder for a while.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, actually, constructors have a name property.

Also, @santiagoaguiar, reporterOptions may be an Array at this point, right? Just from reading the code, one would expect something like:

options: reporterOptions[reporter.name] || reporterOptions._default || {}

(same as at the end of the map's callback)

I might be wrong. I'll continue reviewing and come back if necessary.

};
}

var _reporter;
// Try to load a built-in reporter.
if (reporters[reporter]) {
_reporter = reporters[reporter];
if (builtInReporters[reporter]) {
_reporter = builtInReporters[reporter];
}
// Try to load reporters from process.cwd() and node_modules
if (!_reporter) {
Expand All @@ -165,9 +184,12 @@ Mocha.prototype.reporter = function (reporter, reporterOptions) {
if (!_reporter) {
throw new Error('invalid reporter "' + reporter + '"');
}
this._reporter = _reporter;
}
this.options.reporterOptions = reporterOptions;
return {
fn: _reporter,
options: reporterOptions[reporter] || reporterOptions._default || {}
};
}, this);

return this;
};

Expand Down Expand Up @@ -490,7 +512,14 @@ 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;
options.reporterOptions = reporterConfig.options;
Copy link
Contributor

Choose a reason for hiding this comment

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

Runners may not clone the passed in options, and instead hold a reference, and even modify its content. I think it would be better to clone the options object in each loop, and pass that in to the constructor.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if someone relies on a reference to options being held by the reporter. Thoughts @mochajs/mocha?

Copy link
Author

Choose a reason for hiding this comment

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

Any support for cloning the options object? Can't relay on Object.assign, no polyfill nor dependency that includes a similar utility. Should I go for a JSON.stringify/parse route?

Copy link
Contributor

Choose a reason for hiding this comment

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

@santiagoaguiar Since we support IE7, I'd use stringify/parse 'til we figure out what we do with polyfills / stuff. We lose support for using function as value for options (when using Mocha programmatically), but I don't think anyone is doing that.

return new _reporter(runner, options);
});
runner.ignoreLeaks = options.ignoreLeaks !== false;
runner.fullStackTrace = options.fullStackTrace;
runner.hasOnly = options.hasOnly;
Expand All @@ -502,21 +531,32 @@ Mocha.prototype.run = function (fn) {
if (options.globals) {
runner.globals(options.globals);
}
// 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, reporter);
this._growl(runner, reporters[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Which makes me think: "why does growl need to know anything about reporters".

Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to the PR but, apparently, we pass a reporter to this._growl because we get failures and duration from the Reporter.

IMO that info belongs to the Runner. Do you agree @boneskull? Just from looking at the Runner you should be able to tell whether it's running or not, for how long it's been running, how many pass/fails so far, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no idea about growl. we should look at gh bigdata and see if anyone is using it.

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

function done (failures) {
if (reporter.done) {
reporter.done(failures, fn);
} else {
fn && fn(failures);
function runnerDone(failures) {
function reporterDone(reporterFailures) {
if (--remain === 0) {
fn && fn(reporterFailures);
}
}

var remain = reporters.length;
reporters.forEach(function(reporter) {
if (reporter.done) {
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.

And then no need to pass it in here.

Copy link
Author

Choose a reason for hiding this comment

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

Same as before, reporter.done might call reporterDone. I'm not sure if we can assume it will pass the same failures object.

Copy link
Contributor

Choose a reason for hiding this comment

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

But even if reporter.done might call reporterDone, failures will still be in the scope due to runnerDone, right?

Copy link
Author

Choose a reason for hiding this comment

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

It's on scope, but the failures passed by reporter.done might be a different object. Contrived example:

reporter is a reporter with the following done implementation:

done(failures, cb) {
   cb(failures.concat({}))
}

If reporterDone used the failures captured before calling reporter.done, the above code would't work.

I'm not sure it's a real case, but I think it's safer, and the as far as I know the callback signature is (failures), so I think we should reflect & honor that there and use the actual object passed by arg.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, fair enough, preserves behavior. Post-merge we'll/I'll have to look into if it's any useful being able to change the failures number via the callback.

}
});
}

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

var tty = require('tty');
var fs = require('fs');
var mkdirp = require('mkdirp');
var path = require('path');
var EOL = require('os').EOL;
var diff = require('diff');
var ms = require('../ms');
var utils = require('../utils');
Expand Down Expand Up @@ -235,13 +239,16 @@ exports.list = function (failures) {
* of tests passed / failed etc.
*
* @param {Runner} runner
* @param {Object} options runner optional options
* @api public
*/

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

this.reporterOptions = options ? options.reporterOptions : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not already call openOutput in the constructor? If you specify an output you probably want any logs coming from the reporter to go to that file, by default.

Also, not sure about the naming, output. Not very specific, and output can also mean 'resulting data'. It's always a file right? What about outfile?

Copy link
Author

Choose a reason for hiding this comment

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

output was the name used previously by the xunit reporter, I preferred not changing it to keep backward compat.

Also, since reporterOptions are in principle reporter specific, I don't know if some other reporter might decide to interpret (or it's already interpreting!) output in a different way. That's why I went by making each interested reporter call the purely helper method openOutput explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. We could change this we decide to bump major though. Ideally, outfile/output would work for any reporter (that doesn't move the cursor around), and the reporter implementation could ignore it as long as it is using Base#write.

We could leave this improvement for later though.

/cc @boneskull


if (!runner) {
return;
}
Expand Down Expand Up @@ -336,6 +343,71 @@ Base.prototype.epilogue = function () {
console.log();
};

/**
* Opens for writing the file referenced on the `optionName` option
* of the reporter options, if any.
* Call this method to support writing to this file when calling
* write or writeLine.
*
* @param {string} optionName the name of the option, defaults to 'output'.
* @api public
*/
Base.prototype.openOutput = function (optionName) {
var output = this.reporterOptions && this.reporterOptions[optionName || 'output'];
if (output) {
if (!fs.createWriteStream) {
throw new Error('file output not supported in browser');
}

if (this.fileStream) {
throw new Error('file stream already opened');
}

mkdirp.sync(path.dirname(output));
this.fileStream = fs.createWriteStream(output);
Copy link
Contributor

Choose a reason for hiding this comment

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

If a fileStream already existed, it should close it.

Copy link
Author

Choose a reason for hiding this comment

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

Would it be better just to throw an error instead?

You shouldn't be calling openOutput twice, if you do (and for example you happen to write something in the meantime) that data will be lost. Feels like a path to silent errors for most of the times. If we want to support (for some odd reason) calling it twice, maybe we should add an explicit closeOutput?

}
};

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

/**
* Write to reporter output stream, adding an EOL at the end.
*
* @param {string} line
* @api public
*/
Base.prototype.writeLine = function (line) {
this.write(line + EOL);
};

/**
* Close the output stream and callback with failures.
*
* @param failures
* @param {Function} fn
*/
Base.prototype.done = function (failures, fn) {
if (this.fileStream) {
this.fileStream.end(function () {
fn(failures);
});
} else {
fn(failures);
Copy link
Contributor

Choose a reason for hiding this comment

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

Per the change I suggested above, it's not necessary to pass failures to fn.

Copy link
Author

Choose a reason for hiding this comment

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

I think done already existed as such and received a failures and callback, I only added a default implementation to base and use the mechanism used on the previous PR to call done on each reporter.

Since it already received a failures and a callback, I'm not sure we should change that, but I defer to you on that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good. We won't change it then, but we should look into its usefulness at some point.

}
};

/**
* Pad the given `str` to `len`.
*
Expand Down
15 changes: 12 additions & 3 deletions lib/reporters/json.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

var Base = require('./base');
var inherits = require('../utils').inherits;

/**
* Expose `JSON`.
Expand All @@ -17,16 +18,19 @@ exports = module.exports = JSONReporter;
*
* @api public
* @param {Runner} runner
* @param {Object} options Runner optional options
*/
function JSONReporter (runner) {
Base.call(this, runner);
function JSONReporter (runner, options) {
Base.call(this, runner, options);

var self = this;
var tests = [];
var pending = [];
var failures = [];
var passes = [];

this.openOutput();

runner.on('test end', function (test) {
tests.push(test);
});
Expand Down Expand Up @@ -54,10 +58,15 @@ function JSONReporter (runner) {

runner.testResults = obj;

process.stdout.write(JSON.stringify(obj, null, 2));
self.write(JSON.stringify(obj, null, 2));
});
}

/**
* Inherit from `Base.prototype`.
*/
inherits(JSONReporter, Base);

/**
* Return a plain-object representation of `test`
* free of cyclic properties etc.
Expand Down
Loading