calabash / calabash-ios-server

An embedded HTTP server for performing queries and test automation
Other
87 stars 82 forks source link

LPScreenshot: handle exceptions when calling snapshot API #351

Closed jmoody closed 8 years ago

jmoody commented 8 years ago

Motivation

Some users are reporting crashes using the Snapshot API for screenshots.

We don't want to stop using the Snapshot API because it supports UIKit and OpenGL views.

nfrydenholm commented 8 years ago

Nice fix 👍 :shipit:

sapieneptus commented 8 years ago

@jmoody Are there integration tests for this?

jmoody commented 8 years ago

@sapieneptus

No. I can't reproduce the crash in any of the test apps. The cucumber tests do take screenshots.

Do you have a suggestion about how to induce a crash?

jmoody commented 8 years ago

We might be able to induce a crash in the snapshotting method by adding a backdoor that swizzles the snapshotting method to raise an exception.

Let me know if you want me to add this test.

sapieneptus commented 8 years ago

@jmoody I actually meant are there tests that show both screen shot methods actually work? I don't know how to induce the crash if following the steps users describe doesn't do so, so personally as long as this doesn't break anything and we have any evidence that this will resolve the issue for users (have you contacted the affected users?) I'd be fine merging

jmoody commented 8 years ago

tests that show both screen shots actually work

# Appeared in 0.18.2; replaced the previous implementation.
# Captures OpenGL and UIView elements
* - (NSData *) takeScreenshotUsingSnapshotAPI;

# Previous implementation since 0.9.x days
* - (NSData *) takeScreenshotUsingRenderInContext;

how to induce the crash if following the steps users describe doesn't do so

There are no steps to reproduce; the crash is app specific. One of our long-time users (name withheld) reported that screenshots were crashing the app in 0.18.2 for some view hierarchies.

In a sense this a patch for backward compatibility.

(have you contacted the affected users?

No. I will do that now.

sapieneptus commented 8 years ago

We have given the latest server to the client with the issue. As we have not yet heard back and have not identified any regressions, we will tentatively merge.