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

Merged
merged 10 commits into from
Jun 26, 2024

Conversation

sam-super
Copy link
Contributor

@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.38% (+0.07%) from 94.314%
when pulling aee52d7 on sam-super:issues/4552
into 103c56b on mochajs:main.

@sam-super
Copy link
Contributor Author

Worth a re-run of the failing test?

@sam-super
Copy link
Contributor 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
@JoshuaKGoldberg
Copy link
Member

FWIW @sam-super you don't need to keep merging in main, though I do appreciate the effort! 🙂

I just wanted to wait a bit for a second review in case Pelle had time. That was time boxed till end of last week, so I'll go ahead and merge + release this now. Thanks again for the PR! 🙌

@JoshuaKGoldberg JoshuaKGoldberg merged commit 02c04c4 into mochajs:main Jun 26, 2024
23 of 24 checks passed
@JoshuaKGoldberg
Copy link
Member

Published in [email protected]. 🚀

@adrigzr
Copy link

adrigzr commented Jun 27, 2024

Hello @sam-super @JoshuaKGoldberg, I have received the following exception randomly since this release. Do you know if it could be related to the change? 🤔

  1) Uncaught error outside test suite:
     Uncaught TypeError: this.parent.titlePath is not a function
      at Test.Runnable.titlePath (/node_modules/mocha/lib/runnable.js:217:22)
      at Test.Runnable.fullTitle (/node_modules/mocha/lib/runnable.js:206:15)
      at Test.serialize (/node_modules/mocha/lib/test.js:94:23)
      at SerializableEvent.serialize (/node_modules/mocha/lib/nodejs/serializer.js:256:27)
      at /node_modules/mocha/lib/nodejs/serializer.js:72:13
      at Array.forEach (<anonymous>)
      at SerializableWorkerResult.serialize (/node_modules/mocha/lib/nodejs/serializer.js:71:17)
      at serialize (/node_modules/mocha/lib/nodejs/serializer.js:366:15)
      at /node_modules/mocha/lib/nodejs/worker.js:127:28
      at ParallelBuffered.done (/node_modules/mocha/lib/nodejs/reporters/parallel-buffered.js:151:5)

Regards,

@JoshuaKGoldberg
Copy link
Member

Ah looks like it was! #5170 cc @sam-super

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
5 participants