-
Notifications
You must be signed in to change notification settings - Fork 16
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: allowSizeMismatch error messaging #70
base: master
Are you sure you want to change the base?
fix: allowSizeMismatch error messaging #70
Conversation
Elte156
commented
Jan 8, 2025
•
edited
Loading
edited
- Produces more accurate error messaging regarding image size diffs
- My team had some initial confusion as to why snapshot testing failed and the error messaging about image size was a red herring
- Saw this comment Screenshot height changes between runs. jaredpalmer/cypress-image-snapshot#137 (comment)
71bded9
to
86d5b55
Compare
diffRatio * 100 | ||
}% different from saved snapshot with ${diffPixelCount} different pixels.\nSee diff for details: ${diffOutputPath}` | ||
const message = | ||
diffSize && !options.allowSizeMismatch |
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.
Is the intention here to make this something the user can pass in?
Ideally we should add it to the options type - https://github.com/simonsmith/cypress-image-snapshot/blob/master/src/types.ts#L38-L45
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.
- That was not my original intention
- I think
allowSizeMismatch
is already a native option to pass in via jest-image-snapshot
- I think
- This small change is to respect that option and report on the actual cause of error
- If the diff is different and wrong size, the error will always show the wrong size message
- With this small change; the error will accurately show the different message because
allowSizeMismatch
is true (so we should forgo showing the wrong size message)
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.
Understood, I didn't realise it was part of Jest 👍🏻
Looks like it might just need a test fix, and should be all good
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 tests are failing in the pipeline, but it doesn't seem like my small change would have caused this or require a new baseline of snapshots.
I ran the tests locally on both master
and this branch. They both succeeded with 13 passing tests.
yarn docker:build
yarn docker:run
Something doesn't seem right. @simonsmith can I get a sanity check here?
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.
Okay, if they're passing locally maybe a problem I've introduced on CI. I'll take a look for you. Thanks for checking!