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

Conversation

santiagoaguiar
Copy link

Rework of #2091, rebased to master.

Includes work from @benvinegar & @misterdjules, this PR also:

  • Removes support for --reporter json:<outputfile> which was added on previous PRs, instead reporters that support output to files should use --reporter-options.
  • Added utility methods on Base reporter to open an output option provided on reporter options as an output file, and write to it. This is a generalization of the mechanism used by the xunit reporter to other reporters: json, json-cov, html-cov, markdown. All those reporters, and the xunit reporter, now inherit from Base and use these utility methods to write their output.
  • Fixes lint warnings.
  • Fixed issues with parsing the reporterOptions when there were multiple reporters.
  • Adds a test case to check that the json reporter writes to the file specified by the : option.
  • Adds method documentation.

Submitted as PR independent of #2091 since it's a somewhat different approach. I think this one is better, simpler, and makes more sense, but I leave the other PR in case we want to go the way @benvinegar was proposing (using the --reporter <name>:<outputfile> syntax.)

@santiagoaguiar santiagoaguiar force-pushed the multiple-reporters-no-output branch 2 times, most recently from 7d97daa to 054a1a6 Compare April 25, 2016 16:55
@santiagoaguiar
Copy link
Author

I rebased it to master.

Anything else that can be done to get this one merged to master?

Thanks!

@dasilvacontin
Copy link
Contributor

Hi @santiagoaguiar! As soon as I'm done with the browser CI integration, I'll start reviewing the most demanded PRs, which includes this one. Should be very soon!

@santiagoaguiar
Copy link
Author

OK! Didn't want to rush anyone :) Just let me know if there's anything I can do to help or improve! Thanks.

@dasilvacontin dasilvacontin self-assigned this May 11, 2016
@mpahuja
Copy link

mpahuja commented May 13, 2016

Hope this will also support custom reporters. Nice feature added instead of needing to depend on other modules like mocha-multi to get this job done.

@santiagoaguiar
Copy link
Author

You can specify as many reporters as you want, custom or not.

However, if building a custom reporter that must write to a file, it has to accept reporterOptions, like the xunit reporter does. If it writes to console, you can use it too, but its output might get mixed with output from other reporters that also write to console (so, make sure you only use one of those :)).

Here's a custom reporter that works OK with this PR (we are using it) and supports reporterOptions to write to a file: mocha-trx-reporter

@domharrington
Copy link

Any news on when this might be merged? I'm hoping to use https://github.com/issacg/mocha-bamboo-reporter as well as the spec reporter in CI.

@dasilvacontin
Copy link
Contributor

@domharrington Gonna take a look at it this weekend if no one beats me to it.

}
reporterOptions[reporterName] = reporterOptions[reporterName] || {};
Copy link
Contributor

@dasilvacontin dasilvacontin May 29, 2016

Choose a reason for hiding this comment

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

Cache the options object for this reporter outside the loop, and then you don't have to check it it's initialised to {} on each iteration. Having a variable with a short name like options will also improve the legibility of the code.

After the loop you can assign back options to reporterOptions[reporterName].

@boneskull
Copy link
Contributor

I'm going to close this as it's problematic with Mocha's current ability to handle options.

A proper configuration file would be a prerequisite to using multiple reporters, given what we've seen here.

@boneskull boneskull closed this Dec 13, 2017
@graingert
Copy link
Contributor

@boneskull can we revisit this? We've now got mocha-multi from @glenjamin and mocha-multi-reporters from @stanleyhlng

/cc @benvinegar

@corvinrok
Copy link

so we just have to keep on using mocha-multi and hack our way into the future

@boneskull
Copy link
Contributor

if there's a solution that has a good solution for multiplexing and individual reporter options, I'm open to it.

wking added a commit to wking/node-tap that referenced this pull request Jun 8, 2018
This PR depends on tapjs/tap-mocha-reporter#49; review that first.

