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

.toMatchSnapshot(?string) #2094

Merged
merged 4 commits into from Nov 29, 2016

Conversation

@ericclemmons
Copy link
Contributor

commented Nov 15, 2016

Summary

In reference to #1392, changes .toMatchSnapshot() to .toMatchSnapshot(?string).

This is primarily so that dynamic tests can have more meaningful snapshot names, compared to having the same name with a different numeric suffix.

See: c46d1b7.

Test plan

  1. npm run watch
  2. npm run jest ./integration_tests/__tests__/toMatchSnapshot-test.js

See: 8d05bec.

@ericclemmons ericclemmons changed the title 1392 snapshot name .toSnapShot(?string) Nov 15, 2016

@ericclemmons ericclemmons changed the title .toSnapShot(?string) .toMatchSnapshot(?string) Nov 15, 2016

@ericclemmons

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2016

I just banged this out & going to bed. I'll check feedback in the morning.

@jlfwong

This comment has been minimized.

Copy link

commented Nov 15, 2016

Woohoo!

It's not super obvious to me from the diff, but it looks to me like the passed named overrides the entire snapshot name. That means that the snapshot from one test could override the snapshot from another test, right? I think the actual behavior I'd want is for the passed string to be append and used in place of the autoincrementing number.

Let me know if I'm misunderstanding the diff!

@ericclemmons

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2016

@jlfwong You know what, you could be right!

My use-case is solving the name here:

https://github.com/HigherEducation/eslint-config/pull/34/files#diff-f47a23810bef105f53bb065dec916592

I figured that:

  • The snap filename is already scoped to the test name, so no problems there.
  • .toMatchSnapshot('foo') would turn examples should match snapshots 1 into foo.

Are you recommending that .toMatchSnapshot('foo') override only the numeric suffix?

In my scenario:

  • .toMatchSnapshot('foo') would be examples should match snapshots foo.
@jlfwong

This comment has been minimized.

Copy link

commented Nov 15, 2016

Yep, that is my suggestion -- I would want to be able to use the same snapshot "name" across tests.

Although I'm not even a contributor in this repo, so we'll have to wait and see what the owners say!

@ericclemmons

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2016

Awaiting feedback from @cpojer or someone else on next steps.

(FYI, Yarn borked the build.)

@cpojer

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2016

Hey! I'm sorry, I'm about to embark on a vacation and I will get back to this next week. Overall, lgtm though so happy to merge it when I'm back and make a release :)

@cpojer cpojer force-pushed the ericclemmons:1392-snapshot-name branch from d5f4fa9 to 12b6cbe Nov 29, 2016

@cpojer cpojer merged commit ea68921 into facebook:master Nov 29, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017

.toMatchSnapshot(?string) (facebook#2094)
* toMatchSnapshot accepts a custom testName

* Update toMatchSnapshot test to accept custom name

* Update toMatchSnapshot documentation regarding custom name

* Add a note to the API docs.

Haroenv added a commit to Haroenv/jest that referenced this pull request Sep 11, 2017

feat(snapshot): concatenate name of test and snapshot
I find the context of the test name and the snapshot name very useful in cases where you have multiple different

Meanwhile I also added the option to add a testName to `toThrowErrorMatchingSnapshot`.

I inspired from facebook#2094, but couldn't really find back where the exact tests are I need to change for this to work.

Let me know if I'm on the correct path

cpojer added a commit that referenced this pull request Oct 10, 2017

feat(snapshot): concatenate name of test and snapshot (#4460)
* feat(snapshot): concatenate name of test and snapshot

I find the context of the test name and the snapshot name very useful in cases where you have multiple different

Meanwhile I also added the option to add a testName to `toThrowErrorMatchingSnapshot`.

I inspired from #2094, but couldn't really find back where the exact tests are I need to change for this to work.

Let me know if I'm on the correct path

* update test

* fix: make sure currentTestName exists
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.