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.

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

fix: harden error handling in lib/cli/run.js #5074

Merged
merged 14 commits into from
Mar 26, 2024

Conversation

stalet
Copy link
Contributor

@stalet stalet commented Jan 8, 2024

Before this patch the errorhandling would fail with the error: "ERROR: null". Debug showed that the error "caught error sometime before command handler: TypeError: Cannot convert object to primitive value"

PR Checklist

Overview

When the error in the runner is a typescript error the current script fails with the error: "TypeError: Cannot convert object to primitive value" - leaving the output as a null value.
It is very hard to see the actuall error. With this change the output will be something like this.

Error: undefined test/testdata.ts(59,14): error TS2739: Type '{ loginId: number; spidId: number; sid: string; amr: never[]; }' is missing the following properties from type 'Identity': sub, nbf, exp, iat

Before the fix the output was
✖ ERROR: null
Debug output contailed this
mocha:cli:cli caught error sometime before command handler: TypeError: Cannot convert object to primitive value mocha:cli:cli at exports.handler (/home/stalet/code/account/min-finn-frontend/node_modules/.pnpm/mocha@10.2.0/node_modules/mocha/lib/cli/run.js:372:65) +859ms

Copy link

linux-foundation-easycla bot commented Jan 8, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@voxpelli
Copy link
Member

voxpelli commented Jan 8, 2024

Can you provide a reproduction? Sounds odd that it would swallow all TypeScript errors currently

Other PR:s related to error reporting are eg:

@stalet
Copy link
Contributor Author

stalet commented Jan 11, 2024

I have created a small project that reproduces the error here: https://github.com/stalet/mocha-typescript-testproject

Before this patch the errorhandling would fail with the error: "ERROR: null". Debug showed that the error "caught error sometime before command handler: TypeError: Cannot convert object to primitive value"
@stalet stalet force-pushed the bugfix-typescript-errors-not-outputted branch from ecba5cd to 77934c1 Compare January 15, 2024 07:12
@barak007
Copy link

barak007 commented Jan 21, 2024

It's a general thing with errors not related to Typescript

console.error('\n' + (err.stack || `Error: ${err.message || err}`));

the above line from mocha fail if I throw null as an error

try {
 throw null;
} catch (err) { 
  console.error('\n' + (err.stack || `Error: ${err.message || err}`));
}

the error will be

Uncaught TypeError: Cannot read properties of null (reading 'stack')

@JoshuaKGoldberg
Copy link
Member

👀

  • Addresses an existing open issue: fixes #000

See .github/CONTRIBUTING.md. We ask that PRs address an open accepted issue first. A few benefits:

  • Issues are much more discoverable for folks looking to discuss & report suggested changes.
  • The issue templates ask for the info needed to properly triage the suggestion.
  • We don't want unhappy situations where someone spends their time on a PR that can't be accepted (which would have been caught by an issue's discussion).

Closing this PR so it doesn't show up on our review queue. But if it is determined in an issue that we can take this in, we'd be happy to re-open and review. Thanks for sending!

tl;dr: please file an issue so we can discuss there 🙂.

lib/cli/run.js Outdated Show resolved Hide resolved
@voxpelli voxpelli linked an issue Jan 22, 2024 that may be closed by this pull request
4 tasks
@coveralls
Copy link

coveralls commented Jan 22, 2024

Coverage Status

coverage: 94.312% (-0.02%) from 94.335%
when pulling 8be4ad1 on stalet:bugfix-typescript-errors-not-outputted
into 3345eff on mochajs:master.

Co-authored-by: Pelle Wessman <pelle@kodfabrik.se>
lib/cli/run.js Outdated Show resolved Hide resolved
lib/cli/run.js Outdated Show resolved Hide resolved
Do not change the original functionality.

Co-authored-by: Pelle Wessman <pelle@kodfabrik.se>
@stalet stalet requested a review from voxpelli February 2, 2024 15:09
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.

Requesting changes to handle the node:module case of errors that don't have a toString() method. Please let me know if I've gone down the totally wrong track though!

lib/cli/run.js Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author waiting on response from OP - more information needed label Feb 7, 2024
@stalet
Copy link
Contributor Author

stalet commented Feb 8, 2024

After these changes I have verified the in the testproject i get the following output instead of ✖ ERROR: null

Undefined error: test/index.test.ts(9,44): error TS2322: Type 'string' is not assignable to type 'number'.

Which was the intention.

Copy link

@barak007 barak007 left a comment

Choose a reason for hiding this comment

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

LGTM.

now we only need to support AggregateError and Error cause and it will be perfect reporting
I think they have separated issues

@voxpelli
Copy link
Member

voxpelli commented Feb 9, 2024

Yep, and I think they are all waiting on me, I'll try to squeeze them all in this Monday:

This was referenced Sep 12, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Bug: Errorhandling fails when node:module registered errors are thrown
5 participants