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

feat: support reporting AggregateErrors #5018

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

CheadleCheadle
Copy link

@CheadleCheadle CheadleCheadle commented Oct 19, 2023

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions.

Description of the Change

Addresses #4982
If an AggregateError is thrown, the base reporter will print out the AggregateError's errors. Before this change, only the Aggregate Error itself was printed. So, if the current error being printed is an Aggregate error the errors it contains will also be formatted and printed.

Alternate Designs

I could have completely restructured the Base.list method to only loop over the failures and call a helper function that would create the format(fmt), msg, and title and then print the error, but it was much simpler to just add a conditional check to see if the current error being printed was an AggregateError and if it contained multiple errors and just

Why should this be in core?

Currently, nothing nice prints. When dealing with AggregateErrors it would be better if each subsequent error was printed and formatted as well.

Benefits

The errors within the AggregateError will be nicely formatted when printed.

Possible Drawbacks

If an AggregateError contains a lot of errors, then a lot of errors will be printed!

Applicable issues

Nothing of note.

Copy link
Member

@voxpelli voxpelli left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! 🙏

Some initial feedback

lib/cli/run-option-metadata.js Outdated Show resolved Hide resolved
lib/reporters/base.js Outdated Show resolved Hide resolved
lib/reporters/base.js Show resolved Hide resolved
lib/reporters/base.js Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
lib/reporters/json.js Outdated Show resolved Hide resolved
@wesleytodd
Copy link

Plus one on this. I just ran into it. Is anyone working on this anymore? It looks like maybe the feedback was addressed?

@nwalters512
Copy link

Somewhere between Node 16 and Node 20, Node's own APIs started throwing AggregateError in certain cases, e.g. network failures when autoSelectFamily === true (the default). Without proper formatting, it's very difficult to debug such errors when they occur in Mocha tests. Is there anything we can do to help this along?

@CheadleCheadle
Copy link
Author

I addressed all the changes and sent an email to voxpelli to see if he could approve it.

@voxpelli
Copy link
Member

voxpelli commented Feb 2, 2024

I'll try to get around to it soon, else eg. @JoshuaKGoldberg can chime in?

lib/reporters/base.js Outdated Show resolved Hide resolved
Modified check for if an error is an instance of an AggregateError when recursively printing errors
@voxpelli
Copy link
Member

I'm running out of time to review and merge this today, but this one together with these two are my main priority right now:

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.

Good start! 🙌

I'd also like to hear from @voxpelli, but leaving some thoughts in the meantime too.

lib/cli/run-option-metadata.js Outdated Show resolved Hide resolved
bin/mocha.js Outdated Show resolved Hide resolved
lib/reporters/base.js Outdated Show resolved Hide resolved
lib/reporters/base.js Show resolved Hide resolved
test/reporters/base.spec.js Outdated Show resolved Hide resolved
test/reporters/base.spec.js Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added status: waiting for author waiting on response from OP - more information needed semver-minor implementation requires increase of "minor" version number; "features" labels Mar 4, 2024
@JoshuaKGoldberg JoshuaKGoldberg changed the title Aggregate error feat: support reporting AggregateErrors Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor implementation requires increase of "minor" version number; "features" status: waiting for author waiting on response from OP - more information needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants