🐛 Bug: mocha fails silently on invalid package.json section

[dhdaines]

Bug Report Checklist

• I have read and agree to Mocha's Code of Conduct and Contributing Guidelines
• I have searched for related issues and issues with the faq label, but none matched my issue.
• I have '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, my usage of Mocha, or Mocha itself.
• I want to provide a PR to resolve this

Expected

I made an error in my package.json (an extra comma) because JSON is a bad format for configuration files ;-) like this:

"mocha": {
    "spec": "./*.spec.js",
}

I expected mocha to give me an error, ideally a bit friendlier than the one from npm, but something like that:

$ npm install
npm ERR! code EJSONPARSE
...
npm ERR! JSON.parse Note: package.json must be actual JSON, not just JavaScript.

Actual

Mocha just failed as if I hadn't configured it at all. This was very confusing:

$ npx mocha
Error: No test files found: "test"

Minimal, Reproducible Example

See "expected" above but basically:

npm install -D mocha

then add the offending section to package.json, then create foo.spec.js, then try to run mocha.

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)

[JoshuaKGoldberg]

🤔 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?

[voxpelli]

Relevant piece of code in mocha:

mocha/lib/cli/options.js

Lines 182 to 201 in c44653a

	const filepath = args.package || findUp.sync(mocharc.package);
	if (filepath) {
	try {
	const pkg = JSON.parse(fs.readFileSync(filepath, 'utf8'));
	if (pkg.mocha) {
	debug('`mocha` prop of package.json parsed: %O', pkg.mocha);
	result = pkg.mocha;
	} else {
	debug('no config found in %s', filepath);
	}
	} catch (err) {
	if (args.package) {
	throw createUnparsableFileError(
	`Unable to read/parse ${filepath}: ${err}`,
	filepath
	);
	}
	debug('failed to read default package.json at %s; ignoring', filepath);
	}
	}

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

[dhdaines]

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.

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.

[dhdaines]

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!

[voxpelli]

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:

• Silently ignoring package.json options when package.json is broken
• Inexplicably fail when package.json is broken even its not used for mocha options

For 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

[dhdaines]

I think its rather a case of opting between the least bad of two options:

• Silently ignoring package.json options when package.json is broken
• Inexplicably fail when package.json is broken even its not used for mocha options

For 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

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?

[JoshuaKGoldberg]

Repeating from #5141 (comment) 🙂: 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.

[dhdaines]

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.

[dhdaines]

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.

https://github.com/dhdaines/mocha-5141

[JoshuaKGoldberg]

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!

[dhdaines]

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!

[voxpelli]

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)