3363: Throw error if unable to parse Mocha options file #3381
Conversation
Code now throws an error for any problems (except nonexistent default "test/mocha.opts"). Includes tests. Fixes mochajs#3363 Fixes mochajs#2576
|
Previous discussion occurred in PR #3376. |
outsideris
added
area: usability
|
Please add @rossgardt as contributor to this PR if it is merged. |
|
@plroebuck There's no way for me to do that, AFAIK. You'll have to cherry-pick or rebase any of @rossgardt's changesets into this branch |
|
@boneskull, you have any issues with the PR? Approve? |
|
You can just re-commit one one of the revisions and add me as co-author.
If that’s too cumbersome, nevermind. As long as we get the changed merged 🙂
Can I close my PR then?
…On Thu 17. May 2018 at 18:54, P. Roebuck ***@***.***> wrote:
@boneskull <https://github.com/boneskull>, you have any issues with the
PR? Approve?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3381 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAOVWJry4pgnLRtqyLFMNhmuaBI8Q-Maks5tzas8gaJpZM4T8gWE>
.
|
|
What is the status of this PR? @plroebuck @boneskull |
Minor correction needed attempting to merge newer modifications to this file with this fix.
prettier, I doth loathe you...
|
the good news is that this PR looks good. the bad news is that #3556 deprecates |
|
How could you possibly have deprecated it completely? It would still at minimum contain Node-related cmdline options, as they would not seem to be appropriate content for ".mocharc.yml". Even if deprecated, your code doesn't incorporate the PR's changes. |
|
mocha.opts is not deprecated. getOptions is |
|
might be able to salvage those tests though |
|
and fwiw most of the churn on the yargs PR involves the fact that .mochajs.yml DOES allow node options, since mocha.opts did. there’s not much practical difference between any of them. |
|
...it was hard to get right. |
I'd have left "mocha.opts" for passing Node flags, and "mocha.yml" strictly for Mocha options, once tranformed to JSON, ready to be passed directly to the ctor. Non-Node flags found in "mocha.opts" would produce a one-time warning to migrate them to "mocha.yml" with list of offending. |
imo this seems like a pain in the butt for users--requiring two config files. if we want users to stop using |
|
I'm going to see what of this I can salvage. |
|
@plroebuck I've squashed this into a single commit, then rebased #3556 onto it; see 924e63c. I'll leave open for now unless there's something that needs to be added to this, but it looks fine to me. I was able to pull it all in, with some modifications of course. |
|
I'm going to close this as it's been merged in 7a3e745. |
Description of the Change
Code now throws an error for any problems loading the Mocha options file (except nonexistent default "test/mocha.opts").
Builds off of previous discussions and work done in PR #2716 and PR #3352 .
Why should this be in core?
Silently ignoring problems loading this file is just wrong.
Benefits
Users will now be told if the file failed to load.
Possible Drawbacks
Users with invalid Mocha options files (or permissions problems, etc.) will need to correct the problem.
Applicable issues
Fixes #3363
Fixes #2576
(server-major) [since Mocha could throw an error when it didn't previously]