fix extension and spec handling; closes #3808

[boneskull]

Also fixes some issues in the options unit tests and adds coverage around spec handling.

This needs some manual testing, as I had to monkey with watch a bit.

cc @dedene

[plroebuck]

The only reason this was here was to allow the historical usage of "test.js" via the default spec "test". IMO, we should not propagate this further.

The "Handle directory" code below already handles extensions for "test/*".

[plroebuck]

const rcDefaults = Object.freeze(require('../mocharc.json'));

[plroebuck]

Rebase

[juergba]

I think there is a standard yargs way to achieve this, using the "default" option. I just did some short testing, nevertheless it seems to work.

// line 109
const result = yargsParser.detailed(args, {
    default: {extension: ["js"]},      // set default values with "mocharc"
    configuration,
    configObjects,
    coerce: coerceOpts,
    narg: nargOpts,
    alias: aliases,
    string: types.string,
    array: types.array,
    number: types.number,
    boolean: types.boolean.concat(nodeArgs.map(pair => pair[0]))
  });

// line 326
args = parse(
    args._,
    args,
    rcConfig || {},
    pkgConfig || {},
    optsConfig || {},
    //mocharc                           don't set default values here
  );

[boneskull]

@juergba Hmm, you're right.

I think instead of this strategy, I'll change it so that we can still keep the default config in our lib/mocharc.json, but use its values as defaults instead of just another config object which is merged in.

[juergba]

Also in "lib/cli/run-helpers.js" the default extension of "js" is set hardcoded several times.
i.e. watchRun() or runMocha().

[juergba]

@boneskull is it ok for you, if I work on this one?

[coveralls]

Coverage increased (+0.08%) to 91.819% when pulling 2d1dc34 on boneskull/issue/3808 into a4f1a44 on master.

[boneskull]

@juergba yes, go ahead

[plroebuck]

@juergba second time now you merged without any review (despite prior request to do otherwise) and this time you introduced bug...

[plroebuck]

Corrected code...

"lib/utils.js"

/**
 * Lookup file names at the given `path`.
 *
 * @description
 * Filenames are returned in _traversal_ order by the OS/filesystem.
 * **Make no assumption that the names will be sorted in any fashion.**
 *
 * @public
 * @memberof Mocha.utils
 * @param {string} filepath - Base path to start searching from.
 * @param {string[]} [extensions=[]] - File extensions to look for.
 * @param {boolean} [recursive=false] - Whether to recurse into subdirectories.
 * @return {string[]} An array of file pathnames.
 * @throws {Error} if no files match pattern.
 * @throws {TypeError} if `filepath` is directory and `extensions` not provided or empty.
 */
exports.lookupFiles = function lookupFiles(filepath, extensions, recursive) {
  var files = [];
  var stat;

  if (extensions === undefined) {
    extensions = [];
  }
  if (recursive === undefined) {
    recursive = false;
  }

  if (!fs.existsSync(filepath)) {
    // Check all extensions, using first match found (if any)
    if (
      !(extensions.some(function(ext) {
        var filepathWithExtname = filepath + '.' + ext;
        if (fs.existsSync(filepathWithExtname)) {
          filepath = filepathWithExtname;
          return true;
        }
      })
    ) {
      // Handle glob
      files = glob.sync(filepath);
      if (!files.length) {
        throw createNoFilesMatchPatternError(
          'Cannot find any files matching pattern ' + exports.dQuote(filepath),
          filepath
        );
      }
      return files;
    }
  }

  // ...no further changes to merged code...
}

[juergba]

@plroebuck
Yes, without @boneskull contributions to "needs-review"-PRs have been close to null during the last few weeks. There is no one stepping into his gap, that's worrying.

you introduced bug... can you be more ... specific?

corrected code: this is the solution @boneskull already proposed, so nothing new. It only uses the first match of filepath + '.' + ext and ignores the rest of matches. I wonder why this should be correct.

[plroebuck]

As I commented originally in his PR, the part of the code that added ".js" should probably have been left alone as an anachronism; it allows "test.js" to satisfy Mocha's default, rather than just putting spec files in a "test" directory. But filepath (in file or directory mode) cannot have multiple names, which is why .some() is used. You moved the return (intended solely for glob handling) and bypassed the remaining code in the function. But that code you skipped over provided handling of directories and ensured that only files were returned.

To bust your PR, create a directory named "test.js" and run lookupFiles (with extensions=['js']).
It now brokenly returns a directory.

Please use the corrected code above to fix this.

[plroebuck]

Yes, without @boneskull contributions to "needs-review"-PRs have been close to null during the last few weeks. There is no one stepping into his gap, that's worrying.

True, but almost no PRs here are ever critical; they can sit until needed. Create your PR and add "NeedsReview" tag when ready; make sure and add reviewers (especially ones who might actually do so). Then give it a week. If no one reviewed it by that point, make comment on the PR announcing intent to merge within n days (2 would be nice). If still no review and/or comment after deadline, then merge.

[juergba]

As I commented originally in his PR [...]

You are right, a directory named "test.js" will fail. I will open a PR and use your proposition. I don't agree completely, though. With two files (eg. "unit.ts" and "unit.js") a call "unit" plus extension: ['ts', 'js'] should return both files, not just some/the first one.
I will also have to check wether the glob always returns files.

EDIT: the glob returns files and directories, so this function has never really worked correctly. The directories could be skipped with option.nodir: true

see #3905

True, but almost no PRs here [...]

I can live with that procedure.

[plroebuck]

You are right, a directory named "test.js" will fail. I will open a PR and use your proposition. I don't agree completely, though. With two files (eg. "unit.ts" and "unit.js") a call "unit" plus extension: ['ts', 'js'] should return both files, not just some/the first one.

The filepath + '.js' hack was never intended to handle what you're trying to do with it. Its purpose was to allow small packages that only exported a couple functions (typically from "index.js") to use "test.js" to test them.

If a package is exporting functions in more than one language, its specifications should live in the "test" directory -- the simplistic specification scheme above shouldn't apply.