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

bug: getMismatchedPixels timeout when using clip options in compareScreenshot #4866

Closed
3 tasks done
Spirielle opened this issue Sep 29, 2023 · 16 comments · Fixed by #5167
Closed
3 tasks done

bug: getMismatchedPixels timeout when using clip options in compareScreenshot #4866

Spirielle opened this issue Sep 29, 2023 · 16 comments · Fixed by #5167
Assignees
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil

Comments

@Spirielle
Copy link

Prerequisites

Stencil Version

3.4.0

Current Behavior

We have e2e tests with screenshots failing with the error "Error: Failed: "getMismatchedPixels timeout: 15000ms"
I have noticed that it happens only when

  1. There's a visual difference between the 2 screenshots
    AND
  2. The test uses the "clip" option. For example like this:
    await page.compareScreenshot('', { clip: { x: 0, y: 0, width: 800, height: 800 } });

Expected Behavior

We expect e2e with screenshots using clip options to be comparable.

System Info

No response

Steps to Reproduce

  • Use clip option in a e2e with compare screenshot
    await page.compareScreenshot('', { clip: { x: 0, y: 0, width: 800, height: 800 } });
  • Run test a first time to have an initial screenshot
  • Change e2e slightly to trigger a visual change
  • Run test a second time -> error occurs

Code Reproduction URL

https://github.com/Spirielle/bug-stencil-getMismatchedPixels

Additional Information

Investigation done so far

About # 1, I believe it only happens when there's a difference because when the images are identical, the code which compares pixels is not reached. See here

About # 2, I think the width and height given to pixel-match does not take the clip options into consideration.

We use the clip options when calling the function CompareScreenshots here

However, it seems we don't use the clip size when calling pixelmatch here

And pixel-match fails if the screenshots size is different than the provided width and height here line 24

Now, I'm not sure why we get a timeout error rather than the actual error.

@ionitron-bot ionitron-bot bot added the triage label Sep 29, 2023
@rwaskiewicz rwaskiewicz self-assigned this Oct 4, 2023
@rwaskiewicz rwaskiewicz added the Bug: Validated This PR or Issue is verified to be a bug within Stencil label Oct 4, 2023
@rwaskiewicz rwaskiewicz removed their assignment Oct 4, 2023
@ionitron-bot ionitron-bot bot removed the triage label Oct 4, 2023
@rwaskiewicz
Copy link
Member

Thanks! I've confirmed this is a bug and have labeled it to get it ingested into our backlog

@trazek
Copy link

trazek commented Dec 12, 2023

Thanks @rwaskiewicz! We encountered this as well at our company. Any information/progress on this would be awesome. We've updated to the latest Stencil and cannot use a timeout property anymore, so are dead in the water

@rwaskiewicz
Copy link
Member

Hey folks,

I don't have any additional information at this time. Please do me a favor and add 👍's to the issue summary to upvote it - this is the easiest way for us gauge interest/need for a fix and helps when we prioritize issues.

Thanks!

@christian-bromann christian-bromann self-assigned this Dec 13, 2023
@christian-bromann
Copy link
Member

@trazek I am happy to take a look!

@christian-bromann
Copy link
Member

@trazek @Spirielle I raised a PR to ensure Stencil doesn't make the test stale when comparing the images. It is true that the test fails due to an image size mismatch. Even though the screenshots are the same (600x800px with 72dpi) the size is differs by 150% which causes pixelmatch to fail. I will run some investigations on why that is and will get back to you soon.

@rwaskiewicz
Copy link
Member

The fix for this issue has been released as a part of today's Stencil 4.9.0 release.

@Spirielle
Copy link
Author

Hi, thanks for working on this 😊.

I tested the new build. we indeed do not get the getMismatchPixels timeout anymore. 👍
We now receive the underlying error: "Error: Image data size does not match width/height."

However, part of the bug is that I don't think we should receive this error :S . The 2 screenshots use the default size of 800 x 600. When I check the png in the screenshot folder, they are of the same size.

Same thing happens when we use the "clip" option in compareScreenshot; the png in the screenshot folder are of the correct size, but we still get the "Error: Image data size does not match width/height." error.

"Even though the screenshots are the same (600x800px with 72dpi) the size is differs by 150% "
@christian-bromann Did you figure out why?

@christian-bromann
Copy link
Member

However, part of the bug is that I don't think we should receive this error

I think it shows the flaws of pixel based comparison techniques used by compareScreenshot which uses pixelmatch under the hood. I still don't know why a screenshot made on your CI machine is different than one made by your Mac even though size and dpi is the same. What I can imagine is that Chromium has different ways of rendering a screenshot on e.g. Linux vs MacOS causing this difference.

That said, my recommendation would be to ensure tests are all run on the same (CI) machine to ensure these problems don't happen. If you require running visual comparison tests on different machines, a different screenshot comparison tool might be worth taking a look at. The Stencil team is looking to revisit the frameworks testing story in Q1 next year and the ability to support that is great feedback.

Let me know if you have further questions.

@Spirielle
Copy link
Author

Sorry, I'm a bit confused by your reply. I tested locally, not on a CI machine, and I'm on Windows, not MacOS.

I updated the repo I linked initially with clear repro steps.

Should I open a different bug?

@christian-bromann
Copy link
Member

How should the system behave in this scenario? There was a visual modification that resulted in a test failure. It is correct that the error message is misleading here due to the nature of pixel by pixel comparison used by pixelmatch.

@Spirielle
Copy link
Author

I believe the test should pass rather than throw an unhandled exception. We can then check the result returned by the compareScreenshot function, and its mismatchedPixels field. That's how it used to behave.
However right now, because of the error, we do not even get the results. It does not show either in the compare.html file.

If we clip the screenshot to 600 x 600, it behaves correctly: The test pass, we get the result of compareScreenshot and the 2 images can be compared in compare.html.
I added this example of happy path in the repo I linked above.

I also found the version in which it started to fail. It seems the version of pixelmatch packaged in stencil has changed between v2.18.0 and v2.19.0.
If I change my stencil version to v2.18.0, I do not have any error anymore.

@christian-bromann
Copy link
Member

I did some more debugging and found a weird behavior between the screenshot connector and pixel match. I am fairly new to the code base so I am getting more familiar with every Stencil component every day.

Could you do me a favor and create a new issue for this? Thank you!

@Spirielle
Copy link
Author

Of course! :)
I'll open an new issue later today.
Thanks

@christian-bromann
Copy link
Member

@Spirielle if you get the chance, mind testing a dev build of Stencil that I created with a possible fix? To install the dev version via:

yarn add @stencil/core@4.9.0-dev.1703209691.c710375

Please let me know if this resolves the issue your experience. Thank you!

@Spirielle
Copy link
Author

Hey :)
Sorry for the delay. Just tested the dev build and yes, it indeed fixes the issue when using the clip option! 😊

While I was testing, I also tried without any clip option too, but that path still fails with the "Image data size does not match width/height." error 🤔. It seems the screenshot saved is 800 x 600, but pixelmatch expects a width and height of 600 x 600 ?

Still I'm glad the clip options works, thanks.
I completely forgot to open the new issue. I'll do it right away, sorry about that.

@christian-bromann
Copy link
Member

Thanks for verifying. Let's continue our conversation there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants