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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 5 commits into from

Conversation

dhdaines
Copy link

@dhdaines dhdaines commented Aug 14, 2024 •
edited by JoshuaKGoldberg
Loading

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

@dhdaines
fix: report syntax errors in package.json (fixes: mochajs#5141)
6d8e90d
@dhdaines
fix(tests): incorrect test (should use existing result)
f1fe344 
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 Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Aug 14, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@dhdaines
Copy link
Author

(signed the CLA!)

voxpelli
voxpelli reviewed Aug 14, 2024
View reviewed changes
lib/cli/options.js Outdated Show resolved Hide resolved
lib/cli/options.js Outdated Show resolved Hide resolved
@voxpelli
Copy link
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
JoshuaKGoldberg
JoshuaKGoldberg approved these changes Oct 8, 2024
View reviewed changes
Copy link
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.

test/node-unit/cli/options.spec.js Outdated Show resolved Hide resolved
@JoshuaKGoldberg
Copy link
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
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...)

@dhdaines @JoshuaKGoldberg
fix: clarify invalid JSON
8c4103f 
Co-authored-by: Josh Goldberg ✨ <[email protected]>
@dhdaines
Copy link
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
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!

JoshuaKGoldberg
JoshuaKGoldberg approved these changes Oct 8, 2024
View reviewed changes
Copy link
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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoshuaKGoldberg JoshuaKGoldberg JoshuaKGoldberg approved these changes

@voxpelli voxpelli Awaiting requested review from voxpelli

Assignees
No one assigned
Labels
status: needs review a maintainer should (re-)review this pull request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

🐛 Bug: mocha fails silently on invalid package.json section
3 participants
@dhdaines @voxpelli @JoshuaKGoldberg