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

Recording too large screenshots produces false-positive results. Can't configure max screenshot size #264

Closed
mateuszkwiecinski opened this issue Nov 14, 2021 · 7 comments

Comments

@mateuszkwiecinski
Copy link
Contributor

mateuszkwiecinski commented Nov 14, 2021

Expected behaviour

It seems like I stumbled upon 2 issues at once, but the behavior I expect is:

  1. I expect Shot would fail the build if it can't record a screenshot for any reason. (this is actually Failing to record screenshot should fail test with a exception instead of posting a silent error report #253)
  2. I expect config variable responsible for setting maximum screenshot size to be configurable in Shot as well.

Actual behaviour

  1. recording task passes the build successfully
  2. no screenshot gets added/changed/removed,
  3. generated report is empty
  4. there is no indication something went wrong
  5. verification task also passes successfully

If someone doesn't expect changed screenshot, or trusts the verification task such behavior can easily sneak in an unwanted behavior change.

Steps to reproduce

  1. Have a large view, that spans across >10000000 pixels. (This is easily achievable when capturing wrap_content screenshots Allow null heights to make the screenshot to wrap content #110)
  2. Run ./gradlew executeScreenshotTests -Precord
  3. wait for successful build
    image
  4. Run ./gradlew executeScreenshotTests
  5. wait for successful build with additional confirmation the tests are passing
    image

The workaround for this particual issue is to use facebook library directly to capture the view.

Version of the library

5.11.2

@pedrovgs
Copy link
Owner

How big is the screenshot you are taking?

@mateuszkwiecinski
Copy link
Contributor Author

I don't know how big the screenshot I take is, but I can share it fails here: https://github.com/facebook/screenshot-tests-for-android/blob/cdb5bd36b9be745bb3ba3378bf102815d2165144/core/src/main/java/com/facebook/testing/screenshot/internal/ScreenshotImpl.java#L206

so the plugin starts misbehaving when captured screenshot has more than:
public static final long DEFAULT_MAX_PIXELS = 10000000L;
so 2000x5001px would fall into the scenario I described

@pedrovgs
Copy link
Owner

pedrovgs commented Dec 22, 2021

Ok so then the issue is not on our side 😢 I'm afraid there is nothing we can do to fix this issue. I'm closing it for now. Thanks for reporting @mateuszkwiecinski

@mateuszkwiecinski
Copy link
Contributor Author

mateuszkwiecinski commented Dec 22, 2021

I think it is 100% on the Shot's side 🤔 I mean facebook's library allows to configure the maximum screenshot size (and not to rely on the default value I linked). I claim Shot plugin should also expose such ability.

See: https://github.com/facebook/screenshot-tests-for-android/blob/cdb5bd36b9be745bb3ba3378bf102815d2165144/core/src/main/java/com/facebook/testing/screenshot/internal/RecordBuilderImpl.java#L123 for reference

@mateuszkwiecinski
Copy link
Contributor Author

Also, the behavior where plugin fails silently which leads to false positives is also something needs fixing. That's a clear bug to me which makes the plugin unreliable 👀 Failing screenshot should always fail the build, no?

@pedrovgs
Copy link
Owner

Ok, I get what you mean now. I'm going to open the issue again and update the description to specify that the change we should implement is to let configure the max screenshot size instead of relaying on the default value. Thanks for the clarification btw. The catch preventing Shot to crash when this happens is a different bug I think you've already reported. Thanks for the info, I really appreciate it.

@pedrovgs
Copy link
Owner

pedrovgs commented Feb 5, 2024

@nrotonda fixed the issue and I'll release the fix with Shot 6.1.0

@pedrovgs pedrovgs closed this as completed Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants