split cli parsing

[rijnhard]

Hi

Firstly I know this is large, and I didn't open an issue for it, but bare with me. I firstly didn't know if it was possible, and secondly the proposed change is very difficult to discuss without digging into the rough stuff and seeing what it affects. So I did the unthinkable and coded it anyway. yolo

Goals

Before I get to the feature/refactor itself I'd like to note the following goals i tried to keep.

• NO changes to existing behaviour (like none, users shouldn't even notice anything)
• increase maintainability of the CLI.
• make parts of the CLI reusable at the discretion of users.
• Comment everything to make design choices obvious.
• All existing tests must pass without changes

Feature/Refactor?

So what is this? I took the CLI (namely bin/mocha and bin/_mocha) and separated the code into easier to digest chunks without affecting how it's used.

The issue I ran into was that using mocha programmatically with a custom runner (via the Mocha interface) you lose a lot of convenience and built in functionality, namely all the CLI commands and the mocha.opts file parsing.

It's not hard to understand the value of using mocha.opts or the CLI arguments when using your own runner, yet that logic was so baked into the cli code a user had no choice but to reimplement everything he wanted.

Limitations

I tried not to take any liberties but some were inescapable (and trust me I played with many design options that all retained current functionality).

1. In order to keep the api simple i had to override Command.parseOptions from Commander
2. I force using a mocha helper to make the Commander program (namely in cli.makeCommand)

How it works

To see how it works just look at the new _mocha file.
In cli.makeCommand in the overridden Command.parseOptions function I handle all the argument expansion that was in bin/mocha and all the --opts parsing that was scattered. By doing this users (and you) don't have to give a damn about --opts it Just Works and it works in the same way it did before, with one caveat.
I tore out all the commander overloading and now theres a cli.parseArgv function that handles the parsing instead of calling Command.parse. Since the CLI is no longer going to be exposed.

I don't mutate process.argv directly.

This gives numerous benefits like making it easy to create multiple CLI command instances, it makes it more obvious what is affecting the args instead of the disparate mutation of process.argv, and it essentially is another guard against double expanding --opts which would now take a talented monkey to screw up.

Function names

All of them can be changed, I named them based on what the chunk of code I relocated from the original CLI did, since I didn't change that behaviour, just moved it around.

exposed CLI

I did expose the CLI to users, but this can easily be removed. I dont mind for my purposes using internal parts of mocha that I shouldn't.

Outstanding issues

• test/color.spec.js currently fails, and I have no idea why or how to debug this
• test/cli/cli.spec.js has a few tests that I could use help trying to figure out

Again sorry for the big no issue PR, but since it doesn't change existing functionality (especially if you don't expose the CLI) I'm hoping it's less of an issue.

[rijnhard]

there is something that does worry me.
most plugins or config.js files modify global and if we have multiple CLI instances with different requires then they will pollute the global scope and could collide.

Mocha by default behaves like this anyway, so I dont think it needs be fixed in this PR. But it could be problematic with an exposed CLI

[rijnhard]

So to update this PR is discussed here #2578

Because of numerous limitations in Commander I abandoned the idea to make the Cli Reusable.
Issues were mainly around how to parse options and circumvent commander issues such as tj/commander.js#584 and handle mocha's parameter expansion using the mocha.opts file.
A key use case in a reusable Cli with commander would be to use the Mocha Cli in a subcommand, which is not possible without out serious hackery due to how mocha parses (it always uses the base Command's functions instead of the subcommands' which I was overriding.

Instead I tore out the hackery and unexposed the Cli, so now it's just a code refactor.

[ScottFreeCode]

Because of numerous limitations in Commander I abandoned the idea to make the Cli Reusable.

Hmm, I actually hope this comes back (perhaps after we move away from Commander? there's rumblings of trouble with Commander elsewhere: #2184). I've warmed up to the idea a lot. I'm not sure how much third-party tooling relies on the CLI, but I ran into another project in the wild that was manipulating Mocha via the programmatic API to augment its capabilities or run it in another environment (it seemed to be a competitor to Karma if I understood correct), but was itself a commandline tool, and after poking around at an issue for a while I thought, "Well, it's quite silly that this is reimplementing half of Mocha's commandline logic and leaving out the other half altogether."

So I'm going to have to set aside some time and see how the changes went here!

[rijnhard]

@ScottFreeCode thank you, and remember I'd be willing to help with implementation as this is core part of one of our products, but just to let you know regardless of the CLI not being exposed, we are still using my branch in our project. And it still works. I found a way around how to make the command a subcommand as well (excuse the es6)

I basically just reimplemented the new _mocha script in the action, which I dont think is too bad, but its not elegent.

import program from 'commander';
import * as MochaCli from 'mocha/lib/cli/cli';

const mochaCommand = program.command('mocha');
addMyOptions(mochaCommand);

mochaCommand
    .description('Straight passthrough to mocha runner\n'
                 + '\tJust with the config file already required for the test suite.\n'
                 + '\tAnd additional custom options added.\n'
                 + '\tUsage: my-test mocha --help'
    )
    .allowUnknownOption(true)
    .action(() => {
        const mocha = new Mocha(),
            newArgs = stripMyOptions(
                program.rawArgs // cuts out the subcommand
                    .slice(0, 2)
                    .concat(program.rawArgs.slice(3))
            ),
            subMocha = MochaCli.makeCommand();

        MochaCli.addActions(subMocha);
        MochaCli.parseArgv(newArgs);

        const opts = subMocha._opts(), // commander bug https://github.com/tj/commander.js/issues/584
              specs = MochaCli.resolveFiles(subMocha.args, opts);

        // set options in mocha
        MochaCli.setMochaOptions(mocha, opts);

        try {
            MochaCli.run(mocha, subMocha, specs);
        } catch (e) {
            FrameworkLogger.error(e.message);
            process.exit(1);
        }
    })
    .on('--help', () => {
        const subMocha = MochaCli.makeCommand();

        MochaCli.addActions(subMocha);

        const mochaHelp = subMocha.helpInformation();

        // commander is retarded and outputs the default help anyway
        process.stdout.write(mochaHelp.replace('Options:', 'Mocha Options:'));
    });

[jsf-clabot]

All committers have signed the CLA.

[boneskull]

hi, this is going to need to be rebased, and the CLA system is not finding your email...

[rijnhard]

I'm happy to update it, but stalled last time, I just don't want to waste my effort. We are using it so it does work

…

On Tue, 9 Jan 2018, 06:11 Christopher Hiller, ***@***.***> wrote: hi, this is going to need to be rebased, and the CLA system is not finding your email... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2553 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEF36AH4lNXo8is6EGyMY8hkN6FBYpg4ks5tIubZgaJpZM4KgOrl> .

[rijnhard]

@boneskull CLA signed. I am willing to update this if required.
I should reiterate though that I do think Commander is very annoying due to some of its limitations, as mentioned in the PR. Honestly its just not that composable. However, this is workable and a decent start.

[boneskull]

I don't think we're going to get anywhere with this. I hate commander too. happy to accept a complete rewrite of the options parsing w/ yargs or something. sorry this dragged on forever.

[rijnhard]

There's a chance I'll do that in the not too distant future but no promises.

…

On Sun, 8 Apr 2018, 00:59 Christopher Hiller, ***@***.***> wrote: Closed #2553 <#2553>. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2553 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEF36Prd5PgUNhhKUepPDuFPex0z4kn2ks5tmUTNgaJpZM4KgOrl> .