cypress-match-screenshot icon indicating copy to clipboard operation
cypress-match-screenshot copied to clipboard

Change updateScreenshots config to cause tests to fail

Open oliveroneill opened this issue 5 years ago • 2 comments

To avoid false positives, it makes sense to fail tests when updating screenshots. This way there's no way you could accidentally leave updateScreenshots set to true without noticing.

oliveroneill avatar Dec 17 '18 10:12 oliveroneill

Thanks for taking the time to make this PR :blush:

Hm, I get your point, not sure if making the tests fail is the best approach though. Assuming you have multiple screenshot matches (maybe even within the same test) means it will always only replace the first screenshot and then bail because the test failed. Or am I missing something?

From my experience the way it works in things like Jest snapshots and probably should work here as well is to show a notification how many screenshots were matched and how many screenshots were replaced with new screenshots at the end of the test (or maybe as part of the assertions, basically make the test pass but mention in the assertion that the screenshot didn't match and got updated according to settings).

Thoughts?

julianburr avatar Dec 29 '18 06:12 julianburr

Good point about it failing before updating all screenshots - I hadn't considered multiple screenshot checks within the same test. I can look into fixing this.

You might be right that tests failing on updated screenshots isn't the standard way to do it. I was basing my experience on ios-snapshot-test-case, which fails the test with a useful message stating that the snapshots have been updated. I found it to be a nice feature to avoid accidentally leaving the update setting enabled.

Happy to close the pull request if this isn't the standard way of doing things. Potentially it's not worth the effort. Let me know what you think.

oliveroneill avatar Dec 29 '18 06:12 oliveroneill