🚀 Feature: Public equivalent(s) to loadOptions and handleFiles API(s)

[nicojs]

Congrats on the release of version 6. The code cleaned up nicely and the new config files are a big bonus ☕️. I also love the extended api documentation.

We want to support the loading of config files as well in the Stryker mocha runner. My question is: can we rely on the require('mocha/lib/cli/options').loadOptions to load options? Or is it subject to change without a major release? It is documented here: https://mochajs.org/api/module-lib_cli_options.html#.loadOptions, so I would assume that changes to this would be major.

EDIT:
Just as important for us is the handleFilescollect-files method. That one cannot be found in the api docs (yet). Could we make that api public as well?

[nicojs]

I'm noticing that handleFiles calls process.exit when no files could be found. Should this side effect be there? Doesn't feel right to me. It is pretty difficult for Stryker to work with this behavior.

[juergba]

@nicojs refering loadOptions():

• yes, it's a public function and most probably changes would be semver-major.
• In near future we will deprecate to allow comments within .mocharc.json config file. We recently added support for config files with .jsonc extension instead.
• it's not free of bugs, yet. The handling of default values has to be improved, there is an open PR for this topic.
• it's possible that this function does not cover all your needs entirely. After loading (in "/lib/cli.js" and "/lib/run.js") more changes are applied to the parsed options, f.e. yargs.coerce().

[nicojs]

Hi @juergba thanks for your reply ☕️

It's fine to change the implementation details, as long as the API stays the same. We ideally want to load the exact same options as mocha would have, so fixing bugs, loading from different sources, etc is all fine.

There is one change I would like for this function. The first line is let args = parse(argv);. Could I change it to be let args = Array.isArray(argv)? parse(argv): argv; ? That would make it so we can use it directly with parsed arguments. Right now, we're forced to map parsed arguments back to an array of unparsed arguments, which is not ideal.

I would be happy to open a PR for this. I would ofcourse update the docs and add a unit test for it as well.

Shall I open up another issue for the handleFiles request?

[juergba]

@nicojs Just opening some PR's would probably lead to nowhere, I guess. At least at the moment.

Our parse() functions we use (yargs, yargs-parser, our own wrapper) all accept string|string[] as input parameter. Allowing an external object with already parsed flags seems to be inconsistent. We would also loose control about the structure and quality of this object, skipping the error handling of our wrapped parser.

@boneskull cc

[nicojs]

Just opening some PR's would probably lead to nowhere, I guess. At least at the moment

Well that's why we're discussing it here, aren't we? 😅

The more fundamental question is: do you want to support the use of Mocha as a library? We would like to build an awesome experience in Stryker for mocha, but we need some help (although we're willing to do the work).

Our parse() functions we use (yargs, yargs-parser, our own wrapper) all accept string|string[] as input parameter. Allowing an external object with already parsed flags seems to be inconsistent. We would also loose control about the structure and quality of this object, skipping the error handling of our wrapped parser.

I don't really understand this point. Allowing developers using the mocha library to pass along parsed options won't really hinder normal cli use of mocha. I also have trouble understanding that loadOptions is a "parse() function", but maybe I'm missing something here? How about adding a function parseOptions, besides the loadOptions where we accept the string | string[]?

[juergba]

The more fundamental question is: [...]

That is your subjective point of view. I personally try to weight issues according to (Mocha's/my subjective) priorities and also per my personal know-how / interest.

I think you have already merged the feature in question into your repo. So you have found a way to achieve this.

My personal opinion may not represent Mocha's opinion, maybe other members will join the discussion.

[nicojs]

Hi again. It seems that a recent change in Mocha broke our stryker plugin for mocha. For more info see this: stryker-mutator/stryker-js#1693

I would still love for mocha to provide a public api to load options and find files. This is really preventing plugin creators to use Mocha programmatically 😥

[nicojs]

@boneskull what are you're thoughts on this? Would you allow me to write a PR for this?

[nicojs]

Same goes for handleRequires

[boneskull]

Propose something... unsure if just calling the API "public" is what we should be doing. When you do, please show an example use-case.

[nicojs]

Thanks for pitching in, @boneskull . I agree that making internal stuff in run-helpers.js public isn't maintainable in the long run. Maybe it's better to create a new abstraction that supports our use case more directly.

I think the goal is to be able to mimic the mocha cli, but override certain things. This is what Stryker is doing. I think this is also what @jan-molak is looking for.

An example:

const { createMocha, loadOptions } = require('mocha');

const options = await loadOptions();

// Change options, for example:
options.require.push('my-plugin');
options.reporter = myReporter;
options.parallel = false;

// Now create a mocha instance, 
// this will do all nessesary things to load and initialize a mocha instance.
// For example: `handleRequire`, `loadRootHooks` and `collectFiles`.
const mocha = await createMocha(options);

// Now we can still override stuff before a run. For example:
mocha.cleanReferencesAfterRun(false);

// And now decide to run
mocha.run();

If you agree, I can implement it and rewrite some of the bits in the cli directory so the cli would also be using this new abstraction.

[nicojs]

This is what needed to change for Stryker to support mocha 8's rootHooks, while importing from the semi-private run-helpers file.

https://github.com/stryker-mutator/stryker/pull/2259/files

@boneskull Would it be OK if I took a shot at my proposal above?

[boneskull]

something like this, but try to avoid adding node-specific stuff to the default exports. this doesn’t seem applicable to the browser

[nicojs]

@JoshuaKGoldberg I'm still interested in this issue. We are currently depending on these private API's 😅😓😥

loadOptions
collectFiles
handleRequires
loadRootHooks

https://github.com/stryker-mutator/stryker-js/blob/92679e481dbcad9b4a21be7e851c8c06a8b386fa/packages/mocha-runner/src/lib-wrapper.ts#L42-L45