-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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 match named snapshot #14045
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really right to mark the snapshotName
argument as optional? The documentation states that the specific name will be used to find the snapshot. If the name is not provided, how will this happen?
Thanks for type tests. Could you add few more e2e type tests to this file, please:
8ba1da0
to
efef0bd
Compare
@mrazauskas I make the snapshot parameter to be mandatory to prevent unexpected behavior. Let users use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Few more details to look at.
Could you add e2e type tests for the toThrowErrorMatchingNamedSnapshot
marcher too?
You should also merge the main
branch and resolve merge conflict.
b1ef690
to
b305da6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! All looks good to me. Let’s wait for maintainer’s review.
It feels somewhat strange to add a matcher almost identical to .toMatchSnapshot()
. At the same time, it is hard to find a better solution for snapshot testing in concurrent mode.
@SimenB Could you continue reviewing this PR? |
Can we detect if the same name is used multiple times within a test file and throw an error? |
By the way, this sounds like a good idea for a lint rule as well. |
Good idea! Could you open an issue for the eslint plugin? |
@bakamitai456 sorry, could you sign the CLA? We've moved out of the Facebook GH org, so a new one is now used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/easycla |
|
/easycla |
077fa8a
to
6614da3
Compare
Co-authored-by: Tom Mrazauskas <[email protected]>
Co-authored-by: Tom Mrazauskas <[email protected]>
fdbf720
to
5e58009
Compare
@SimenB I add the code that check the duplicate snapshot name. It check from the snapshot key that produce from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
Hiya! After some consideration, I'll revert this and go with #14139 instead - it solves the same issue without needing a new matcher. Thanks for the time spent on this PR, though! |
This reverts commit ab13484.
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
Adds a toMatchNamedSnapshot matcher for use:
toMatchSnapshot
could not use with the concurrent test because it uses the current test name to select the snapshot to compare with the received result.I found #11605. He tried to add a new snapshot matcher that specifies the snapshot name directly. I think it could solve the problem but the PR was closed by the GitHub bot due to inactive activity. #2180 This issue is closed too.
Test plan
jest-snapshot
- ensure the new matcher work like the old one