Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix --config loading: cwd-relative, fallback to require() #3862

Closed
wants to merge 1 commit into from

Conversation

cspotcode
Copy link
Contributor

@cspotcode cspotcode commented Apr 5, 2019

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions.

Description of the Change

Fixes #3822.

--config is first interpreted as a cwd-relative path. We check if it exists and is a file. (not directory)
If found, we try to parse it according to its file extension. (this behavior is unchanged)
If not found, we attempt to require.resolve() it relative to cwd.
If require.resolve() is able to locate a file, we require() it. We do not check file extension nor use a custom loader.

Error messages are logged for a) failing to locate a file in the first place and b) failing to parse or load the file that was found. For b) the error message logs the resolved path, so users can see that we found e.g. ../../node_modules/monorepo-shared-config/mocha.js.


Worth noting, cwd may not always be the correct directory for resolving. For example, if in the future the --config value comes from a package.json file -- "mocha": "@cspotcode/my-mocha-config" -- then we should resolve relative to the directory containing package.json, not necessarily cwd.

Alternate Designs

Why should this be in core?

Benefits

Possible Drawbacks

Applicable issues

#3822
#3823
#3829

@coveralls
Copy link

coveralls commented Apr 5, 2019

Coverage Status

Coverage increased (+0.03%) to 91.738% when pulling 523dcac on cspotcode:fix-config-loading into b079d24 on mochajs:master.

@cspotcode
Copy link
Contributor Author

Added tests.

@outsideris outsideris added the area: usability concerning user experience or interface label Apr 20, 2019
@juergba
Copy link
Member

juergba commented Feb 17, 2021

The --config loading should be identical for all RC file extensions, so the status-quo doesn't make sense. You are right.
@cspotcode are you still interested in this PR?

We could try to get this one in somehow. Since lib/utils.js is part of the browser bundle, we should not put your helper function into this module.

@cspotcode
Copy link
Contributor Author

IIRC the discussions for #3822, #3823 and #3829 left a bad taste in my mouth, so I'd rather someone else finish this one.

@juergba
Copy link
Member

juergba commented Feb 17, 2021

@cspotcode ok. Our team has changed meanwhile, but thanks anyway. I remember times when I was sweating blood because of this Windows CRLF publishing story.
Closing.

@juergba juergba closed this Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: usability concerning user experience or interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--config does not resolve paths relative to CWD
4 participants