Migrating from mocha.opts to mocharc broke glob pattern support

[TheOptimisticFactory]

Prerequisites

Click for details

• Checked that your issue hasn't already been filed by cross-referencing issues with the faq label
• Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
• 'Smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, your usage of Mocha, or Mocha itself
• Ensured that there is no discrepancy between the locally and globally installed versions of Mocha. You can find them with: node node_modules/.bin/mocha --version(Local) and mocha --version(Global). We recommend that you not install Mocha globally.

---

Description

• Using opts configuration, the glob pattern {controllers,test}/**/*.test.js used to expand into:

  • controllers/**/*.test.js
  • test/**/*.test.js

• Using RC configration files, this glob partten gets tokenized into invalid ones:

[mocha/lib/cli/options.js] > parse() > yargsParser.detailed() > result.argv.spec [ '{controllers', 'test}/**/*.test.js' ]

Warning: Cannot find any files matching pattern "{controllers"
Warning: Cannot find any files matching pattern "test}/**/*.test.js"

• Commenting the yargs parsing option coerce: coerceOpts fixes the glob tokenization, even though it isn't a long-term fix.
• The coerceOtps function can be found here: https://github.com/mochajs/mocha/blob/master/lib/cli/options.js#L63

As a side I should add that these two following spec values give the results highlighted above

• spec: '{controllers,test}/**/*.test.js',
• spec: [ '{controllers,test}/**/*.test.js' ],

---

Steps to Reproduce

I created a barebone repository to highlight the issue: https://github.com/TheOptimisticFactory/mocha-glob-issue

1. Open a terminal
2. Clone the demo project using git clone [email protected]:TheOptimisticFactory/mocha-glob-issue.git
3. Install packages using cd mocha-glob-issue && npm i
4. Run any of the following scripts:

• npm run test-legacy-working: Check tests passes when using lecacy opts test/mocha.opts
• npm run test-bugged-baseline: tests WONT BE FOUND when using .mocharc.js
• npm run test-bugged-showcase: Dumps the BROKEN file patterns when using .mocha.multi-paths.js

---

LEGACY behavior: [What used to happen]

LEGACY configuration file: /test/mocha.opts

--require test/setup.js
{controllers,test}/**/*.test.js
--exit

npm run test-legacy-working

---

Actual behavior: [What actually happens]

Using .mocharc.js

'use strict';

module.exports = {
  exit: true,
  require: 'test/setup.js',
  spec: '{controllers,test}/**/*.test.js',
};

npm run test-bugged-baseline

Using .mocharc.multi-paths.js

'use strict';

module.exports = {
  exit: true,
  require: 'test/setup.js',
  spec: [ '{controllers,test}/**/*.test.js', 'test/**/*.test.js' ],
};

npm run test-bugged-showcase

---

Reproduces how often: [What percentage of the time does it reproduce?]

About 100% of the time :)

---

Versions

[adjerbetian]

I confirm, I have the same problem.

[juergba]

@TheOptimisticFactory thank you for your detailed description.
I haven't tested, but I guess you are correct.

It's the list function which splits the parsed string by the , separator.
As a work around you can use: spec: [ 'controllers/**/*.test.js', 'test/**/*.test.js' ]

[rgroothuijsen]

@juergba From the looks of it, one possible fix would be to do an additional globbing pass over the raw spec values (thus performing brace expansion if necessary) before parsing them any further.

[juergba]

@rgroothuijsen I don't know. I don't really like the idea to have two glob expansion steps in two different places. There could be more comma-delimited glob patterns than just brace expansion?

spec is kind of a bastard option form. In our first parsing step by yargs-parser, it's an option. In our second parsing step by yargs, it's a positional argument.
We could disallow comma-delimited lists in spec:

• ok: node mocha test1 test2
• ok?: node mocha -- test1 test2
• ok: node mocha --spec test1 --spec test2
• ok, but disallow?: node mocha --spec test1,test2

[adjerbetian]

@juergba Just as a note, commas are sometimes necessary in forms like this: {,!(node_modules)/**/}*.js, and there the workaround doesn't work.

[juergba]

IMO the best way to solve this, is to remove the comma splitting for options which could contain glob patterns: --spec and --ignore.
As per yargs configuration comma-delimited lists are not customized for these two option/positional argument anyway.

@TheOptimisticFactory @adjerbetian if you have some time left ...
Could you check wether the following patch in lib/cli/option.js is working, please?

const globOptions = ['spec', 'ignore'];   // new
const coerceOpts = Object.assign(
  types.array.reduce(
    (acc, arg) =>
      Object.assign(acc, {
        [arg]: v => Array.from(new Set(globOptions.includes(arg) ? v : list(v)))   // modified
      }),
    {}
  ),
 ....

[adjerbetian]

@juergba I just tested, it works with me 👍

[TheOptimisticFactory]

IMO the best way to solve this, is to remove the comma splitting for options which could contain glob patterns: --spec and --ignore.
As per yargs configuration comma-delimited lists are not customized for these two option/positional argument anyway.

@TheOptimisticFactory @adjerbetian if you have some time left ...
Could you check wether the following patch in lib/cli/option.js is working, please?

const globOptions = ['spec', 'ignore'];   // new
const coerceOpts = Object.assign(
  types.array.reduce(
    (acc, arg) =>
      Object.assign(acc, {
        [arg]: v => Array.from(new Set(globOptions.includes(arg) ? v : list(v)))   // modified
      }),
    {}
  ),
 ....

@juergba I also confirm your diff fixes the issue in all my projects 👍

[juergba]

@adjerbetian @TheOptimisticFactory thank you!