-
Notifications
You must be signed in to change notification settings - Fork 200
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: allow storing received screenshot on failure #298
feat: allow storing received screenshot on failure #298
Conversation
Feedback why this is a good/bad idea would be appreciated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a useful change. thanks for the contribution
@@ -219,12 +223,38 @@ describe('diff-snapshot', () => { | |||
expect(mockWriteFileSync).toHaveBeenCalledTimes(1); | |||
}); | |||
|
|||
it('should write a received image if the test fails and storeReceivedOnFailure = true', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add another test for when storeReceivedOnFailure
is false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an explicit test case for this situation. However, this was already tested implicitly since false is the default. I did change the implementation a little bit so that it also does not return the receivedSnapshotPath
if storeReceivedOnFailure
is set to false.
snapshotIdentifier: mockSnapshotIdentifier, | ||
snapshotsDir: mockSnapshotsDir, | ||
storeReceivedOnFailure: true, | ||
receivedDir: mockReceivedDir, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add a case for when receivedDir
is undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check for undefined receivedDir
and other custom paths is handled in the index.js
file. There did not seem to be a specific test that checked for default values, so I added that in the index.spec.js
file.
@Matthew-Mallimo could you check the changes I made? |
# [5.1.0](v5.0.0...v5.1.0) (2022-05-30) ### Features * allow storing received screenshot on failure ([#298](#298)) ([cfb81c9](cfb81c9))
🎉 This PR is included in version 5.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [5.1.0](americanexpress/jest-image-snapshot@v5.0.0...v5.1.0) (2022-05-30) ### Features * allow storing received screenshot on failure ([americanexpress#298](americanexpress#298)) ([cfb81c9](americanexpress@cfb81c9))
Description
Currently, when a screenshot test fails, the actual (received) image is not stored, other than in a diff image. This PR allows users to set 2 variables:
storeReceivedOnFailure
andcustomReceivedDir
.When a test fails, and
storeReceivedOnFailure
is set to true, the received image will also be stored. You can optionally specify a custom location where to store these images by settingcustomReceivedDir
.Not really sure about the naming. Could also use actual instead of received. The feature is currently an addition, but could also be a toggle between storing the composed diff image or storing seperate images. 🤷♂️
Motivation and Context
This fixes #260 .
This is useful when running in CI. If a screenshot test fails. you can get a new baseline image directly from the build and commit it as the new baseline if the change is as expected.
How Has This Been Tested?
Added unit tests. Tested it locally.
Types of Changes
Checklist:
What is the Impact to Developers Using Jest-Image-Snapshot?
Impact should be none. Use of this feature is optional.