This is inspired by [Mocha's][1]:

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

Except:

* I'm using a JSON value for easier parsing and more explicit typing.
  This will end up using a few more characters, but the format is more
  explicit than Mocha's (which only uses string values?).  And callers
  are likely to already be familiar with JSON, so we save the mental
  overhead of teaching them a new serialization format.

* I'm using a 'reporter' namespace.  This allows us to add other
  namespaces in the future to address other configurable aspects of
  tap-mocha-reporter's Formatter.  For example, we could use this
  same CLI option to configure the runner.

Somewhat related to this, if we have plans for allowing multiple
reporters [2], we may want to namespace *those* now.  For example:

```
  --reporter-options {"reporter": {"progress": {"open": "(", "close": ")"}}}
```

to avoid the issues Mocha bumped into [3] when trying to add multi-reporter support.

[1]: https://mochajs.org/#usage
[2]: tapjs#335
[3]: mochajs/mocha#2184 (comment)
@ShadSterling
Copy link

@boneskull, my PR to add support for individual reporter options was merged into mocha-multi today: glenjamin/mocha-multi#53

Doesn't look like it's made it to NPM yet, but if you're able to use a git dependency you could try it out.

@hayesgm
Copy link

hayesgm commented Jan 25, 2019

I'm a little confused why this issue was closed due to bad CLI syntax, only to be followed up by new npm modules mocha-multi and mocha-multi-reporters that seemed to have found a solution to that issue. I understand the need to keep the code-base clean, but this is pushing the extra code complexity on to the projects which use mocha. I suppose this is a long-winded way of saying: is there a chance this issue is going to be re-addressed in the future, or was this a permanent closure?

@plroebuck
Copy link
Contributor

Working on plumbing. Once that works and accepted, it'll be much easier to add the multi-reporter capability.

@Bradeskojest
Copy link

Is there anything new regarding this?

@plroebuck
Copy link
Contributor

Not yet.
Working on PR #3874 which will provide file output options for XUnit, TAP, and JSON reporters.
Afterwards, need to standardize reporter options handling.

@michaelseno
Copy link

Can we define several --reporter and --reporterOption inside the mocha.opts file? It seems I'm able to define but the last report is only prevailing. i'm using the mochawesome reporter and also another reporter which is a testrail integration mocha-testrail-reporter. Please let me know if this is possible. Thanks

@ShadSterling
Copy link

@michaelseno, AFAIK using multiple reporters still requires declaring a single reporter that is separately configured to use multiple reporters as an additional step. I was able to use multiple reporters via https://www.npmjs.com/package/mocha-multi

@michaelseno
Copy link

@ShadSterling Could you provide an example for using multiple custom 3rd party reporter using mocha-multi? since I cannot find any reference for using multiple 3rd party reporters for mocha. the one that I can see are the the native reporters only for mocha. Thanks in advance

@ShadSterling
Copy link

ShadSterling commented Jul 17, 2019

IIRC there's no difference in specifying 3rd party vs native. My test case uses this set of reporters(including 3rd party reporters mochawesome and mocha-sonar-reporter), and generates all of the reports when I run npx mocha --reporter mocha-multi --reporter-options mocha-multi=mocha-multi.json build/test. (The package scripts runs mocha through nyc to generate coverage reports, and that command seems to use --reporter for both coverage reporters and test reporters... or something.)

@brettz9
Copy link

brettz9 commented Jan 6, 2020

@plroebuck , if this issue can't be reopened, where would be a good place to subscribe to get updates on this?

@Sayan751
Copy link

Sayan751 commented Feb 6, 2020

Having a custom solution is quite easy, assuming the reporters you want to combine already exist. What I did is to create a small and naive custom reporter, and used the reporter in .mocharc.js config.

// junit-spec-reporter.js
const mocha = require("mocha");
const JUnit = require("mocha-junit-reporter");
const Spec = mocha.reporters.Spec;
const Base = mocha.reporters.Base;

function JunitSpecReporter(runner, options) {
    Base.call(this, runner, options);
    this._junitReporter = new JUnit(runner, options);
    this._specReporter = new Spec(runner, options);
    return this;
}
JunitSpecReporter.prototype.__proto__ = Base.prototype;

module.exports = JunitSpecReporter;
// .mocharc.js
module.exports = {
    reporter: './junit-spec-reporter.js',
    reporterOptions: {
        mochaFile: './tests-results/results.xml'
    }
};

The example above shows how to use both spec and junit reporter. For my purpose it is working pretty good. Not sure whether other reporters can be combined in the same manner.

@ShadSterling
Copy link

@Sayan751, the mocha-multi README gives me the impression that combining multiple reporters isn't always so easy: https://github.com/glenjamin/mocha-multi#user-content-how-it-works

@Sayan751
Copy link

Sayan751 commented Feb 9, 2020

@ShadSterling Well, it is not that difficult as well, is it? 😄

Jokes apart, it might be difficult if you want to combine the reporters generically. But generally when setting up the project you know which reporters are going to be used. For my case it is 'junit' (for ci) and 'spec' (for development). Combining these 2 together, gives me all info on ci log as well. And if you do know the reporter requirements, then combining those is few lines of code (with ES6/TypeScript, it is even fewer). So, I will take that solution over a hacky solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for author waiting on response from OP - more information needed type: feature enhancement proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.