fix timeout/slow string values and duplicate arguments

[boneskull]

This fixes two problems:

1. Duplicate values are no longer allowed on the command line, and an error will be thrown (e.g., --timeout 0 --timeout 0)
2. String values of timeout and slow (e.g., 1s, 500ms) are no longer evaluated to NaN (because their type was set to number). It appears that a value of NaN would cause timeouts to be disabled.

Ref: #3817

UPDATE: Duplicate arguments are now allowed; the last value is chosen.

[coveralls]

Coverage increased (+0.003%) to 91.822% when pulling 6c8b7e1 on boneskull/timeout-fixes into 94033bf on master.

[plroebuck]

Duplicate values are no longer allowed on the command line, and an error will be thrown (e.g., --timeout 0 --timeout 0)

This is a completely valid cmdline argument usage -- last value wins. It's not an error.

[juergba]

This is a completely valid cmdline argument usage -- last value wins. It's not an error.

Agree. Sometimes it's even necessary to overwrite flags, i.e. ones set by IDE's.

[boneskull]

This is a completely valid cmdline argument usage -- last value wins. It's not an error.

Hm, ok. Assuming the array yargs-parser gives us is in order, then I can just pop it.

[boneskull]

@juergba @plroebuck I've changed this to use the last value instead of throwing an exception.

[juergba]

LGTM, except the duplicate test already mentioned.

(there can still be duplicate arguments within one single config file)

[plroebuck]

Said this before and provided code for it.
This should be run for all arguments expecting a value, not just --ui.

[juergba]

I haven't understood this "yargs" / "yargs-parser" thing completely, yet.

• why can't "yargs-parser" use the default values defined in lib/cli/run.js
• couldn't we use the duplicate-arguments-array option to reduce duplicate arguments?

var parse = require('yargs-parser');

var args = parse(
    ['--timeout', '2s', '--timeout', '1s', '--timeout', '800'], {
        string: ['timeout'],
        configuration: {
            'duplicate-arguments-array': false,
            'camel-case-expansion': false
        }
});
console.log(args);         // { _: [], timeout: '800' }

[juergba]

This part does not work correctly:

• input per CLI: --slow 5 --slow 10
• result after parsing: s: ["5"], slow: "10"

[juergba]

There is a bug in "yargs-parser" where the coerce function returns a value of type number as string. I opened an issue for that.

This affects the parsing of our --retries option and I had to adapt some of our tests. Nevertheless there are no implications since Yargs#parse (without coerce function) converts the value back to number.

Ready for review.

[juergba]

I have chosen yargs-parser#coerce() in order to reduce duplicate arguments:

• it reduces the flags incl. all its aliases
• it is part of loadOptions() which is a public function. This is not the case with yargs.coerce(). Arguments should be reduced when a third party package depends on Mocha's loadOptions().

outdated

[plroebuck]

Upstream fix for yargs-parser unnecessary then?

[juergba]

For our purposes, yes unnecessary.

[juergba]

I will merge these changes tomorrow.