🚀 Feature: Make reporter options standard

[plroebuck]

Description

1. Make reporterOptions a standard object within Mocha.options.
2. Reparent the following Mocha.options as reporterOptions properties:
   • useColors
   • useInlineDiffs
   • hideDiff
3. Update all reporter constructors to accept second argument options.
4. Update Mocha.run() to pass options.reporterOptions to reporter constructor. Currently passes options, but most Mocha option properties are not applicable/unused by reporter.
   • Update XUnit reporter constructor to expect options.reporterOptions
5. Migrate Base reporter settings portion of Mocha.run() to Base reporter constructor (or somewhere more related).

[plroebuck]

Perhaps simpler to have a --settings (JSON?) configuration file used with Mocha ctor. It would be applied prior to "mocha.opts" processing (so cmdline still overrides).

An extension of the settings file would turn options.reporterOptions into an ever-present part of options. Each specific reporter would have its own options object, referenced by its name.
Prior to reporter instance creation, the options object passed to Base ctor would be a meld of shared defaults, reporter defaults from settings file, and cmdline overrides (underrides?).

settings = {
  reporterOptions: {
    _augment: {
      //useColors
      //useInlineDiffs
      //hideDiff
      //suiteName
    },
    _override: {},
    spec: {
      stdout: '-'  // console
    },
    xunit: {
      stdout: '/path/to/xunit.xml'
    },

    // other reporters
  },
  timeout: 75,
  // etc.
}

[johanblumenberg]

I think it is a good idea to add a structure that allows to specify options to individual reporters, and allow for multiple reporters.

It might also be a good idea to only pass the specific reporters options to a reporter when constructing it. Maybe even take it one step further and only pass options.reporterOptions.xunit to the xunit constructor. There is no need to pass options for all reporters to all reporters.
A drawback could be that you don't get the mocha options. You might want to output something like 'Test failed due to timeout. The test took ${time} ms, and the timeout is ${options.timeout}'. But maybe you can get those things, and others, from the runner object passed to the constructor. That would probably be a better solution, that also allows you to get properties that are not from cmdline arguments.

I don't think it is a good idea to have a set of common options to all reporters. For example useColor doesn't make sense to xunit, because you are typically writing xml to a file for some CI system to read. It only makes sense for the single reporter that you would have writing to stdout, all other reporters should have colors off.

[plroebuck]

@johanblumenberg wrote:

It might also be a good idea to only pass the specific reporters options to a reporter when
constructing it. Maybe even take it one step further and only pass options.reporterOptions.xunit
to the xunit constructor. There is no need to pass options for all reporters to all reporters.

I thought I had said that, but not explicitly enough. Yes, exactly.
This simplifies the reporter code since it would be unnecessary to verify the intermediate data structures before accessing an option (e.g., from inside Spec reporter [given the above settings],
it would just be options.stdout).

A drawback could be that you don't get the mocha options. You might want to output something
like 'Test failed due to timeout. The test took ${time} ms, and the timeout is ${options.timeout}'.
But maybe you can get those things, and others, from the runner object passed to the
constructor. That would probably be a better solution, that also allows you to get properties that
are not from cmdline arguments.

As you said, you could grab those from Runner.options if actually needed.
Haven't seen it IRL.

I don't think it is a good idea to have a set of common options to all reporters. For example
useColor doesn't make sense to xunit, because you are typically writing xml to a file
for some CI system to read.

Poorly named. Maybe better would be to have _override (applied regardless) and _augment (applied if not specified). These were basically the reparented Mocha.options.
useColors would belong in _augment, whereas maybe the diff options in _override.

It only makes sense for the single reporter that you would have writing to stdout,
all other reporters should have colors off.

I was of two minds here since you might want colors in your output. If you really wanted to save your Nyan Cat output... (does anyone actually use that reporter?) Could possibly see someone wanting to save the colorized spec output.

[johanblumenberg]

I thought I had said that, but not explicitly enough. Yes, exactly.

Ok, then I misread. Cool.

Although it would be difficult to change the reporter constructor arguments without a new major, and a rewrite of all reporters...

Poorly named. Maybe better would be to have _override (applied regardless) and _augment (applied if not specified).

Ok, I get your point.

Could possibly see someone wanting to save the colorized spec output.

Yes, that's true.

It should be configurable per reporter then anyway. But if I understand your proposal above correct, you are actually proposing that it would be configurable per reporter, but if one wants to have it the same for all reporters, one could add it to the _override object instead.

[plroebuck]

Although it would be difficult to change the reporter constructor arguments without a new major, > and a rewrite of all reporters...

Self-referencing. I know it's ugly, but backwards compatible!

reporterOptions = {
  foo: "bar",
  reporterOptions: undefined
};
reporterOptions.reporterOptions = reporterOptions;   // [Circular]
reporterOptions.foo === reporterOptions.reporterOptions.foo

[plroebuck]

Assuming cmdline processing of --reporter-options had been applied, something like this:

reporterOptions = _.merge({ reporterOptions: undefined },
  settings.reporterOptions._augment,
  settings.reporterOptions[options.reporter],
  settings.reporterOptions._override);
reporterOptions.reporterOptions = reporterOptions;

[johanblumenberg]

@plroebuck Yes, that might work

[plroebuck]

Related discussion in #3487

[brian-lagerman]

This is pretty interesting for us too. We recently kicked out a beta version of Mochaterial, an ES6 reporter for the browser, and I ran into some issues with the reporterOptions configuration.

The main issue was that reporterOptions isn't accepted at all through browser entry, as passing the reporterOptions object just blows up. #3068 is a reference issue of this. I got around it by adding this into the reporter, but it sure looks and feels like a monkey hack. I'm not sure if this rises to the level of a bug, but it sure would be nice if any reporterOptions refactor takes browser entry into account. (Also note that we were running mocha 5.2 when we stumbled on this, and haven't yet confirmed if 6 still has this issue)

The less pressing issue is just general clarity. For example, our reporter supports line-by-line or side-by-side diffs, so the presence of --inline-diffs could get confusing. At the same time, maybe that's just a documentation issue, as it can't be expected that all reporters are going use/support all options.

[JoshuaKGoldberg]

Per #5027, we're trying to avoid making big changes that don't have a lot user demand. This hasn't been touched since 2019 and would be a breaking change to reporter options, I think. Closing out as wontfix.

If someone things I'm wrong to close this out, great! Please file a new issue with your reasoning and a link to this one. The new issue templates will prompt for the context we'd need to be able to re-triage this. We'd be happy to take a look. Cheers all! 🤎