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

fix '-u' suggestion and snapshot summary #9

Merged
merged 1 commit into from
Jun 11, 2018

Conversation

thomasbertet
Copy link
Contributor

@thomasbertet thomasbertet commented May 15, 2018

Hi,

Goal

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

How

  • I just sum-up the common counter with each value of the "child" state counters.

Results

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

How to test

  • Launch your example, add a story on the fly, Jest should display "1 snapshot written".
  • 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 :D
Also, thanks for your work on this, I believe there was some resistance from Jest core team to add this functionnality into core, so this package helps a lot :)
Also you may want to follow what's going on there since there might have a future official and less hacky way of doing it!

});
});

function toMatchSpecificSnapshot(received, snapshotFile, testName) {
const absoluteSnapshotFile = getAbsolutePathToSnapshot(this.testPath, snapshotFile);

const commonSnapshotState = this.snapshotState;
// store the common state to re-use it in "afterAll" hook.
commonSnapshotState = this.snapshotState;
Copy link
Owner

Choose a reason for hiding this comment

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

As I understand this will be replaced on every toMatchSpecificSnapshot call ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed its is the only way we can get the original snapshotState. Not sure it worth it to add some checks to only store it the first time we get there. Wdty ?

Copy link
Owner

Choose a reason for hiding this comment

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

I have to debug it a bit to understand the whole picture =)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you need more intel, please tell me :) Don't want to rush things, just would love to see this happen so it eases the process of updating snapshots in storyshots.

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, will get to it this week. Just was a bit out of focus.

Copy link
Owner

@igor-dv igor-dv left a comment

Choose a reason for hiding this comment

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

@thomasbertet I've checked it now, and it looks working. one thing though - if I have two different tests that are saving to the same snapshot file, I am getting a permission error on watch: EPERM: operation not permitted, lstat.

@thomasbertet
Copy link
Contributor Author

@igor-dv I'm thinking this is not related to this fix, is it ?
I'm not really sure how this change could do anything on the file system 🤐

@igor-dv
Copy link
Owner

igor-dv commented Jun 8, 2018

I think that the problem is that when adding a watch flag, jest starts watching changes on files, and if two different tests are editing the same snapshot (the same file), it conflicts because this snapshot file is locked or something like this. I think this scenario is rare, so I will approve the changes to check them with storyshots.

@igor-dv igor-dv merged commit c59a498 into igor-dv:master Jun 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants