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

feat: add support for snapshot matchers in concurrent tests #14139

Merged
merged 4 commits into from
Jun 30, 2023

Conversation

dmitri-gb
Copy link
Contributor

Summary

This implements support for snapshot matchers in concurrent tests. The long-standing issue with this has been rooted in the fact that because concurrent tests run in parallel there is no concept of a single currently running test and so jest-snapshot is not able to get this information from a global state object.

This problem can however be solved with AsyncLocalStorage, which is an object that can hold different values for different async contexts. Each concurrent test runs in its own async context and when asking for a value from the storage, gets a result that is unique to it.

Support is added only to jest-circus.

Test plan

There is a new integration test for this use-case.

Fixes #2180.
Fixes #5801.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 12, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@netlify
Copy link

netlify bot commented May 12, 2023

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit d53dfd8
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/647c7047af661f000839ca99
😎 Deploy Preview https://deploy-preview-14139--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 settings.

@SimenB
Copy link
Member

SimenB commented Jun 21, 2023

Hey! I landed #14045 which aims to solve the same issue, albeit with a new matcher. Do you think this PR adds additional value?

@mrazauskas
Copy link
Contributor

To add my two cents: this PR looks like a better solution than new matcher.

  1. The solution propose here makes the same matcher work in normal and constant modes. This means users can switch back and forth between the modes easily.
  2. This solution does need any additional checks or lint rules. (See Add a rules to help with the .toMatchNamedSnapshot() matcher usage jest-community/eslint-plugin-jest#1367)
  3. The toMatchSnapshot() and toMatchNamedSnapshot() are very similar. More API surface and not so clear when to use which.
  4. toMatchNamedSnapshot() requires manually naming each snapshot. Hm.. That’s an extra step in contrary the solution proposed here simply works with toMatchSnapshot().

It is a pity that so much work went into toMatchNamedSnapshot() PR, but I would say it should be reverted. This PR should land instead to solve the issue.

@dmitri-gb
Copy link
Contributor Author

I tend to agree with @mrazauskas. The fact that the same matcher can be used in concurrent and normal tests and that you don't need to give it an explicit name makes it more appealing to me.

@SimenB
Copy link
Member

SimenB commented Jun 30, 2023

Yeah, I'm kinda convinced by that argument 😅

@SimenB
Copy link
Member

SimenB commented Jul 4, 2023

@nutchanon-c
Copy link

i bumped my jest to 29.6.0 and there are multiple concurrent tests in multiple describes in the same file, I think I'm still getting random snapshot names. Is this a known issue or an expected behaviour?

@SimenB
Copy link
Member

SimenB commented Jul 26, 2023

#14335 is not released yet - might be it

@nutchanon-c
Copy link

Ohh I see. Thanks a lot. Can't wait for the release.

@SimenB
Copy link
Member

SimenB commented Jul 27, 2023

@nutchanon-c new release is out - can you confirm it fixes your issue? If not, please open a new issue 🙂

@nutchanon-c
Copy link

It seems to fix the issue so far! If any new issues come up, I'll open a new issue later. Thank you very much!

@frenic
Copy link

frenic commented Aug 3, 2023

Why is @types/node added in this PR?

"dependencies": {
    "@types/node": "*"
}

It just caused my non-node project to explode with type errors since Jest types are global and @types/jest depends on expect. I cannot see any obvious reason why this dependency needs to be added at all.

@dmitri-gb
Copy link
Contributor Author

I added it because expect now imports a type from the Node.js module async_hooks. But you're right, this can probably be abstracted away such that this type import wouldn't be necessary. I'll try to fix this.

@github-actions
Copy link

github-actions bot commented Sep 3, 2023

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 Sep 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No way to get stable snapshot names with test.concurrent test.concurrent and snapshot support.
5 participants