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-runner): Handle unsafe worker_threads structureClone with function type in matchers #14436

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

dj-stormtrooper
Copy link
Contributor

@dj-stormtrooper dj-stormtrooper commented Aug 21, 2023

Summary

This PR solves this particular issue #14423

Test plan

Issue solved, now workerThreads can be used safely with function types

I tested the behaviour on examples/typescript suite. I added example test, changed config in package.json to use workers and reproduced the behaviour from the issue and the checked that it is fixed after my changes were implemented. Example test:

it('should not explode', () => {
    const foo = () => {}
    const bar = () => {}
    expect(foo).toBe(bar)
})

Config:

// examples/typescript/package.json
 "jest": {
    "testEnvironment": "jsdom",
    "workerThreads": true,
    "maxWorkers": 1
  }

Command:

yarn test --watch

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 21, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@netlify
Copy link

netlify bot commented Aug 21, 2023

Deploy Preview for jestjs ready!

Name Link
🔨 Latest commit 3f50677
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/65e1ad90c7c04a00087b2df6
😎 Deploy Preview https://deploy-preview-14436--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.

@dj-stormtrooper dj-stormtrooper changed the title Fix/worker threads data clone fix(jest-runner): Handle unsafe worker_threads structureClone with function type in matchers Aug 21, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
function makeSerializableTestResults(result: TestResult): TestResult {
const {testResults} = result;
const serializableResults = testResults.map(resultItem => {
if (resultItem.failureDetails) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I get it right: the problem is that failureDetails includes data which cannot be serialised? If so, it be better to serialised it earlier in the test runner.

This was discussed already, I will try to dig out the thread. The idea is that we know in advance that all what the runner returns has to be serialisable, because that data will be passed through workers. If I remember it right, the test runner is already serialising most of the data. Seem like something was simply overlooked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I get it right: the problem is that failureDetails includes data which cannot be serialised?

Yes, exactly, the problem with function type which throw an error during clone. Also DOM nodes might cause similar behaviour, but I'm not sure if it's a valid case for workers.

If so, it be better to serialised it earlier in the test runner.

I was also thinking about it, my only concern was that it might be exposed to other tools (outside of jest repo). But if the approach is ok I can fix this and add some tests to cover the serialisation

This was discussed already, I will try to dig out the thread

Sounds good, found this related PR, would be nice to look into other discussions

Copy link
Contributor

Choose a reason for hiding this comment

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

Here it is. I was talking about #11467. The conclusions are made in #11467 (comment) and following comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrazauskas Thanks a lot, now I have full context

@dj-stormtrooper
Copy link
Contributor Author

dj-stormtrooper commented Aug 24, 2023

@SimenB I'm trying to solve the issue with unserialisable values with worker_threads, as discussed here, unfortunately I can't post comments to original PR since it's closed, so let's discuss here?

I made an implementation in this PR with the patch in jest-runner for workers exclusively but @mrazauskas pointed out that it's better to prepare it directly at circus/jasmine. While I'm completely fine with the approach I still have few topics to discuss:

  1. WDYT about this format, is it ok to replace undesired values with string placeholders?
  2. Maybe we could get some inspiration from jest/pretty-format or reuse some code.
  3. For what cases do we need an actual references to expected/actual values? Wild idea, but can we keep only string representation of these objects (e.g. formatted with pretty-format)

@dj-stormtrooper
Copy link
Contributor Author

dj-stormtrooper commented Sep 1, 2023

@SimenB @mrazauskas Could you guys take a look here, please?

I'm ok to implement it in circus and jasmine, but I'm not sure if it's ok to replace any complex value with a string placeholder? (because of the type mismatch)

Good thing is that at least we keep the name of the original type, e.g. [Function foo] - it can be used by reporters

@SimenB SimenB added this to the Jest 30 milestone Sep 12, 2023
@SimenB
Copy link
Member

SimenB commented Dec 26, 2023

I'm ok to implement it in circus and jasmine, but I'm not sure if it's ok to replace any complex value with a string placeholder? (because of the type mismatch)

I think that's fine - it should be stringified by the time a reporter gets the value. If people want to format the message themselves, that should be done in a custom matcher

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants