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

Multiple reporters support (continuation of #930) #1360

Closed
wants to merge 8 commits into from
Closed

Multiple reporters support (continuation of #930) #1360

wants to merge 8 commits into from

Conversation

benvinegar
Copy link
Contributor

This is a continuation of @misterdjules PR #930 – multiple reporters support.

On top of @misterdjules work, it has:

  • Merged up w/ master (original patch was > 1 year old)
  • Restores 'spec' as the default reporter
  • Fixed unit tests
  • An additional unit test for multiple reporters (tests/reporters/multiple.js)
  • Renames variables to be closer to original mocha.js code (e.g. _reporter -> _reporters)
  • Keeps original Mocha.prototype.reporter interface (vs the plural reporters) for API compatibility. IMHO anybody using Mocha directly shouldn't have to change their code to remain compatible.

It does not implement Growl as a separate reporter, as was discussed here. I think it could be done as a follow-up patch just fine, but if you'd prefer that was done before accepting then that's cool too.

Julien Gilli and others added 5 commits July 19, 2013 10:36
…ake the same types of arguments

as before, plus a list of reporter names as a comma separated string, or a list of constructors.
This allows using more than one reporter.
- Replaced command line switch --reporter <name> by --reporters <name>,... so that users
can pass a comma separated list of reporters on the command line.
- The old --reporters command line switch used to list available reporters is now --list-reporters.
… --reporter and --list-reporters

is back to --reporters.
- Use the "list" arguments parser for --reporter now.
- Mocha.reporters does not accept a comma-separated string of reporter names anymore, this is the command line parser's job
to parse the --reporter command line switch and provide an array to this function.
Conflicts:
	bin/_mocha
	lib/mocha.js
} catch (err) {
throw new Error('reporter "' + program.reporter + '" does not exist');
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, should this come back? Mocha.prototype.reporter also throws if the reporter isn't found.

Copy link
Contributor

Choose a reason for hiding this comment

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

Checking if the reporters exist is still necessary. Ignoring those which are not found is not the way to go, since there is a reason why they have been specified.

@benvinegar
Copy link
Contributor Author

Looking at this closer ... I think there's a fundamental flaw. Even though, yes, this invokes multiple reporters – they still all go to stdout.

e.g. if you specify --reporters spec,json you will have the spec output to stdout, followed by JSON output to stdout. If you use json-stream as one of your reporters, I think the output could also intermingle.

So, I'm not sure if this patch is useful unless you can additionally specify a target output file per reporter, something like:

mocha test.js --reporter:spec,json=tmp/out.json

... which seems kind of gross. Thoughts?

@travisjeffery
Copy link
Contributor

ya that's bit gross. what's your need/use case for multiple reporters btw?

@boneskull
Copy link
Contributor

@travisjeffery lcov generation + arbitrary console logger. As it stands if I want this, I need to run my tests 2x.

@demmer
Copy link
Contributor

demmer commented Sep 15, 2014

@travisjeffery Please consider #1218 as part of pushing this multiple reporter work through.

From my standpoint, being able to include the xunit reporter writing to a file and a spec reporter to console would be great, but I agree with @benvinegar that having multiple reporters going to console doesn't seem very useful.

@benvinegar
Copy link
Contributor Author

@travisjeffery We use both json and xunit at the same time. The former for some scripts that generate reports on our unit tests. The latter for integrating with our particular CI system.

I was working on a little converter – pretty trivial – but I saw this patch was nearly accepted and thought I should help push it through.

@jbnicolai
Copy link

+1 from me :)

@travisjeffery travisjeffery force-pushed the master branch 2 times, most recently from 35c1580 to 64dfc0b Compare October 22, 2014 04:56
@boneskull boneskull added the type: feature enhancement proposal label Nov 7, 2014
@Zensavona
Copy link

Is this going in any time soon?

@lpinca
Copy link

lpinca commented Dec 28, 2014

I would like to have this feature. 👍

@dasilvacontin
Copy link
Contributor

mocha --reporters [spec],[json:path/to/file],[nyan:path/to/file]
mocha --reporters spec, json:path/to/file, nyan:path/to/file

I'm not sure how possible these are with commander, I should investigate the possibilities.

@benvinegar
Copy link
Contributor Author

@dasilvacontin That format looks good to me (and certainly better than whatever the hell I suggested above). I'd be glad to move forward with it.

@dasilvacontin
Copy link
Contributor

I really like the colon to define the file path for a given reporter. It would be nice to hear someone else's opinion too.

Very excited about landing this PR on master soon. 😄

@benvinegar
Copy link
Contributor Author

Okay, just tried merging up with master and noticed that mocha introduced --reporter-options (7ce102b) ... which appears to only be used by the XUnit reporter and is not documented on mochajs.org.

@dasilvacontin – any thoughts on how you'd like multiple reporters to play nicely with --reporter-options?

e.g. something like:

mocha --reporter spec,json:tmp/out.json --reporter-options json:prettyprint

@dasilvacontin
Copy link
Contributor

Reporter options currently have the following format:

--reporter-options <k=v,k2=v2,...>'

Since you can set multiple options for each reporter, what about:

mocha --reporter spec,json:tmp/out.json --reporter-options spec{style:prettyprint,color:blue},json{prettify,indent:tabs}
// or
mocha --reporter spec,json:tmp/out.json --reporter-options spec{style=prettyprint,color=blue},json{prettify,indent=tabs}

I kind of prefer the {x:y} syntax since it resembles object definition.

@benvinegar
Copy link
Contributor Author

That works for me. I think we should keep the default behavior the same as today though, for backwards compatibility. Meaning, if you don't specify multiple reporters, this should work fine:

mocha --reporter json --reporter-options prettyprint

That make sense?

@dasilvacontin
Copy link
Contributor

Sure. We could change it in the future, if necessary, or requested. Current reporter-options code needs to be updated to support multiple options, and also to support options without value. Like the prettyprint one, which should set prettyprint to true in the options object.

@travisjeffery
Copy link
Contributor

mocha --reporter spec,json:tmp/out.json --reporter-options spec{style:prettyprint,color:blue},json{prettify,indent:tabs}

this looks good to me

@dasilvacontin
Copy link
Contributor

@travisjeffery, glad to hear that. :)

@jaredly
Copy link

jaredly commented Apr 30, 2015

+1
What's the status here? once merge conflicts are fixed, is this ready to go?

Conflicts:
	bin/_mocha
	lib/mocha.js
@benvinegar
Copy link
Contributor Author

Picking this back up.

@boneskull
Copy link
Contributor

I think we need to close this one, as I imagine we'd want this in branch v3.0.0

@boneskull
Copy link
Contributor

please create new PR and squash when ready

@darkn3rd
Copy link

👍

@e-cloud
Copy link

e-cloud commented Oct 15, 2016

any progress? mocha v3.0.0 has been released for a while

@kay-ramme
Copy link

would be great to have ..

@azachar
Copy link

azachar commented Nov 11, 2016

+1

@pmarreck
Copy link

pmarreck commented Mar 1, 2018

Literally just used mocha for the first time and one of the first things I noticed was the total absence of this feature (having used multiple test reporters on Rails tests in the past).

FFS, guys...

@boneskull
Copy link
Contributor

@pmarreck Please keep your entitled attitude off of this issue tracker.

@Anthony-Mckale
Copy link

guessing the developers of mocha have good reasons for not supporting multiple reporters out the box

for anyone else coming to this ticket looking for multiple reports, i found writing a wee simple custom reporter which pulled in multiple reports from mocha really worked for me

As I found a lot of old node modules with combo reporters, on the web copy and pasted the reports, which I found a bit dangerous for my liking (would worry about have a old version of the reporters)

my ~20 line reporter

`let mocha = require('mocha');

exports = module.exports = MultiReporter;

function MultiReporter(runner, options) {
this.reports = [];
if (!options.reporterOptions.reporters) {
console.log('\nneeds --reporter-options reporters="SPACE_SEPARATED_MOCHA_REPORTS"');
return;
}
let that = this;
options.reporterOptions.reporters.split(' ').forEach(function(report) {
let ReportClass = mocha.reporters[report];
if (!ReportClass) {
console.log('\ninvalid report class available: ' + Object.keys(mocha.reporters).join(','));
return;
}
let reportInstance = new ReportClass(runner, options);
that.reports.push(reportInstance);
});
}
MultiReporter.prototype.epilogue = function() {
this.reports.forEach(function(reportInstance) {
reportInstance.epilogue();
});
};
`

which you can just reference with

mocha STUFF_FLAGS TEST_FILES --reporter PATH_TO_REPORTER/REPORTER_FILE.js --reporter-options reporters=REPORTERS,OTHER_OPTS

aka for my project (with my project flags)

node node_modules\mocha\bin\mocha --require babel-register --require test/mocha/bootstrap.js --ui chai-style-jasmine src\**\*.spec.js --reporter test/mocha/multi-reporter.js --reporter-options reporters="XUnit Spec",output=reports/mocha-xunit/test-results.xml

@benjamingr
Copy link

Hey @boneskull 👋

Is doing #1360 (comment) generally considered fine?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature enhancement proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.