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: better tracking of seen objects in error serialization #5032

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

Conversation

sam-super
Copy link

@sam-super sam-super commented Nov 21, 2023

Description of the Change

Breaks circular references in before objects are serialized to prevent cryptic errors in parallel mode.
E.g. (taken from the linked issue):

 1) Uncaught error outside test suite

  0 passing (308ms)
  1 failing

  1) Uncaught error outside test suite:
     Uncaught TypeError: Converting circular structure to JSON
    --> starting at object with constructor 'Object'
    |     property 'props' -> object with constructor 'Array'
    --- index 0 closes the circle
      at stringify (<anonymous>)
      at writeChannelMessage (node:internal/child_process/serialization:120:20)
      at process.target._send (node:internal/child_process:819:17)
      at process.target.send (node:internal/child_process:719:19)
      at processTicksAndRejections (node:internal/process/task_queues:93:5)

For context we have seen this sort of error when using nestjs (e.g. when a module is missing a dependency, the error thrown contains a circular reference). It's likely something that would be good to fix in nestjs, but mocha would ideally handle it gracefully, regardless.

Alternate Designs

We could just strip all 'extra' props of the Error object and keep only the standard parts (e.g. message,name and stack). Would need to make sure those props are what we expectthem to be. This would be more performant but at the cost of losing useful information on the error object.

Why should this be in core?

Saves people from having to turn parallel mode off to work out why their test failed.

Benefits

Errors that contain circular references can be more easily debugged.

Possible Drawbacks

There is probably a performance penalty when inspecting the objects. The assumption is that it's not prohibitive.

Applicable issues

This will be a fix/patch release

Fixes #4552

Breaks circular references in before objects are serialized to prevent
cryptic error in parallel mode.
Copy link

linux-foundation-easycla bot commented Nov 21, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@coveralls
Copy link

coveralls commented Nov 21, 2023

Coverage Status

coverage: 94.424% (+0.07%) from 94.358%
when pulling c3a1129 on sam-super:issues/4552
into 99601da on mochajs:master.

@sam-super
Copy link
Author

Worth a re-run of the failing test?

@sam-super
Copy link
Author

@Uzlopak looks like it might be a false positive. Windows tests ok for v14/v18 and i can't think of anything that might be low-level enough to affect just that set of versions.

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, nice and clean! ✨

@voxpelli I know you were looking at errors too, so I'd want to defer to your review on this one too.

@JoshuaKGoldberg JoshuaKGoldberg added the status: in triage a maintainer should (re-)triage (review) this issue label Mar 4, 2024
@JoshuaKGoldberg JoshuaKGoldberg changed the title fix: closes #4552 fix: better tracking of seen objects in error serialization Mar 4, 2024
@JoshuaKGoldberg JoshuaKGoldberg added the semver-patch implementation requires increase of "patch" version number; "bug fixes" label Mar 4, 2024
@JoshuaKGoldberg JoshuaKGoldberg removed the status: in triage a maintainer should (re-)triage (review) this issue label Mar 12, 2024
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: Parallel mode crashes if test exception contains circular references
4 participants