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

feat: images of difference sizes cause instant test failure #94

Closed
dhulme opened this issue Aug 24, 2022 · 15 comments · Fixed by #108
Closed

feat: images of difference sizes cause instant test failure #94

dhulme opened this issue Aug 24, 2022 · 15 comments · Fixed by #108
Assignees
Labels
enhancement New feature or request

Comments

@dhulme
Copy link
Contributor

dhulme commented Aug 24, 2022

Sometimes Cypress takes screenshots of slightly differences sizes during the same test. If the screnshots are different sizes the test fails. But I notice in the code that it calls alignImagesToSameSize function if they are different sizes to try and do the comparison on resized images. If the test always fails what is the reason for alignImagesToSameSize? Perhaps it should attempt to continue with the comparison after resizing the images?

Example of the diff we see in screenshot size between runs:

image

@dhulme dhulme added the bug Something isn't working label Aug 24, 2022
@FRSgit
Copy link
Member

FRSgit commented Aug 24, 2022

Hi @dhulme,

Code indeed calls alignImagesToSameSize method to make the images used for the pixelDiff to have the same dimensions. So unless you get some explicit error in the console I would say that this part of code should work well.
I think there might be a big enough difference between new and old image and file diff fails.

Also - why the cypress creates differently sized screenshots? Are there any differences between environments the screenshots are created on? I'm asking, because it doesn't happen in the projects where I'm using this plugin - I might need more info to reproduce that situation.

Can you please post here:

  • the dump of console output or screenshot from Cypress UI visible when running tests,
  • the .diff.png file generated by the plugin.

Also could you add additional information about your environment:

OS and version: [e.g. Windows 10 build. 19043.1319]
Browser and version [e.g. chrome 22]
Cypress version [e.g. 8.6.0]
Node version: [e.g. 16.0.1]

@FRSgit FRSgit self-assigned this Aug 24, 2022
@dhulme
Copy link
Contributor Author

dhulme commented Aug 25, 2022

Hi @FRSgit, thanks for your reply.

When alignImagesToSameSize is called, isImgSizeDifferent willl be true so it fails automatically on line 166, ignoring the result of pixelmatch? So even if the resized images were below tolerance, it will still fail?

I'm not sure why Cypress is captuing different sized images. It only happens on a couple of my components, The images are all captured on a Linux container in a pipeline (cypress/browsers:node16.14.2-slim-chrome103-ff102), so they should be consistent. I will investigate further. I'm on Cypress 10.4.0.

@FRSgit
Copy link
Member

FRSgit commented Aug 25, 2022

When alignImagesToSameSize is called, isImgSizeDifferent willl be true so it fails automatically on line 166, ignoring the result of pixelmatch? So even if the resized images were below tolerance, it will still fail?

My bad @dhulme, you're right - if images have different sizes the test will always fail. The image size check can be removed, however that would be a breaking change - I do not want to make without giving other possibility to react on it.

I'm not sure why Cypress is captuing different sized images. It only happens on a couple of my components, The images are all captured on a Linux container in a pipeline (cypress/browsers:node16.14.2-slim-chrome103-ff102), so they should be consistent. I will investigate further. I'm on Cypress 10.4.0.

Are you capturing images during E2E or component-testing?
If during CT then you might want to try to make screenshots of the whole viewport (scale in each test using viewport command so it fits your component).

@dhulme
Copy link
Contributor Author

dhulme commented Aug 25, 2022

My bad @dhulme, you're right - if images have different sizes the test will always fail. The image size check can be removed, however that would be a breaking change - I do not want to make without giving other possibility to react on it.

Ok thanks, that makes sense. What would be the process for collecting feedback on the change?

Are you capturing images during E2E or component-testing?
If during CT then you might want to try to make screenshots of the whole viewport (scale in each test using viewport command so it fits your component).

This is in component testing yes. In most cases I've been screenshoting the element in question e.g. button, not the whole viewport. Are you aware of issues with screenshotting elements rather than the viewport?

@FRSgit
Copy link
Member

FRSgit commented Aug 28, 2022

@dhulme As I won't have time to look into the issue this week I would let it sit here for the time being and see if anybody comes around. If nobody will respond - then it will be up to us 2 (the library doesn't have that much of an audience) 🤷
In the meantime you can give it a try with a PR if you want to - it will surely speed up the process of shipping it!

This is in component testing yes. In most cases I've been screenshoting the element in question e.g. button, not the whole viewport. Are you aware of issues with screenshotting elements rather than the viewport?

No, I'm not. Quite the contrary - I'm using this plugin in projects where elements are being image targets and it works well.
Even in this repo we run on the CI a test that makes a screenshot of a "description" element.
I would check if there isn't anything interfering with how your element gets rendered. Maybe asynchronously loaded CSS? Or not every time font is being rendered fast enough? Other elements are pushing this one throughout the screen? Maybe you're able to pinpoint the problem in your codebase.

@oguzhantx
Copy link

