Implement support for multiple reporters #1681
Conversation
|
Thanks for working on this, @benvinegar! :) |
So in the end, we can't ignore |
|
For the reporters that use special characters to move the cursor, change colors, clear lines, etc. we will write those as-is if the target is a file. |
|
@dasilvacontin – can you comment on my latest changes and whether you feel the approach is correct? Right now, only the For now, you can verify the behavior from the command line like so: bin/_mocha --reporter dot,xunit=tmp |
|
I'll check it out tonight, looking forward to it! |
| try { | ||
| Reporter = require(program.reporter); | ||
| reporter = reporterConfig.split(':')[0]; |
There was a problem hiding this comment.
This would be easier to understand as reporterName or something similar.
|
Looks quite good, thanks for working on it @benvinegar! 😄 |
| var _reporter = reporterConfig.fn | ||
| , path = reporterConfig.path; | ||
|
|
||
| options.reporterOptions = reporterConfig.options; |
There was a problem hiding this comment.
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.
|
After the latest set of changes, Mocha now accepts per-reporter Example: mocha --reporter spec,json:tmp/out.json --reporter-options spec{style=prettyprint,color=blue},json{prettify,indent=tabs}The original syntax still works, but if multiple reporters are present, it will apply that configuration to "all" active reporters. In the example below, "style=prettyprint" is passed to both the "spec" and "json" reporters (which they may ignore). mocha --reporter spec,json:tmp/out.json --reporter-options style=prettyprint |
| program.reporterOptions.split(",").forEach( | ||
|
|
||
| // Two possible formats ... | ||
| /\w+{(\w+:\w+)+}/.test(program.reporterOptions) ? |
There was a problem hiding this comment.
@dasilvacontin – Is there an existing test for bin/_mocha that I've missed? If not, how would you recommend I test these changes?
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Oh, the new syntax for --reporter-options has been added. Sorry, I missed that.
There was a problem hiding this comment.
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. (?)
There was a problem hiding this comment.
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).
|
This is cool! Could you provide short code snippet on how to let mocha run with a spec and e.g. a bamboo reporter? We are using grunt-mocha-test. |
|
Going to recreate and target master. |
|
👍 |
Squashed commit of #1360 (previously #930), against v3.0.0.
Necessary for completion:
--reporter-optionsneeds to be capable of configuring each reporter