--config does not resolve paths relative to CWD

[cspotcode]

Prerequisites

• 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

.js config loading with --config does not handle relative paths correctly, because it calls require() with a relative path.

Here's the problem: https://github.com/mochajs/mocha/blob/master/lib/cli/config.js#L38

Steps to Reproduce

cd to an empty directory. touch foo.js

mocha --config foo.js

Expected behavior: [What you expect to happen]

mocha loads config from foo.js

Actual behavior: [What actually happens]

/c/Users/cspotcode/Documents/Personal-dev/@mochajs/mocha/lib/cli/config.js:64
    throw new Error(`failed to parse ${filepath}: ${err}`);
    ^

Error: failed to parse foobar.js: Error: Cannot find module 'foo.js'

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

All the time

Versions

Mocha 6.0.2
WSL WLinux bash

Additional Information

Issue #3818

[juergba]

I don't understand why you have opened this issue.
You did realize there is already an open, active one, so please just participate in that one.

cc: @plroebuck

[plroebuck]

Uncertain whether this should be considered a bug or perhaps just in need of additional documentation. It works differently from the other parsers, but not necessarily in a bad way.

Because of implementation's use of require, his example wouldn't be interpreted as a relative path.

Interim workaround:
Always use absolute pathname with JS config files.

@boneskull
Given my druthers, continue to parse as is. Iff require.resolve fails, retry using path.resolve. It's a PITA to debug for user since the error doesn't provide enough information to debug the problem when given an unfound relative path; we should fix this aspect regardless.

[cspotcode]

It makes sense to match the behavior of other tools that support .js config files: webpack, Babel, prettier.

…

On Mon, Mar 11, 2019, 8:55 AM P. Roebuck ***@***.***> wrote: Uncertain whether this should be considered a *bug* or perhaps just in need of additional documentation. It works *differently* from the other parsers, but not necessarily in a bad way. Because of implementation's use of require, his example wouldn't be interpreted <https://nodejs.org/api/modules.html#modules_file_modules> as a relative path. Interim workaround: *Always use absolute pathname with JS config files.* @boneskull <https://github.com/boneskull> Given my druthers, continue to parse as is. Iff <https://en.wikipedia.org/wiki/If_and_only_if> require.resolve fails, retry using path.resolve. It's a PITA to debug for user since the error doesn't provide enough information to debug the problem when given an unfound relative path; we *should* fix this aspect regardless. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3822 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAW-uC_zyKDKc0RQuhSD8tYKd2jqzjeVks5vVlJHgaJpZM4bnl5U> .

[cspotcode]

https://webpack.js.org/configuration/#use-different-config-file

webpack --config webpack.alternate.config.js

https://prettier.io/docs/en/cli.html#find-config-path-and-config

prettier --config ./my/.prettierrc --write ./my/file.js

Relevant babel issue, where the maintainer wants to require a leading ./ but log a helpful message pointing out how people can fix it.
babel/babel#8919

[plroebuck]

I already said:

Given my druthers, continue to parse as is. Iff require.resolve fails, retry using path.resolve.

You have an issue with this approach? It preserves existing behavior (used by existing issue), and provides for local file if that fails...

[cspotcode]

Correct; I disagree with that approach. It differs from the established, more intuitive behavior of other well-known tools that also load .js config files and also accept a path via --config flag.

If mocha's behavior matched webpack, babel, prettier, etc, then #3818 would never have been filed, because @AartiJ initial attempt (supplying a relative path) would have worked the first time. For mocha to intentionally deviate seems unnecessary.

[boneskull]

@juergba what issue are you referring to?

[plroebuck]

Correct; I disagree with that approach. It differs from the established, more intuitive behavior of other well-known tools that also load .js config files and also accept a path via --config flag.

If mocha's behavior matched webpack, babel, prettier, etc, then #3818 would never have been filed, because @AartiJ initial attempt (supplying a relative path) would have worked the first time. For mocha to intentionally deviate seems unnecessary.

It is exactly the same approach used to load reporters, so it's not deviating from anything.
This portion of Mocha is brand new code, and lacks some of the finesse of older portions of
the codebase.

[juergba]

@boneskull

@juergba what issue are you referring to?

Additional Information
Issue #3818

[cspotcode]

In this case, deviation refers to the treatment of other config filetypes, and deviation from other tools in the ecosystem, rather than deviation from reporter loading. Reporters are expected to be libraries; the intuition is different.

Attempting to require relative to node_modules/mocha/lib/cli before attempting path.resolve introduces the possibility that mocha accidentally pulls a config file from node_modules or from within mocha itself, when the user wanted to use a local config. It also requires the docs to become more complicated. Right now they say --config is a "path to config file" which clearly explains an intuitive behavior that is identical for all config file extensions. Clarifying that it may use node's module resolution algorithm, but only if there's an explicit .js extension, just complicates things. It is deviating from node's module resolution algorithm as well, because a user can't publish my-common-mocha-config to npm and specify --config my-common-mocha-config. It'll attempt to parse JSON from CWD.

[plroebuck]

Reporters are expected to be libraries; the intuition is different.

No they're not. You can just as easily load custom reporter from file -- it's done all the time.

Attempting to [SNIP]

Look, the behavior exists per Mocha-6 -- it cannot be reverted without YA major revision. The evaluation order can likely be flipped (cwd-relative, then module-relative), but that's as close as it can get for now.

It is deviating from node's module resolution algorithm as well, because a user can't publish my-common-mocha-config to npm and specify --config my-common-mocha-config. It'll attempt to parse JSON from CWD.

The original issue's resolution demonstrates using a JS configuration file contained in a different npm package.

[chrisgodsey]

It is exactly the same approach used to load reporters, so it's not deviating from anything.

This feels a little disingenuous because if I try to add in arguments like file, or opts, I can use the relation to the current working directy. Heck, if I provide a non .js configuration file it's also based off of cwd.

Look at it from this perspective. I've been running with a .json config file for awhile but a use case comes up where I want conditional logic in my configuration. It would make total sense to me to be able to slap a module.exports around that existing .json file, rename it to .js, change my npm step to account for the file name extension, and fire that bad boy up.

But trying to do that now, it takes you from a perfectly functional test suite to "Cannot find module' error because in changing the file extension you completely changed the file lookup method.

I'd say rather than treating all javascript files the same, you should err on treating all configuration files the same - that way, as @cspotcode said, you don't have to have this convoluted explanation on how the file lookup works different ways with different file extensions.

It seems much easier to say that import mechanisms like reporters and requires - because their typical use case is from node modules, default to looking in your node modules directory - than it is to say that all javascript files are treated in this unique manner... especially when all javascript files aren't treated in this unique manner.