jestjs / jest

Delightful JavaScript Testing.
https://jestjs.io
MIT License
44.12k stars 6.44k forks source link

fix(jest-worker): hangs caused by failed assertions with circular values #15191

Closed DmitryMakhnev closed 2 months ago

DmitryMakhnev commented 2 months ago

Summary

Test plan

Unit tests were added

Detailed design

Based on @SimenB’s suggestion to @nodkz’s #14675, I added @ungap/structured-clone for cases where default JSON serialization doesn’t work. @ungap/structured-clone applies only to edge cases to preserve performance for regular cases. @ungap/structured-clone was chosen over flatted flatted because it supports more data structures and also solves the BigInt issue #11617.

If you have any other suggestions, you are welcome.

netlify[bot] commented 2 months ago

Deploy Preview for jestjs ready!

Name Link
Latest commit 19f66cc11e11283003d46e3798cbed0fdbafbd27
Latest deploy log https://app.netlify.com/sites/jestjs/deploys/669f5dc63dfcc000080d1faa
Deploy Preview https://deploy-preview-15191--jestjs.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

DmitryMakhnev commented 2 months ago

✅ PR is ready for review

G-Rath commented 2 months ago

awesome stuff! it looks pretty good to me though I don't consider myself an expert in this area so I might have missed something 😅

one something that I would recommend considering is making sure there are some tests with complex data such as functions - afaik it's not possible for them to survive serialization (or maybe it's if they use a variable outside of their body...?), but I think it would be good to make sure we've got some tests that show off what happens to ensure everyone understands what's expected (even if that's "the function is destroyed")

DmitryMakhnev commented 2 months ago

Hello @G-Rath and @SimenB,

one something that I would recommend considering is making sure there are some tests with complex data such as functions - afaik it's not possible for them to survive serialization (or maybe it's if they use a variable outside of their body...?), but I think it would be good to make sure we've got some tests that show off what happens to ensure everyone understands what's expected (even if that's "the function is destroyed")

Maybe a couple of tests with e.g. function, symbol etc might be a good idea?

Thank you so much for these suggestions. I missed this part. I have thought a lot about the most correct solution for this. All the approaches I see now have trade-offs. So, I hope the chosen approach has acceptable trade-offs. Fix https://github.com/jestjs/jest/pull/15191/commits/b6e4a5b80bbdd88208c86bc24ca0c52f96c6b8ae

DmitryMakhnev commented 2 months ago

@SimenB, I see that the PR has 11 failed checks. All of them have same failed message https://github.com/jestjs/jest/actions/runs/9995583298/job/27628086141?pr=15191. Also I see that other last PR have same issue. It looks like the fails were produced by 22.5.0 version of Node.js https://github.com/jestjs/jest/actions/runs/9995583298/job/27628086141?pr=15191#step:3:10. Last PR with passed Node.js 22 version had 22.4.1 version https://github.com/jestjs/jest/actions/runs/9969386751/job/27546278751?pr=15194#step:3:10. Could you please tell me what I should do for fixing this? Thanks.

SimenB commented 2 months ago

I cannot reproduce the failure locally, but https://github.com/yarnpkg/berry/issues/6398 is probably it

SimenB commented 2 months ago

Sorry, just realised this change is only applied to process, not worker threads. Would you mind adding the same there?

DmitryMakhnev commented 2 months ago

Hello @SimenB,

Sorry, just realised this change is only applied to process, not worker threads. Would you mind adding the same there?

Sorry for missing this. Thank you so much for catching this. Also I added tests worker threads. Fix https://github.com/jestjs/jest/pull/15191/commits/0b2eb637bdd8ef314838ed34afa0afe60e952b11

SimenB commented 1 month ago

https://github.com/jestjs/jest/releases/tag/v30.0.0-alpha.6

github-actions[bot] commented 1 week ago

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.