search

pedrovgs / Shot

Screenshot testing library for Android
Apache License 2.0
1.19k stars 116 forks source link

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

Closed mateuszkwiecinski closed 9 months ago

mateuszkwiecinski commented 3 years ago

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 https://github.com/pedrovgs/Shot/issues/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 #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 commented 2 years ago

How big is the screenshot you are taking?

mateuszkwiecinski commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 9 months ago

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