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(jest-worker): failed assertion with circular value hangs the test run #14675

Closed
wants to merge 8 commits into from

Conversation

nodkz
Copy link
Contributor

@nodkz nodkz commented Nov 2, 2023

Relates #10577

Summary

When an assertion fails where either the expected or actual value is circular, and both values are objects, jest encounters an error stating it failed to convert a circular structure to JSON, resulting in the test run not completing.

This PR fixes sending result with circular references from worker to runner.

       TypeError: Converting circular structure to JSON
        --> starting at object with constructor 'ClientRequest'
        |     property 'socket' -> object with constructor 'TLSSocket'
        --- property '_httpMessage' closes the circle
        at stringify (<anonymous>)

      at messageParent (node_modules/jest-worker/build/workers/messageParent.js:29:19)

Copy link

linux-foundation-easycla bot commented Nov 2, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

netlify bot commented Nov 2, 2023

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit d76b9a2
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/65490d6a5a7a3300086813d0
😎 Deploy Preview https://deploy-preview-14675--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.

@nodkz nodkz changed the title fix: failed expect with circular references hangs jest worker fix(jest-worker): failed assertion with circular value hangs the test run Nov 3, 2023
@nodkz
Copy link
Contributor Author

nodkz commented Nov 3, 2023

✅ PR is ready for review @SimenB @fa93hws

@nodkz nodkz closed this Nov 3, 2023
@nodkz nodkz reopened this Nov 3, 2023
@nodkz
Copy link
Contributor Author

nodkz commented Nov 3, 2023

Close/open for test re-run

@nodkz
Copy link
Contributor Author

nodkz commented Nov 4, 2023

rerun tests again due to infra reliability (yarn install on mac)

@nodkz nodkz closed this Nov 4, 2023
@nodkz nodkz reopened this Nov 4, 2023
@nodkz
Copy link
Contributor Author

nodkz commented Nov 6, 2023

rerun again

@nodkz nodkz closed this Nov 6, 2023
@nodkz nodkz reopened this Nov 6, 2023
@nodkz nodkz closed this Nov 6, 2023
@nodkz nodkz reopened this Nov 6, 2023
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, awesome to see some movement on this!

Can you also unskip this?

test.skip('handles circular inequality properly', async () => {

packages/jest-worker/src/workers/messageParent.ts Outdated Show resolved Hide resolved
packages/jest-worker/src/workers/processChild.ts Outdated Show resolved Hide resolved
* LICENSE file in the root directory of this source tree.
*/

export function withoutCircularRefs(obj: unknown): unknown {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we do the inverse as well - rebuild the object with the circular references in place?

I.e. what https://www.npmjs.com/package/flatted does (I see they now wanna push users to https://github.com/ungap/structured-clone/blob/main/README.md#extra-features, might be worth a look)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimenB flatted and structured-clone have own serialization format, so both jest-runner and jest-worker should have the same serialize/parse logic. But currently runner and worker are different packages and end-users might have different versions on their installation.

So we break a transport layer by this improvement. Better to do it in another PR as improvement and R&D, this PR is a bug fix PR. And I'm very sorry but I don't have enough bandwidth to deal with bumping v30 😉

@nodkz nodkz requested a review from SimenB November 7, 2023 10:10
@nodkz
Copy link
Contributor Author

nodkz commented Nov 21, 2023

@SimenB friendly ping ;)

Copy link

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Feb 19, 2024
Copy link

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

@github-actions github-actions bot closed this Mar 20, 2024
Copy link

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants