fix(jest-worker): hangs caused by failed assertions with circular values #15191
Conversation
✅ Deploy Preview for jestjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
✅ PR is ready for review |
|
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") |
… suggestion from the review
…serialization fallback case
Thank you so much for these suggestions. I missed this part. |
|
@SimenB, |
|
I cannot reproduce the failure locally, but yarnpkg/berry#6398 is probably it |
| * It's not ideal to lose `function` and `symbol` types, | ||
| * but reliability is more important. Additionally, | ||
| * information about issues with these types will be available | ||
| * in the error message. |
There was a problem hiding this comment.
what is the error message? can we have some test for this case (if nothing else so we know if the case is somehow supported?)
There was a problem hiding this comment.
Thank you so much for catching that. I'm too immersed in the reporters context and used the incorrect place for saving this information.
Fix f4bff33
…ions-with-circular-values
|
Sorry, just realised this change is only applied to process, not worker threads. Would you mind adding the same there? |
| process.send([PARENT_MESSAGE_OK, result]); | ||
| try { | ||
| process.send([PARENT_MESSAGE_OK, result]); | ||
| } catch (error: unknown) { |
There was a problem hiding this comment.
| } catch (error: unknown) { | |
| } catch (error) { |
it's unknown by default
There was a problem hiding this comment.
Thanks.
Fix bdafa91
* Check config * Clean generate code * Add verbose control * Change license follow root project * Add abis * Copy abis * Update config, add contracts * test abi * test contracts * clean generate * Try test check * check proxy admin * merge main * test * clean taiko * test proxy admin * add contract * format * format * add native token check * test config * operator test * test protocol * test bridge protocol operator * full test * fix syntax * fix syntax * fix syntax * split test * catch all error, related: jestjs/jest#10577 (comment) jestjs/jest#10577 (comment) jestjs/jest#15191 * test over all code * reorder rpc * jest script * test spec chains * chains.test * verify tokens * merge main * test repair & bugfix * token registered info repair * check protocol fee * Fix generated test * Rename names to codes and fix test function * Fix tests * not check now --------- Co-authored-by: xiaoch05 <xiaoch2010@gmail.com>
DmitryMakhnev
deleted the
ISSUE-10577-fix-hang-caused-by-failed-assertions-with-circular-values
branch
|
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. |
Summary
messageParent.tsinstead of showing the actual assertion error message when there are 2 or more test suite files executing in parallel #11617, closes feat: make detailed errors opt-in #15016@ungap/structured-clonefor serialization between workers in edge cases.Test plan
Unit tests were added
Detailed design
Based on @SimenB’s suggestion to @nodkz’s #14675, I added
@ungap/structured-clonefor cases where default JSON serialization doesn’t work.@ungap/structured-cloneapplies only to edge cases to preserve performance for regular cases.@ungap/structured-clonewas chosen over flattedflattedbecause it supports more data structures and also solves theBigIntissue #11617.If you have any other suggestions, you are welcome.