Skip to content

fix: handle case of invalid package.json with no explicit config#5198

Merged
JoshuaKGoldberg merged 5 commits intomochajs:mainfrom
dhdaines:issue-5141
Oct 29, 2024
Merged

fix: handle case of invalid package.json with no explicit config#5198
JoshuaKGoldberg merged 5 commits intomochajs:mainfrom
dhdaines:issue-5141

Conversation

@dhdaines
Copy link
Copy Markdown
Contributor

@dhdaines dhdaines commented Aug 14, 2024

PR Checklist

Overview

Handle the corner case which can happen if:

  • You had a perfectly good package.json which npm was happy with
  • You decided to put mocha configuration inside your package.json
  • You left a stray comma in there, because you are used to more sensible configuration file formats ;-)
  • Instead of running npm test you ran mocha directly (with npx or just mocha.js)

As detailed here: https://github.com/dhdaines/mocha-5141

Since `readFileSync` is only stubbed `onFirstCall` we get a different answer
the second time around which is probably Not What You Want.  But also we
*already checked that config = false*.  So you could just remove this
test, it does nothing useful.
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Aug 14, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@dhdaines
Copy link
Copy Markdown
Contributor Author

(signed the CLA!)

Comment thread lib/cli/options.js Outdated
Comment thread lib/cli/options.js Outdated
@voxpelli
Copy link
Copy Markdown
Member

Thanks for the PR! Gave some feedback

@dhdaines dhdaines changed the title Issue 5141 fix: handle case of invalid package.json with no explicit config Aug 14, 2024
Copy link
Copy Markdown
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nicely done! Agreed with the strategy inside 👍 thanks!

Since @voxpelli was looking at this before, leaving open for re-review.

Comment thread test/node-unit/cli/options.spec.js Outdated
@JoshuaKGoldberg
Copy link
Copy Markdown
Member

Btw @dhdaines, just a heads up, the - [ x ]s in the description can be - [x]s (no spacing) to work with GitHub's task lists. I edited them just now. 🙂

@JoshuaKGoldberg JoshuaKGoldberg added the status: needs review a maintainer should (re-)review this pull request label Oct 8, 2024
@dhdaines
Copy link
Copy Markdown
Contributor Author

dhdaines commented Oct 8, 2024

Btw @dhdaines, just a heads up, the - [ x ]s in the description can be - [x]s (no spacing) to work with GitHub's task lists. I edited them just now. 🙂

Ohhh! Thank you, I have been wondering about that for ages now! (suppose I should have RTF the Markdown manual...)

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
@dhdaines
Copy link
Copy Markdown
Contributor Author

dhdaines commented Oct 8, 2024

Ah, I suspect that the more obviously invalid JSON will break the too-specific assertion in the test... will fix it in a second.

@dhdaines
Copy link
Copy Markdown
Contributor Author

dhdaines commented Oct 8, 2024

Ah, I suspect that the more obviously invalid JSON will break the too-specific assertion in the test... will fix it in a second.

Done!

Copy link
Copy Markdown
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌 Looks great to me! Just waiting on @voxpelli. Thanks!

@JoshuaKGoldberg JoshuaKGoldberg merged commit f72bc17 into mochajs:main Oct 29, 2024
lennonnikolas pushed a commit to lennonnikolas/mocha that referenced this pull request Jan 24, 2026
…hajs#5198)

* fix: report syntax errors in package.json (fixes: mochajs#5141)

* fix(tests): incorrect test (should use existing result)

Since `readFileSync` is only stubbed `onFirstCall` we get a different answer
the second time around which is probably Not What You Want.  But also we
*already checked that config = false*.  So you could just remove this
test, it does nothing useful.

* fix: separate read and parse errors

* fix: clarify invalid JSON

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

* fix(test): expect only a SyntaxError nothing else

---------

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: needs review a maintainer should (re-)review this pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

3 participants