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

fixes #4994 - Preserves error.name during serialization process #4995

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jamesmortensen
Copy link

@jamesmortensen jamesmortensen commented Jul 8, 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

When error objects are serialized, the name is stripped from the object. The name contains the type of the error, whether it's an AssertionError, TypeError, ReferenceError, or just Error. Some test reporters use the type of the error to determine how to process the failure. For example, Allure treats AssertionErrors as failures and all other errors as broken tests.

The change involved modifying serializer.js so that when the error object is serialized, it includes the name property so that when it's later deserialized, the name is still present. Currently, only the stack and message properties were preserved.

Alternate Designs

One other alternative would have been to attempt to patch this in the reporter directly by extracting the error type from the stack trace. This seems messy and would have involved adding conditional logic to determine whether mocha is running in parallel or sequentially. Since it shouldn't matter -- the results should be the same way no matter what -- I decided it would be best to make the change upstream, in mocha. This also allows other reporters to capitalize on this change.

Since no name property existed in mocha, this should be a non-breaking change.

Why should this be in core?

Sending test results to reporters is a core function of mocha. What to do with that data is of course the responsibility of the reporter, but it's mocha's responsibility to make sure there's consistency in how the data is being sent to the reporters. When running tests sequentially, the error object contains the name property. I believe this happens because no serialization is required. Reporters receive this information when running sequentially.

But when running in parallel, Mocha's behavior of how errors are passed to reporters changes. Important information, such as error type, is not included in the name property. Including it in the name property creates consistency for those developing reporters.

Benefits

Users of Allure reporter can run mocha in parallel mode.

Since Mocha released parallelism in version 8.0.0, users of allure were not able to capitalize on the feature without complicated hacks like this: allure-framework/allure-js#245 (comment). Fixing this issue makes Allure more usable.

Maintenance of reporters using the error object is easier.

Hacking the stack object would have been cool, but it might have been more difficult to understand or maintain than changing one line of code in mocha, which represents an additive change, not a breaking change.

Possible Drawbacks

The only thing I can think of is if some reporter was, for whatever reason, looking for the error.name to be undefined, and it ended up being empty string or null, that could potentially impact the behavior of their reporter. However, I cannot imagine why someone would do that. Additionally, if the error.name is already undefined, then the name property should continue to be undefined. I do not see this as being high risk change.

Applicable issues

…s so type of error can be used by reporters who differentiate between failed tests and broken tests, such as allure-mocha
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 8, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: jamesmortensen / name: James Mortensen (1c59fac)

@jamesmortensen
Copy link
Author

Hi. I wanted to check in on the status of this pull request? Please let me know if there are any other details you need in order to evaluate this small change. Thank you!

@JoshuaKGoldberg
Copy link
Member

Marking as draft and blocked on an isolated reproduction for #4994. cc @jamesmortensen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: blocked Waiting for something else to be resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants