Allow to generate diff for images with different sizes #42

sbekrin · 2018-01-19T17:01:09Z

Hey there 👋

I've noticed that in some cases image snapshots might be different in size, so 1px difference fails test without helpful diff.

This PR adds new opt-in failOnSizeMismatch configuration flag which is true by default and keeps current behaviour. With set to false it would generate diff snapshot even for non-identical in size images which is helpful. Also, exact images with different transparent padding on right and bottom would also pass with that flag w/o cropping.

Thanks!

CLAassistant · 2018-01-19T17:01:16Z

All committers have signed the CLA.

anescobar1991 · 2018-01-19T20:51:00Z

Does it actually work comparing differently sized images? I thought pixelmatch would not work with it? mapbox/pixelmatch#25

Edit: nvm I see what you are doing

anescobar1991 · 2018-01-20T02:42:13Z

Hi @sergeybekrin!

So I guess my first question is why are the image snapshots you are generating different in size sometimes?
And is this something that should be taken care of by jest-image-snapshot or should the user pre-process the image to make sure it is the same size as the baseline image? 🤔

@jking90 @PixnBits what are your thoughts?

sbekrin · 2018-01-20T12:30:35Z

@anescobar1991 we're using it to test visual regression based on screenshots — due to environment's factor we have to crop it first, resulting in different snapshot size if styling and/or content has changed.

I agree this might sound specific, but since we don't have an access to baseline image which we need to align in size in some case too, I don't see any other way to do it.

anescobar1991 · 2018-01-22T20:42:00Z

@sergeybekrin It sounds like in the case that the screenshot changes due to environmental factors you should regenerate the snapshots (--updateSnapshot).

sbekrin · 2018-01-22T23:36:37Z

Oh, I didn't explained that part, my bad:

We make a screenshot from non-web environment we don't control
We automatically crop it by visual boundaries to extract target to test visually
At this point any changes in content / styling results in different cropped area size

Update:
Alright, I understood why this change is so confusing. It doesn't make sense to pass test for images with different sizes, but it does make sense to output diff. I've renamed failOnSizeMismatch to outputDiffOnSizeMismatch and updated code respectively.

Sorry for confusion and let me know if you have additional feedback ✌️

anescobar1991 · 2018-01-23T23:29:36Z

So all you really want is for the matcher to behave as usual (fail and produce a diff image) when given an image with different dimensions?

If you remove this line without doing anything else does that work for your use case? (That is assuming I understand your use case correctly)

sbekrin · 2018-01-24T09:32:47Z

So all you really want is for the matcher to behave as usual (fail and produce a diff image) when given an image with different dimensions?

Yep, correct

If you remove this line without doing anything else does that work for your use case? (That is assuming I understand your use case correctly)

pixelmatch would fail in that case, they also have open issue about comparing different sized images and suggestion is to prepare it manually

anescobar1991 · 2018-01-24T18:07:04Z

@sergeybekrin I don't think this is the right solution for your use case. What you should do is resize your image before you pass it to toMatchImageSnapshot() to match the baseline snapshot dimensions.

You can use the customSnapshotsDir and customSnapshotIdentifier options to make the snapshot file path predictable and allow you to easily calculate its dimensions without having to worry too much about the path to the snapshots.

sbekrin · 2018-01-24T18:33:25Z

@anescobar1991 I don't see how that helps in case if received image is bigger. By cropping to smaller size we potentially missing regression.

If you think this is to specific, maybe we should implement a way to optionally preprocess both images before comparison?

anescobar1991 · 2018-01-24T19:09:31Z

@sergeybekrin I don't mean crop. You can actual resize the image using something like sharp pretty easily.

As for the preprocess idea that is interesting... How would you imagine that to be implemented?

sbekrin · 2018-01-25T14:52:10Z

@anescobar1991 alright, I would create a separate PR with an idea and PoC.

anescobar1991 · 2018-01-25T14:56:01Z

@sergeybekrin Did my idea work for you?

sbekrin · 2018-01-25T15:37:35Z

@anescobar1991 unfortunately not, quick try shown that that approach results in unreadable diff because of different proportions after resizing. I've attached a reference for better explanation.

sbekrin · 2018-01-25T16:56:35Z

@anescobar1991 I've created #44 to continue discussion here

thomasbertet · 2018-02-26T13:21:45Z

@anescobar1991 I would like to ask a quick question about this size thing.

Let's say I'm using jest-image-snapshot and using Chrome headless (puppeteer) to take the screenshot.
Puppeteer has an option to take a screenshot of the full page there.
Let's say you are screenshotting a page with a lot of content, a lot more than a default viewport would contains. (ie. the page is 3000px height, and viewport only 728px height)
When you take the screenshot, Chrome will take an image of 3000px height. That is the expected behavior.
If you add some content the page, you might end-up changing the height of the page, thus the screenshot made by Chrome is a bit longer than before.

I believe you will then won't be able to get a diff right ? It will complain both images have not the same size ?

Do I get that wrong ? Do you see any workaround we could use to make it work ? Can we expect pixelmatch to help in such matter?

Thanks in advance for any help / thoughts, and also big 💌 thank you for this lib ! Really love it & use it extensively !

anescobar1991 · 2018-02-26T16:34:57Z

@thomasbertet For the sake of simplicity I am going off of the assumption that you care about the entirety of the page and that your tests cannot be broken up into smaller more focused snapshots (which is what I would suggest as a best practice).

If that is the case then if the image has changed (which is what a height change would be) then what you would want is to see a failing test (with a diff) and then you can either update the snapshots or update your code at test to get back to the original snapshot state right? I can see that removing the image dimensions check would allow for that.

@sergeybekrin this would help you out as well too right? Now that I understand this better I don't think this change would cause any issues.

sbekrin · 2018-02-26T16:42:59Z

@thomasbertet @anescobar1991 removing that check wouldn't be enough since pixelmatch requires both images to be same size, related to mapbox/pixelmatch#25. My strategy was to align both images to max common dimensions to output diff w/o cropping potentially important parts.

thomasbertet · 2018-02-26T17:29:21Z

Thanks for the quick reply @anescobar1991 ! You understood me perfectly.

Yes at first this seems to be enough for me to remove the check.

But, I'm not sure to know what pixelmatch will generate then @sergeybekrin ? Will it generate a diff even badly ? Or nothing at all, throwing an error, meaning we are still at the same step ?

The ideal case would be, as you said @anescobar1991, to be able to generate some kind of a diff.

Maybe we could generate one "manually" ? I mean we may be able to just generate a diff -not by pixelmatch- but by jest-image-snapshot displaying the extra part (or the missing part if the size of the new image is smaller) ?

This way, we would be able to have two kinds of "failure":

Mismatch of the same image size (current behavior)
Mismatch of different image size (new case)

Having nothing to visually compare is sooo sad, I feel like we might just generate the comparison image using a very naive algorithm. Even simplier, just generate the two images side-by-side as diff output for a first step would be quite helpful.

What do you think ? Is this somewhere you would like to go ? I may help if you will.

anescobar1991 · 2018-02-26T17:56:53Z

@sergeybekrin the comparison would not be accurate but it would still occur and jest-image-snapshot would still generate a diff. In that case then a user would just update their snapshot to grab the new one. I tested locally and it was working and I think that is what would help @thomasbertet out. @thomasbertet can you validate locally yourself and see?

Remembering back to what we talked about earlier though, your use case is different than @thomasbertet's in that you would actually want the comparison between differently sized images to be valid. And sadly that, as I mentioned to you earlier, we cannot support unless pixelmatch makes changes.

thomasbertet · 2018-02-26T18:12:09Z

Thanks @anescobar1991 yep, if it does like you said, that's all I need to be very happy :)

I tried and it seemed to not work as expected.

