Skip to content

fix(jest-worker): failed assertion with circular value hangs the test run#14675

Closed
nodkz wants to merge 8 commits into
jestjs:mainfrom
nodkz:fix-child-circular-result
Closed

fix(jest-worker): failed assertion with circular value hangs the test run#14675
nodkz wants to merge 8 commits into
jestjs:mainfrom
nodkz:fix-child-circular-result

Conversation

@nodkz

@nodkz nodkz commented Nov 2, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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)

@linux-foundation-easycla

linux-foundation-easycla Bot commented Nov 2, 2023 •
edited
Loading

Copy link
Copy Markdown

CLA Signed

The committers listed above are authorized under a signed CLA.

@netlify

netlify Bot commented Nov 2, 2023 •
edited
Loading

Copy link
Copy Markdown

✅ 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 force-pushed the fix-child-circular-result branch from 703cfad to b06be16 Compare November 3, 2023 14:09
@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

nodkz commented Nov 3, 2023

Copy link
Copy Markdown
Contributor Author

✅ PR is ready for review @SimenB @fa93hws

@nodkz nodkz closed this Nov 3, 2023
@nodkz nodkz reopened this Nov 3, 2023
@nodkz

nodkz commented Nov 3, 2023

Copy link
Copy Markdown
Contributor Author

Close/open for test re-run

@nodkz

nodkz commented Nov 4, 2023

Copy link
Copy Markdown
Contributor Author

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

nodkz commented Nov 6, 2023

Copy link
Copy Markdown
Contributor Author

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
SimenB
SimenB reviewed Nov 6, 2023
View reviewed changes

@SimenB SimenB left a comment

Copy link
Copy Markdown
Member

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?

jest/e2e/__tests__/circularInequality.test.ts

Line 28 in 4fedfbd

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

Comment thread packages/jest-worker/src/workers/messageParent.ts Outdated
Comment thread packages/jest-worker/src/workers/processChild.ts Outdated
Comment thread packages/jest-worker/src/workers/withoutCircularRefs.ts
* LICENSE file in the root directory of this source tree.
*/

export function withoutCircularRefs(obj: unknown): unknown {

@SimenB SimenB Nov 6, 2023

Copy link
Copy Markdown
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)

@nodkz nodkz Nov 6, 2023

Copy link
Copy Markdown
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

nodkz commented Nov 21, 2023

Copy link
Copy Markdown
Contributor Author

@SimenB friendly ping ;)

@github-actions

github-actions Bot commented Feb 19, 2024

Copy link
Copy Markdown

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
@github-actions

github-actions Bot commented Mar 20, 2024

Copy link
Copy Markdown

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
@github-actions

github-actions Bot commented Mar 20, 2024

Copy link
Copy Markdown

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 commented Apr 21, 2024

Copy link
Copy Markdown

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.

Reviewers

@SimenB SimenB Awaiting requested review from SimenB

Assignees

No one assigned

Labels

Stale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nodkz @SimenB