fix: print helpful message when internal CLI error happens

[Rob--W]

PR Checklist

• Addresses an existing open issue: fixes 🐛 Bug: Mocha prints out command help text and ERROR: null when unexpected error in -r module occurs #5078 / mocha failed and shows ERROR: null #5048 / others (the ERROR: null part of it)
• That issue was marked as status: accepting prs
• Steps in CONTRIBUTING.md were taken

Overview

There have been issues where the reported error is "ERROR: null", which is very unhelpful. Here is an analysis of a specific example: #5048 (comment)

Although the trigger for that error was fixed by #5074, the unhelpfulness of "ERROR: null" was not addressed.

To help with debugging, this patch prints the original error when this stage is unexpectedly reached.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Rob--W / name: Rob Wu (9f1fad1)

[JoshuaKGoldberg]

Switching to draft pending #5078 (comment). We can't review without a reproduction, and it might be that all buggy behavior was already fixed?

Thanks for the PR in the interim though!

[Rob--W]

Switching to draft pending #5078 (comment). We can't review without a reproduction, and it might be that all buggy behavior was already fixed?

For clarity, this is not about fixing whatever that triggered the error, but about improving the logged diagnostics information when an error (unexpectedly) happens.

While the specific trigger was fixed, it is still possible to end up in this case if some other internal error occurs. If you want to reproduce locally, you could throw an error from

mocha/lib/cli/run.js

Line 377 in abf3dd9

console.error('\n Exception during run:', err);

There are probably other ways to trigger this issue, but I haven't looked.

If you are concerned about logging too much information (when an error message is already present), I could add a check to only log the error when the message is null.

[Copilot]

Pull Request Overview

This PR enhances error reporting in the CLI by printing the original error object to stderr when an internal error occurs, making the “ERROR: null” message more informative.

• Prints the caught error object after showing help and the generic error message.
• Exits with status code 1 after logging the full error.

Comments suppressed due to low confidence (1)

lib/cli/cli.js:64

• Add a test case that simulates an internal CLI error to verify that the original error message (or stack) is printed as expected.

console.error(err);

[JoshuaKGoldberg]

@Rob--W #5078 is open and I see that you were right all along 🙂. I think this PR is ready for un-drafting - and pretty close to ready for merge. WDYT about the suggested change? And, mind if we un-draft this?

Normally I'd request test coverage but for an edge case like this that "shouldn't" ever happen, I think it's reasonable to skip testing.

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.50%. Comparing base (9cc3ada) to head (9f1fad1).
⚠️ Report is 37 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/cli/cli.js	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5344      +/-   ##
==========================================
- Coverage   93.52%   93.50%   -0.02%     
==========================================
  Files          57       57              
  Lines        4465     4467       +2     
  Branches      918      919       +1     
==========================================
+ Hits         4176     4177       +1     
- Misses        289      290       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mark-wiemer]

Hey @Rob--W, could you add a new unit test for this? Something that would've failed on main but passes with your changes?

[Lightning00Blade]

@mark-wiemer Hey if you take another look at the PR I have linked in the comments (#5562) You can see that this is happening on internal Mocha Error.
With that one would need to exploit a such failure to reproduce this and have a test, but then the bug should be fixed rendering the test failing.

Can you explain why exactly in this very niche issue (mostly for people to report/fix mocha issue) a test would be required?

[mark-wiemer]

Hey @Lightning00Blade, I know this is a pretty niche issue from what we can see, but it's hard to know just how often it occurs to end-users with wonky setups. Mocha specifically had a huge userbase a decade ago, and many folks using it today have patched together build pipelines over the years that are surprisingly weird. So even if an edge case is extremely rare, we want a unit test to cover it. That way, if we need to change behavior later, we can better guarantee that we didn't break any "promises" we made with previous fixes. In short, we test everything because it's best practice :)

[Rob--W]

Do you have pointers / examples of preferred practices to use for testing this? I did not find something close when I looked for existing ones at https://github.com/mochajs/mocha/tree/main/test/node-unit/cli

To test this effectively, we would have to inspect the serialized output of the cli execution. The next best alternative is to spy on the console global to detect console log calls.

I'm leaning towards the latter so that I can simply check the raw error instead of having to interpret Node's interpretation of the error serialization.

[mark-wiemer]

Hey @JoshuaKGoldberg, can you assist here? I don't have capacity right now :/

[JoshuaKGoldberg]

Yeah, this is a tricky one. Normally I'd suggest extracting the code that includes logging into a function in a separate file, and then unit test that function. But this specific area of code is pretty inter-tangled with the yargs stuff.

This is also a single-line change that is easy to verify manually. So I feel a little more comfortable suggesting: how about we merge as-is, and file a followup task for adding test coverage? @mark-wiemer

[mark-wiemer]

Sounds good. I'll manually verify later today, merge this if it works, and open a chore to cover this scenario with automated tests

[mark-wiemer]

Was able to repro the issue and get the additional information thanks to this PR. See backing issue for details. Thank you very much!

[JoshuaKGoldberg]

🚢

[JoshuaKGoldberg]

🚢