Open dhdaines opened 5 months ago
๐ค I don't reproduce this in https://github.com/mochajs/mocha-examples/tree/4b00891d6c7886f2d451e962a974478e7d3c1aa9/packages/hello-world. After adding an invalid abc
to the top of its package.json
:
$ npm run test
npm ERR! code EJSONPARSE
npm ERR! JSON.parse Invalid package.json: JSONParseError: Unexpected token 'a', "abc
npm ERR! JSON.parse {
npm ERR! JSON.parse "n"... is not valid JSON while parsing 'abc
npm ERR! JSON.parse {
npm ERR! JSON.parse "name": "hello-world",
npm ERR! JSON.parse "versio'
npm ERR! JSON.parse Failed to parse JSON data.
npm ERR! JSON.parse Note: package.json must be actual JSON, not just JavaScript.
Could you post a standalone reproduction please @dhdaines?
Relevant piece of code in mocha:
If one hasn't specified a package.json
explicitly then it simply ignores it when it can't be read, instead only doing a debug()
, so yeah โ it will completely ignore the config in these cases.
@JoshuaKGoldberg Your error might be from npm itself
If one hasn't specified a
package.json
explicitly then it simply ignores it when it can't be read, instead only doing adebug()
, so yeah โ it will completely ignore the config in these cases.
Ah. I suppose the author of the code simply didn't bother distinguishing a non-existent package.json
(in the case where it wasn't explicitly specified) and an invalid one.
Hi maintainers! Thank you for all your hard work, but I don't understand your process for PRs, it seems that you have to mark this issue as "accepting-prs" before I can submit one? As you can see above I have fixed the bug. Please let me know when I can make a PR, thanks again!
it seems that you have to mark this issue as "accepting-prs" before I can submit one
We're discussing if we should drop that
I suppose the author of the code simply didn't bother distinguishing a non-existent
package.json
(in the case where it wasn't explicitly specified) and an invalid one.
It does actually:
const filepath = args.package || findUp.sync(mocharc.package);
if (filepath) {
I think its rather a case of opting between the least bad of two options:
package.json
options when package.json
is brokenpackage.json
is broken even its not used for mocha optionsFor those that have options in their package.json
it would be better if it didn't fail silently. For everyone else it would be worse. And there's no way of knowing that there are options to be expected in the package.json
unless that's explicitly indicated through mocha --package package.json
I think its rather a case of opting between the least bad of two options:
- Silently ignoring
package.json
options whenpackage.json
is broken- Inexplicably fail when
package.json
is broken even its not used for mocha optionsFor those that have options in their
package.json
it would be better if it didn't fail silently. For everyone else it would be worse. And there's no way of knowing that there are options to be expected in thepackage.json
unless that's explicitly indicated throughmocha --package package.json
Ah, I understand. I would be inclined to think that if your package.json
is broken then you will also have other problems, and you might want to fix that :-) How common is it for people to configure mocha via package.json
versus .mocharc
(and I guess there are a few other ways to do it)? The documentation seems to encourage using a .mocharc
instead.
Perhaps the debug log message could be changed to a warning?
Repeating from https://github.com/mochajs/mocha/issues/5141#issuecomment-2206519931 ๐: Could you post a standalone reproduction please @dhdaines? (or anybody else?)
process
We can't accept a PR unless we fully understand the problem. We can't fully understand the problem unless we can reproduce it. And I haven't figured out how to reproduce it. Hence the status: waiting for author
label.
For anybody reading this wondering why I'm being such a stickler (๐ ): https://antfu.me/posts/why-reproductions-are-required is a big deep dive into it. https://antfu.me/posts/why-reproductions-are-required#how-to-create-a-minimal-reproduction specifically talks about a minimum reproduction, which is what I'm asking for here.
We can't accept a PR unless we fully understand the problem. We can't fully understand the problem unless we can reproduce it. And I haven't figured out how to reproduce it. Hence the
status: waiting for author
label.
Ah okay! I didn't realize that's what "waiting for author" meant. Happy to provide a reproduction, will do so in the next few minutes.
Here you go. Note that it clarifies the NPM error you got above, which comes from NPM. You have to run mocha
directly to see the problem.
Aha! Thanks, that's exactly what I was looking for. Perfect. Agreed on the bug, this is definitely an edge case Mocha should handle better as you've described. ๐ accepting pull requests!
Aha! Thanks, that's exactly what I was looking for. Perfect. Agreed on the bug, this is definitely an edge case Mocha should handle better as you've described. ๐ accepting pull requests!
Thank you and sorry for the confusion!
Perhaps the debug log message could be changed to a warning?
I like this suggestion. Something warning that the package.gain could not be checked for config (ensuring it only triggers if thereโs no other config found either)
Bug Report Checklist
faq
label, but none matched my issue.Expected
I made an error in my
package.json
(an extra comma) because JSON is a bad format for configuration files ;-) like this:I expected
mocha
to give me an error, ideally a bit friendlier than the one fromnpm
, but something like that:Actual
Mocha just failed as if I hadn't configured it at all. This was very confusing:
Minimal, Reproducible Example
See "expected" above but basically:
then add the offending section to
package.json
, then createfoo.spec.js
, then try to runmocha
.Versions
10.4.0 10.4.0 v20.11.0
Additional Info
I could definitely provide a PR if you could direct me to the appropriate place in the code (with which I am not familiar)