I got an error: bitblt reading outside image and it did not generate the diff :(

anescobar1991 · 2018-02-26T18:17:33Z

🤦‍♂️ I think because when I ran it locally the baseline image was larger than the new image to compare. But if you reverse the scenario it fails with that the message you got...

So back to step 1 then... Let me think about this for a bit.

sbekrin · 2018-02-26T18:27:11Z

@anescobar1991 I actually needed just a pretty diff and that's exactly my case, you can check my first PR with a solution which doesn't pass test but fails with outputting a valid diff

updated: key changes were this and this, failOnSizeMismatch option name might be confusing but it's basically flag which disable that check.

anescobar1991 · 2018-03-06T01:24:45Z

@sergeybekrin opening this back up as discussed in #49. Let me know if you are still willing to work on this, otherwise I can go ahead and pick this up.

If you are still willing to work on it I just have a few comments:

I don't think we need the failOnSizeMismatch option, let's just always create the diff.

HOWEVER, I think we need to always fail the test if the size is different (while still generating that diff of course). Just in case someone has a large failure threshold and the diff is not large enough (but should still fail since the images are different sizes and thus not the same image).

The only thing about that is that we still need to generate some type of output image in case pixelmatch comes back as a pass with differently sized images. I was thinking we could use some checkerboard pattern with some text letting the user know that the images are different sizes yet within the threshold range?

sbekrin · 2018-03-06T11:29:19Z

@anescobar1991 awesome, thanks! Sure I'll work on it. Good point regarding case when there's no diff but size is different, I'll check what we can do for it.

anescobar1991 · 2018-03-08T04:01:48Z

@sergeybekrin do you know when you will be getting to this? Do you think it will be by the end of this week? Wondering if I should wait for this to be merged before publishing next version or not.

sbekrin · 2018-03-08T09:18:24Z

@anescobar1991 yeah, I'll continue on it today. Should be ready by the end of the week.

sbekrin · 2018-03-09T11:25:33Z

src/index.js

        message = () => `Expected image to match or be a close match to snapshot but was ${differencePercentage}% different from snapshot (${result.diffPixelCount} differing pixels).\n`
-                  + `${chalk.bold.red('See diff for details:')} ${chalk.red(result.diffOutputPath)}`;
+                  + `${chalk.bold.red('See diff for details:')} ${chalk.red(result.diffOutputPath.replace(cwd, '.'))}`;


This one is handy change converts absolute diff path to relative which is easier to find (which also allows using .toThrowErrorMatchingSnapshot in jest)

I am not sure it is easier to find/read the path this way.

Maybe it is just me but I think that seeing:
See diff for details /Users/username/path/to/__tests__/__image_snapshots__/diff.png

is easier to read and more clear than:
See diff for details ./__tests__/__image_snapshots__/diff.png

Can you remove this change and just implement something like it at the test level? (in order to allow usage of .toThrowErrorMatchingSnapshot()

I think a change like this for the user experience should be its own PR where a discussion could happen.

Haha, here's tricky part: if I add NODE_ENV === 'test' check it reduces coverage since it's always set to test in Jest env. I'll switch to regex option for now.

sbekrin · 2018-03-09T11:26:37Z

src/diff-snapshot.js

@@ -69,7 +127,7 @@ function diffImageToSnapshot(options) {
    const totalPixels = imageWidth * imageHeight;
    const diffRatio = diffPixelCount / totalPixels;

-    let pass = false;
+    let pass = !hasSizeMismatch;


This marks test as failed, but this is not necessary since resized images are always has diff at this point and would fail the test

what do you mean by but this is not necessary since resized images are always has diff at this point and would fail the test?

Ah nevermid, just realised it's actually a valid case then size difference is small

sbekrin · 2018-03-09T11:27:45Z

src/diff-snapshot.js

+    );
+    // Align images in size if different
+    if (hasSizeMismatch) {
+      const resizedImages = alignImagesToSameSize(receivedImage, baselineImage);


Key change, this resolves both images in same size and with new filled area after resize

sbekrin · 2018-03-09T11:28:34Z

src/diff-snapshot.js

+ * Fills diff area with black transparent color for meaningful diff
+ */
+/* eslint-disable no-plusplus, no-param-reassign, no-bitwise */
+const fillSizeDifference = (width, height) => (image) => {


This function just fills new area added after resize with transparent black color. I like idea of checker board pattern, but it seems to be too complicated to implement considering how low-level pngjs API is.

Yeah the worry we had was that just filling it with color would confuse the user as they would think that the color was part of their passed in image.

Since you made it transparent though when I was running it locally it seemed clear enough that the black color was not a part of the image so this should be fine from my POV at least.

sbekrin · 2018-03-09T11:29:54Z

@anescobar1991 done, I've left some comments for easier review. Cheers!

sbekrin · 2018-03-09T11:40:59Z

Oh, and here's example diff with regular case:

And here's reference how diff would look like in edge case (resized image filled with transparent black pixels) from test suite I've added:

anescobar1991 · 2018-03-09T18:10:11Z

src/diff-snapshot.js

-      receivedImage.height !== baselineImage.height || receivedImage.width !== baselineImage.width
-    ) {
-      throw new Error('toMatchImageSnapshot(): Received image size must match baseline snapshot size in order to make comparison.');
+    let receivedImage = PNG.sync.read(receivedImageBuffer);


I'd rather keep these as const and maybe just rename them rawReceivedImage or something like that like you had before. Then you can have different variables for the "processed" images that will actually be diffed by pixelmatch.

I think it will make it easier to follow and debug this code in the future.

anescobar1991 · 2018-03-09T18:21:33Z

src/diff-snapshot.js

+/**
+ * Fills diff area with black transparent color for meaningful diff
+ */
+/* eslint-disable no-plusplus, no-param-reassign, no-bitwise */


I am okay with disabling no-plusplus since in a loop that is a pretty standard way to iterate that has been around since the beginning of programming. And no-bitwise since you do need it.

However you should definitely not disable no-param-reassign.

no-param-reassign required to mute errors for reassigning image.data value

anescobar1991 · 2018-03-09T18:31:02Z

Alright I made my comments, for the most part it looks good @sergeybekrin!

@10xLaCroixDrinker @PixnBits can you guys review as well?

sbekrin · 2018-03-12T12:45:43Z

@anescobar1991 thanks for review, I've addressed your feedback

anescobar1991 · 2018-03-12T17:34:58Z

src/diff-snapshot.js

@@ -70,7 +127,9 @@ function diffImageToSnapshot(options) {
    const diffRatio = diffPixelCount / totalPixels;

    let pass = false;
-    if (failureThresholdType === 'pixel') {
+    if (hasSizeMismatch) {
+      // Always fail test on image size mismatch


Should we explicitly set pass to false for clarity of what the intent is?

Alright, done

anescobar1991 · 2018-03-13T19:11:44Z

@sergeybekrin merged! Thanks for your patience with this PR, I know there was a lot of back and forth!

thomasbertet · 2018-03-13T19:14:39Z

Thanks @sergeybekrin for bringing this up in the first place & @anescobar1991 for staying open-minded on the subject 👍 Can't wait to try it 💯

sbekrin · 2018-03-13T19:57:05Z

Alright, perfect! Thanks everyone for taking part in this ✌️

emanuelbsilva · 2019-12-31T07:09:10Z

Hi everyone,

Sorry for re-opening this issue almost 2 years after the last comment, but i think it makes sense to be in here.

I'm currently evaluating visual testing, using cypress, however when running different browsers we experience that even though the result images are the same, they have different sizes (with the same ratio, just scaled down/up).

Instead of trying to align the images so that the smallest image is filled with transparent pixels, what if it would scale down the biggest image to match the smallest one?

For instance we have an image A which is 50x50 and another B which 100x100. We could take image B and scale it down to 50x50 and then diff them. Also, this approach only makes sense if both images have the same ratio.

Is this something which make sense for this project? I wouldn't mind tackling it if would suit the project. Do you also have any feedback on this matter? I'm fairly new to visual testing so it might be that i'm overlooking something.

Thanks!

anescobar1991 · 2020-01-03T15:39:17Z

I think doing so makes sense in your case where the ratios are the same but would not make sense to implement in jest-image-snapshot as we cannot make that assumption.

What I would do if I was you is manipulate the images to make them the same size prior to passing them to jest-image-snapshot.

sbekrin changed the title ~~Allow diffing snapshots with different size~~ Allow to generate diff for images with different sizes Jan 22, 2018

anescobar1991 closed this Jan 24, 2018

anescobar1991 mentioned this pull request Feb 1, 2018

Allow preprocessing images before comparison #44

Closed

anescobar1991 mentioned this pull request Feb 26, 2018

"Image size must match baseline" error #49

Closed

thomasbertet mentioned this pull request Mar 5, 2018

fix screenshots tests & add getScreenshotOption to storyshots storybookjs/storybook#3102

Merged

anescobar1991 reopened this Mar 6, 2018

sbekrin force-pushed the feat/handle-size-mismatch branch from 1410859 to e8c756c Compare March 6, 2018 11:47

sbekrin force-pushed the feat/handle-size-mismatch branch from e8c756c to 32861a2 Compare March 9, 2018 11:19

sbekrin commented Mar 9, 2018

View reviewed changes

anescobar1991 reviewed Mar 9, 2018

View reviewed changes

sbekrin force-pushed the feat/handle-size-mismatch branch from 32861a2 to b68935a Compare March 12, 2018 12:45

anescobar1991 requested a review from 10xLaCroixDrinker March 12, 2018 17:15

anescobar1991 reviewed Mar 12, 2018

View reviewed changes

feat(diff-snapshot): handle image sizes mismatch

f74df4e

sbekrin force-pushed the feat/handle-size-mismatch branch from b68935a to f74df4e Compare March 13, 2018 13:36

10xLaCroixDrinker approved these changes Mar 13, 2018

View reviewed changes

anescobar1991 merged commit fef4dd2 into americanexpress:master Mar 13, 2018

sbekrin deleted the feat/handle-size-mismatch branch March 13, 2018 19:55

goverdhan07 pushed a commit to goverdhan07/jest-image-snapshot that referenced this pull request Jul 23, 2023

feat(diff-snapshot): handle image sizes mismatch (americanexpress#42)

b967302

Allow to generate diff for images with different sizes #42

Allow to generate diff for images with different sizes #42

Conversation

sbekrin commented Jan 19, 2018

CLAassistant commented Jan 19, 2018 • edited Loading

anescobar1991 commented Jan 19, 2018 • edited Loading

anescobar1991 commented Jan 20, 2018

sbekrin commented Jan 20, 2018

anescobar1991 commented Jan 22, 2018

sbekrin commented Jan 22, 2018 • edited Loading

anescobar1991 commented Jan 23, 2018 • edited Loading

sbekrin commented Jan 24, 2018 • edited Loading

anescobar1991 commented Jan 24, 2018

sbekrin commented Jan 24, 2018 • edited Loading

anescobar1991 commented Jan 24, 2018

sbekrin commented Jan 25, 2018

anescobar1991 commented Jan 25, 2018

sbekrin commented Jan 25, 2018 • edited Loading

sbekrin commented Jan 25, 2018

thomasbertet commented Feb 26, 2018 • edited Loading

anescobar1991 commented Feb 26, 2018

sbekrin commented Feb 26, 2018

thomasbertet commented Feb 26, 2018

anescobar1991 commented Feb 26, 2018

thomasbertet commented Feb 26, 2018

anescobar1991 commented Feb 26, 2018

sbekrin commented Feb 26, 2018 • edited Loading

anescobar1991 commented Mar 6, 2018

sbekrin commented Mar 6, 2018

anescobar1991 commented Mar 8, 2018

sbekrin commented Mar 8, 2018

sbekrin Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbekrin Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

sbekrin Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbekrin Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbekrin commented Mar 9, 2018

sbekrin commented Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anescobar1991 commented Mar 9, 2018

sbekrin commented Mar 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anescobar1991 commented Mar 13, 2018

thomasbertet commented Mar 13, 2018

sbekrin commented Mar 13, 2018

emanuelbsilva commented Dec 31, 2019

anescobar1991 commented Jan 3, 2020

CLAassistant commented Jan 19, 2018 •

edited

Loading

anescobar1991 commented Jan 19, 2018 •

edited

Loading

sbekrin commented Jan 22, 2018 •

edited

Loading

anescobar1991 commented Jan 23, 2018 •

edited

Loading

sbekrin commented Jan 24, 2018 •

edited

Loading

sbekrin commented Jan 24, 2018 •

edited

Loading

sbekrin commented Jan 25, 2018 •

edited

Loading

thomasbertet commented Feb 26, 2018 •

edited

Loading

sbekrin commented Feb 26, 2018 •

edited

Loading

sbekrin Mar 9, 2018 •

edited

Loading

sbekrin Mar 12, 2018 •

edited

Loading

sbekrin Mar 9, 2018 •

edited

Loading

sbekrin Mar 9, 2018 •

edited

Loading

sbekrin commented Mar 9, 2018 •

edited

Loading