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

Display n° of unmatched snapshots in the snapshot summary + "u" usage suggestion #77

Conversation

thomasbertet
Copy link
Contributor

Hi there, this is my attempt to fix #61

Goal

  • Have the "snapshot summary" with correct informations about unmatched number of snapshots.
  • Have the "u" & "i" usage suggestions in Jest watch mode. (update snapshot)

How

  • I updated the counter of the "unmatched" snapshots in the singleton snapshotState when images differs.
  • I also refactored a little the updateSnapshotState, I hope you don't mind... there was a mix between merging & assigning to the singleton (the one & only snapshotState Jest is aware of and bases its report on)

Results

  • Jest now displays the number of unmatched snapshots + hints about the "u" to update snapshots.

How to test

  • Edit a test, change the rendering, Jest should display › 1 snapshot test failed in 1 test suite. Inspect your code changes or press u to update them.

Let me know if you need anything else to have this merge. Thanks!

@CLAassistant
Copy link

CLAassistant commented May 15, 2018

CLA assistant check
All committers have signed the CLA.

@thomasbertet
Copy link
Contributor Author

@anescobar1991 I see tests are failing. I know exactly why and I did some work to make it work ... but I'm not sure it enters in the scope of this PR.
I'm going to create another branch/PR to explain what I did as this may open new discussions.

@anescobar1991
Copy link
Member

@thomasbertet this looks good! I do want to refactor the snapshot state related code at some point as I may be able to make use of some of the functions that jest-snapshot provides but I will do that later!

I am curious about the tests, the test results looks funny because they show all tests passing but 7 snapshots failing. Is that a result of the changes you made?

@thomasbertet
Copy link
Contributor Author

Yes well indeed it looks funny :) That is why I created the other branch #78

Is that a result of the changes you made?

Yep, when you attempt to generate the screenshot and it fails, since I update the "singleton" snapshotState, Jest is aware of snapshots not matching expected shape, thus displaying the info.

My work on the new matcher is supposed to handle this case, when you want a specific snapshot not to match.
Jest made this exact same thing to avoid displaying the report when you on purpose expect the snapshot to fail.

@thomasbertet
Copy link
Contributor Author

@anescobar1991 it is not pretty, but it works... I'm not really happy with this but nothing came up quite easily. Not updating the snapshotState is something deep enough that it needs some work around.
What do you think about this ?

@anescobar1991
Copy link
Member

@thomasbertet I think this is fine. Just left one comment that should be easy to address. I might refactor the integration tests to avoid having to use that global but that is for me to worry about in my own separate PR that should not block this.

@thomasbertet
Copy link
Contributor Author

@anescobar1991 sorry but it seems you did not left your comment ... or I am missing something ? 🦉

src/index.js Outdated
function updateSnapshotState(oldSnapshotState, newSnapshotState) {
return merge({}, oldSnapshotState, newSnapshotState);
function updateSnapshotState(originalSnapshotState, partialSnapshotState) {
if (global.skipReporting) {
Copy link
Member

Choose a reason for hiding this comment

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

maybe name this variable in a way that makes it obvious that it is for test purposes only? Something like UNSTABLE_SKIP_REPORTING following the React convention? IDK

@anescobar1991
Copy link
Member

Yeah I had submitted a review but it never showed up. Now you should be able to see it though. @thomasbertet

@thomasbertet
Copy link
Contributor Author

Yep thanks. It's done !

@anescobar1991 anescobar1991 merged commit 0ce55c6 into americanexpress:master Jun 7, 2018
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.

It is not possible to update screenshots in interactive mode
4 participants