I am having same screenshot size issue with component tests as well. Somehow cypress is not picking up the specified view port when it comes to taking a screenshots. Also having a hard time to match locally uploaded screenshots to match the ones in the ci pipeline. Any suggestion on this?

@FRSgit
Copy link
Member

FRSgit commented Aug 30, 2022

Hey @oguzhantx!
Can you please open up a new bug request for each of the issues you've mentioned? 🙏 I believe that viewport/screenshot size issues could be tried to looked up on the cypress repository as well.

Regarding mismatch between local and ci screenshots please provide additional data in the new issue you create - example screenshots or at least description what's different between them. And also - elaborate please on what does does it mean to "match locally uploaded screenshots to match the ones in the ci pipeline"? How do you try to do such matching and what do you want to achieve as a result?
Cheers 👋

@oguzhantx
Copy link

`@

Hey @oguzhantx! Can you please open up a new bug request for each of the issues you've mentioned? 🙏 I believe that viewport/screenshot size issues could be tried to looked up on the cypress repository as well.

Regarding mismatch between local and ci screenshots please provide additional data in the new issue you create - example screenshots or at least description what's different between them. And also - elaborate please on what does does it mean to "match locally uploaded screenshots to match the ones in the ci pipeline"? How do you try to do such matching and what do you want to achieve as a result? Cheers 👋

I was able to fix the first screenshot problem. looks like setting up the viewport in config file or beforeeach() block does not set the screenshot size by itself. For the screenshot size, I had to tell screenshot to use viewport as well. so this worked --> Cypress.Screenshot.defaults({ capture: 'viewport' });

When I say mismatch, I meant uploading the base screenshots from your local to repo then you run your tests in ci. then test fails in ci because screenshots that are taken on my machine is different than the ones in the ci. Here is the sample failure screenshot. (blocked some content)
image

@FRSgit
Copy link
Member

FRSgit commented Aug 31, 2022

@oguzhantx Great to hear that you've been able to fix the first issue. I've added info about that to the README - could you review the PR to check if I got it correctly please?

Regading the mismatch - screenshot done locally will almost always be different than the one on the CI. There are differences in the OS (e.g. every OS renders fonts a bit differently), in headless vs windowed mode and other things. The main flow for working with the screenshots is to generate & compare them on the CI (and maybe generate & compare other copy of screenshots for better developer experience). If you feel that there is still something left regarding this topic - please open up a new issue as it's not connected with the one created by @dhulme

@FRSgit FRSgit added enhancement New feature or request and removed bug Something isn't working labels Aug 31, 2022
@FRSgit FRSgit changed the title Images of difference sizes cause instant test failure feat: images of difference sizes cause instant test failure Aug 31, 2022
FRSgit added a commit that referenced this issue Sep 4, 2022
BREAKING CHANGE: different resolution doesn't fail test immediately - img diff is being done

closes #94

Signed-off-by: Jakub Freisler <jakub@frsource.org>
@FRSgit
Copy link
Member

FRSgit commented Sep 4, 2022

Hey @dhulme!
I've just created new PR resolving this issue: #108. I've published it to npm under 2.0.0-alpha.0, can you give it a try please? 🙏

@dhulme
Copy link
Contributor Author

dhulme commented Sep 15, 2022

Hi @FRSgit, sorry for the delay. That's working great, thanks!

@FRSgit
Copy link
Member

FRSgit commented Sep 15, 2022

Uf, I started wondering what should I do with this PR 😄 Thanks for confirming that it works @dhulme!
I'll do a full 2.0.0 release later today

FRSgit added a commit that referenced this issue Sep 15, 2022
BREAKING CHANGE: different resolution doesn't fail test immediately - img diff is being done

closes #94

Signed-off-by: Jakub Freisler <jakub@frsource.org>
FRSgit added a commit that referenced this issue Sep 15, 2022
BREAKING CHANGE: different resolution doesn't fail test immediately - img diff is being done
closes #94
github-actions bot pushed a commit that referenced this issue Sep 15, 2022
# [2.0.0](v1.9.21...v2.0.0) (2022-09-15)

### Features

* img diff when resolution differs ([#108](#108)) ([c8a5044](c8a5044)), closes [#94](#94)

### BREAKING CHANGES

* different resolution doesn't fail test immediately - img diff is being done
@FRSgit
Copy link
Member

FRSgit commented Sep 15, 2022

Released 🎉

@idasilva1
Copy link

@oguzhantx Great to hear that you've been able to fix the first issue. I've added info about that to the README - could you review the PR to check if I got it correctly please?

Regading the mismatch - screenshot done locally will almost always be different than the one on the CI. There are differences in the OS (e.g. every OS renders fonts a bit differently), in headless vs windowed mode and other things. The main flow for working with the screenshots is to generate & compare them on the CI (and maybe generate & compare other copy of screenshots for better developer experience). If you feel that there is still something left regarding this topic - please open up a new issue as it's not connected with the one created by @dhulme

@FRSgit I'm actually having the same issue running cypress in headless mode but changing monitors.
If I run the matchImage for the first time, in headless, using one monitor, when I run it with a different monitor, even in headless, the snapshot fails with just some fonts mismatching. Any idea what this could be?

@FRSgit
Copy link
Member

FRSgit commented Oct 6, 2022

@idasilva1 unfortunately the screen density has an effect on the rendering of a webpage - which might be causing the test to fail :(
Also, it's possible to force the same resolution for different screens/modes only in chrome testing as the only solution to this issue works for Chrome-only.

Cypress says this issue was fixed in 8.0.0, but well - not completely as can be seen here, here and here.

From my experience I would propose to:

  • setup capture: 'viewport' configuration (more details in our FAQ section),
  • always try to make a screenshot of a part of your screen (never make a screenshot of a full page with cy.matchImage() as the size of the cypress window might differ).

github-actions bot pushed a commit to braze-inc/cypress-plugin-visual-regression-diff that referenced this issue Oct 31, 2022
# 1.0.0 (2022-10-31)

### Bug Fixes

* btoa utf8 encoding/decoding error ([FRSOURCE#114](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/114)) ([0137014](0137014))
* create missing dirs when renaming screenshot files ([38e5ff5](38e5ff5))
* **deps:** pin dependency vue to 3.2.37 ([FRSOURCE#68](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/68)) ([d09a762](d09a762))
* **deps:** update dependency @frsource/base64 to v1.0.3 ([FRSOURCE#144](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/144)) ([09ecbd8](09ecbd8))
* **deps:** update dependency move-file to v3 ([FRSOURCE#62](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/62)) ([4f6eaf6](4f6eaf6))
* **deps:** update dependency pixelmatch to v5.3.0 ([FRSOURCE#55](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/55)) ([ca5d278](ca5d278))
* **deps:** update dependency sharp to v0.31.1 ([FRSOURCE#132](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/132)) ([15f0f5d](15f0f5d))
* **deps:** update dependency vue to v3.2.38 ([FRSOURCE#101](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/101)) ([e2d3c82](e2d3c82))
* **deps:** update dependency vue to v3.2.39 ([FRSOURCE#110](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/110)) ([8a7f055](8a7f055))
* **deps:** update dependency vue to v3.2.40 ([FRSOURCE#131](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/131)) ([537fd16](537fd16))
* image diff calculation ([529cb22](529cb22)), closes [FRSOURCE#107](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/107)
* proper readme info ([dd87e19](dd87e19))
* remove alias leftovers from dist bundles ([407ce79](407ce79))
* remove automated screenshots update ([acb3ef0](acb3ef0))
* reset name cache after tests run ([bfbf138](bfbf138))
* sanitize screenshot filenames ([fc57380](fc57380))
* security vulnerabilities ([d0bda44](d0bda44))
* security vulnerability ([d6f849c](d6f849c))
* text overflowing when image is small ([3b04f8e](3b04f8e))

### Features

* add forceDeviceFactor option ([8d69632](8d69632))
* add matchAgainstPath option ([FRSOURCE#146](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/146)) ([7a5e3a8](7a5e3a8)), closes [FRSOURCE#88](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/88)
* add possibility to change images dirname ([b831e94](b831e94))
* add queue flushing in after block ([70f828f](70f828f))
* add title option to matchImage command ([FRSOURCE#81](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/81)) ([4d03866](4d03866))
* add typings ([0a0e8e6](0a0e8e6))
* auto clean unused files ([FRSOURCE#124](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/124)) ([38679a7](38679a7)), closes [FRSOURCE#118](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/118)
* don't override screenshots if not needed ([9066017](9066017))
* externalize important APIs ([9f94086](9f94086))
* first implementation ([388cccf](388cccf))
* img diff when resolution differs ([FRSOURCE#108](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/108)) ([c8a5044](c8a5044)), closes [FRSOURCE#94](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/94)
* introduce imagesPath option ([FRSOURCE#152](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/152)) ([961e137](961e137)), closes [FRSOURCE#147](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/147)
* make library cypress 10 compatible ([b26beb3](b26beb3))
* make plugin Cypress 10 compatible ([a03a17d](a03a17d))
* migrate to @frsource/base64 package ([e4f3a14](e4f3a14))
* provide modern exports ([5c911a1](5c911a1))
* show comparison for successful tests ([FRSOURCE#137](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/137)) ([c09bab3](c09bab3)), closes [FRSOURCE#104](https://github.com/braze-inc/cypress-plugin-visual-regression-diff/issues/104)
* show scrollbar for overflowing images ([de994b9](de994b9))
* stop logging all of the tasks ([573e728](573e728))

### BREAKING CHANGES

* deprecate imagesDir option in favor of imagesPath - see docs for additional information
* To use autocleanup feature you need to update all of the screenshots, best do it by running your test suite with cypress env 'pluginVisualRegressionUpdateImages' set to true.
* matchImage returns object containing comparison details from now on (previously was returning subject element from a chain)
* different resolution doesn't fail test immediately - img diff is being done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants