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 match named snapshot #14045

Merged
merged 33 commits into from Jun 21, 2023
Merged

Conversation

bakamitai456
Copy link
Contributor

@bakamitai456 bakamitai456 commented Apr 3, 2023

Summary

Adds a toMatchNamedSnapshot matcher for use:

  • in concurrent test mode
  • for people who want to name their snapshots for whatever reason.

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

  • add unit test in jest-snapshot - ensure the new matcher work like the old one
  • e2e test - ensure the new matcher can use with concurrent testing

@bakamitai456 bakamitai456 changed the title Add match named snapshot feat: add match named snapshot Apr 3, 2023
@bakamitai456 bakamitai456 reopened this Apr 7, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@mrazauskas mrazauskas left a 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:

https://github.com/facebook/jest/blob/285b40d5954fd81e07b02db92185fd317ba92578/packages/jest-types/__typetests__/expect.test.ts#L342-L345

CHANGELOG.md Outdated Show resolved Hide resolved
packages/jest-snapshot/src/types.ts Outdated Show resolved Hide resolved
packages/jest-snapshot/src/types.ts Outdated Show resolved Hide resolved
packages/jest-snapshot/src/types.ts Outdated Show resolved Hide resolved
packages/jest-snapshot/src/types.ts Outdated Show resolved Hide resolved
packages/jest-snapshot/src/types.ts Outdated Show resolved Hide resolved
packages/jest-snapshot/src/types.ts Outdated Show resolved Hide resolved
packages/jest-snapshot/src/types.ts Outdated Show resolved Hide resolved
packages/jest-snapshot/src/index.ts Outdated Show resolved Hide resolved
@bakamitai456
Copy link
Contributor Author

@mrazauskas I make the snapshot parameter to be mandatory to prevent unexpected behavior. Let users use toMatchSnapshot instead if they don't need to specify the snapshot name. I also fixed all reviews. Thank you for your time.

Copy link
Contributor

@mrazauskas mrazauskas left a 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.

packages/jest-snapshot/src/types.ts Outdated Show resolved Hide resolved
packages/jest-snapshot/src/types.ts Outdated Show resolved Hide resolved
packages/jest-snapshot/src/index.ts Outdated Show resolved Hide resolved
packages/jest-snapshot/src/index.ts Outdated Show resolved Hide resolved
docs/SnapshotTesting.md Outdated Show resolved Hide resolved
docs/ExpectAPI.md Outdated Show resolved Hide resolved
docs/ExpectAPI.md Outdated Show resolved Hide resolved
e2e/__tests__/toThrowErrorMatchingSnapshot.test.ts Outdated Show resolved Hide resolved
docs/ExpectAPI.md Outdated Show resolved Hide resolved
docs/SnapshotTesting.md Outdated Show resolved Hide resolved
Copy link
Contributor

@mrazauskas mrazauskas left a 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.

docs/SnapshotTesting.md Outdated Show resolved Hide resolved
@bakamitai456
Copy link
Contributor Author

@SimenB Could you continue reviewing this PR?

@SimenB
Copy link
Member

SimenB commented Apr 20, 2023

Can we detect if the same name is used multiple times within a test file and throw an error?

@mrazauskas
Copy link
Contributor

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.

@SimenB
Copy link
Member

SimenB commented Apr 20, 2023

Good idea! Could you open an issue for the eslint plugin?

@SimenB
Copy link
Member

SimenB commented Apr 22, 2023

@bakamitai456 sorry, could you sign the CLA? We've moved out of the Facebook GH org, so a new one is now used.

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.

@SimenB
Copy link
Member

SimenB commented Apr 22, 2023

/easycla

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 22, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@bakamitai456
Copy link
Contributor Author

/easycla

@bakamitai456
Copy link
Contributor Author

@SimenB I add the code that check the duplicate snapshot name. It check from the snapshot key that produce from match function. Is it okay with this solution?

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!

@SimenB SimenB merged commit ab13484 into jestjs:main Jun 21, 2023
7 of 9 checks passed
@SimenB
Copy link
Member

SimenB commented Jun 30, 2023

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!

SimenB added a commit to SimenB/jest that referenced this pull request Jun 30, 2023
@github-actions
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 Jul 31, 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.

None yet

4 participants