Repository: mochajs/mocha

Problem statement:
Migrating from mocha.opts to mocharc broke glob pattern support
### Prerequisites

<details>
  <summary>Click for details</summary>

  - [X] Checked that your issue hasn't already been filed by cross-referencing [issues with the `faq` label](https://github.com/mochajs/mocha/issues?utf8=%E2%9C%93&q=is%3Aissue%20label%3Afaq%20)
  - [X] 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.
  - [X] '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
  - [X] 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.
</details>

---

### Description

- Using `opts configuration`, the [glob pattern](https://globster.xyz/) `{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`](https://github.com/mochajs/mocha/blob/master/lib/cli/options.js#L124) 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 git@github.com:TheOptimisticFactory/mocha-glob-issue.git`
2. 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]

<details>
  <summary>LEGACY configuration file: /test/mocha.opts</summary>

  ```javascript
  --require test/setup.js
  {controllers,test}/**/*.test.js
  --exit
  ```
</details>

`npm run test-legacy-working`

![image](https://user-images.githubusercontent.com/2607260/83260239-ba02f880-a1b9-11ea-871e-2c4619eabd34.png)

---

**Actual behavior:** [What actually happens]

<details>
  <summary>Using .mocharc.js</summary>

  ```javascript
  'use strict';

  module.exports = {
    exit: true,
    require: 'test/setup.js',
    spec: '{controllers,test}/**/*.test.js',
  };
  ```
</details>

`npm run test-bugged-baseline`

![image](https://user-images.githubusercontent.com/2607260/83260600-5deca400-a1ba-11ea-84ed-96c5698d9b01.png)

<details>
  <summary>Using .mocharc.multi-paths.js</summary>

  ```javascript
  'use strict';

  module.exports = {
    exit: true,
    require: 'test/setup.js',
    spec: [ '{controllers,test}/**/*.test.js', 'test/**/*.test.js' ],
  };
  ```
</details>

`npm run test-bugged-showcase`

![image](https://user-images.githubusercontent.com/2607260/83260968-ed925280-a1ba-11ea-9a35-45be078be6ae.png)

---

**Reproduces how often:** [What percentage of the time does it reproduce?]

About 100% of the time :)

---

### Versions

![image](https://user-images.githubusercontent.com/2607260/83262687-b5d8da00-a1bd-11ea-8cf6-c02a16a7278e.png)



Hints:
I confirm, I have the same problem.
@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' ]`
@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.
@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`
@juergba Just as a note, commas are sometimes necessary in forms like this: `{,!(node_modules)/**/}*.js`, and there the workaround doesn't work.
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?

```js
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 just tested, it works with me :+1: 
> 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?
> 
> ```js
> 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](https://github.com/mochajs/mocha/issues/4307#issuecomment-639486379) fixes the issue in all my projects :+1: 
@adjerbetian @TheOptimisticFactory thank you!

Patch:
diff --git a/lib/cli/options.js b/lib/cli/options.js
--- a/lib/cli/options.js
+++ b/lib/cli/options.js
@@ -54,16 +54,19 @@ const configuration = Object.assign({}, YARGS_PARSER_CONFIG, {
 
 /**
  * This is a really fancy way to:
- * - ensure unique values for `array`-type options
- * - use its array's last element for `boolean`/`number`/`string`- options given multiple times
+ * - `array`-type options: ensure unique values and evtl. split comma-delimited lists
+ * - `boolean`/`number`/`string`- options: use last element when given multiple times
  * This is passed as the `coerce` option to `yargs-parser`
  * @private
  * @ignore
  */
+const globOptions = ['spec', 'ignore'];
 const coerceOpts = Object.assign(
   types.array.reduce(
     (acc, arg) =>
-      Object.assign(acc, {[arg]: v => Array.from(new Set(list(v)))}),
+      Object.assign(acc, {
+        [arg]: v => Array.from(new Set(globOptions.includes(arg) ? v : list(v)))
+      }),
     {}
   ),
   types.boolean


Test patch:
diff --git a/test/node-unit/cli/options.spec.js b/test/node-unit/cli/options.spec.js
--- a/test/node-unit/cli/options.spec.js
+++ b/test/node-unit/cli/options.spec.js
@@ -562,7 +562,9 @@ describe('options', function() {
           readFileSync = sandbox.stub();
           readFileSync.onFirstCall().throws();
           findConfig = sandbox.stub().returns('/some/.mocharc.json');
-          loadConfig = sandbox.stub().returns({spec: '*.spec.js'});
+          loadConfig = sandbox
+            .stub()
+            .returns({spec: '{dirA,dirB}/**/*.spec.js'});
           findupSync = sandbox.stub();
           loadOptions = proxyLoadOptions({
             readFileSync,
@@ -573,10 +575,41 @@ describe('options', function() {
           result = loadOptions(['*.test.js']);
         });
 
-        it('should place both into the positional arguments array', function() {
-          expect(result, 'to have property', '_', ['*.test.js', '*.spec.js']);
+        it('should place both - unsplitted - into the positional arguments array', function() {
+          expect(result, 'to have property', '_', [
+            '*.test.js',
+            '{dirA,dirB}/**/*.spec.js'
+          ]);
         });
       });
     });
+
+    describe('"ignore" handling', function() {
+      let result;
+
+      beforeEach(function() {
+        readFileSync = sandbox.stub();
+        readFileSync.onFirstCall().throws();
+        findConfig = sandbox.stub().returns('/some/.mocharc.json');
+        loadConfig = sandbox
+          .stub()
+          .returns({ignore: '{dirA,dirB}/**/*.spec.js'});
+        findupSync = sandbox.stub();
+        loadOptions = proxyLoadOptions({
+          readFileSync,
+          findConfig,
+          loadConfig,
+          findupSync
+        });
+        result = loadOptions(['--ignore', '*.test.js']);
+      });
+
+      it('should not split option values by comma', function() {
+        expect(result, 'to have property', 'ignore', [
+          '*.test.js',
+          '{dirA,dirB}/**/*.spec.js'
+        ]);
+      });
+    });
   });
 });