1081043 - Reduce how often we check for crashes

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

We currently have a decorator [1] that checks for crashes (by default only when an exception is raised). This is attached to _send_message in the client [2]. As seen in bug 1038868 we have an excessive amount of crash checks, which I suspect are caused by Waits, which we expect to raise exceptions a number of times before they complete.

Most exceptions, including these commonly used in waits: NoSuchElementException, StaleElementException, ElementNotVisibleException are not indicative of a crash, so there's not a lot of value in checking for one.

[1] http://hg.mozilla.org/mozilla-central/file/50b689feab5f/testing/marionette/client/marionette/decorators.py#l18
[2] http://hg.mozilla.org/mozilla-central/file/50b689feab5f/testing/marionette/client/marionette/marionette.py#l604

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

One thought would be to limit the check for crashes to exception of type MarionetteException (ignoring inheritance) and IOError. If there are other explicit exceptions that might indicate a crash then we can include those. Malini: Do you think this sounds reasonable?

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Only check for crashes if we get a MarionetteException or an instance of IOError. v1.0

In my testing this has considerably reduced the number of times we check for crashes. For example, running test_kill.py went from 155 checks to just 6.

Comment 3

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8503141 [details] [diff] [review]
Only check for crashes if we get a MarionetteException or an instance of IOError. v1.0

Review of attachment 8503141 [details] [diff] [review]:
-----------------------------------------------------------------

looks great, thanks!

::: testing/marionette/client/marionette/decorators.py
@@ +33,5 @@
>  
>          try:
>              return func(*args, **kwargs)
> +        except Exception as e:
> +            if type(e) is MarionetteException or isinstance(e, IOError):

I think a socket.error may also be thrown

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Only check for crashes if we get a MarionetteException or an instance of IOError. v1.1

Also catch socket.error, and took a slightly different approach.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d49789096063
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ba3b9d7dcd84

Comment 6

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8503970 [details] [diff] [review]
Only check for crashes if we get a MarionetteException or an instance of IOError. v1.1

Review of attachment 8503970 [details] [diff] [review]:
-----------------------------------------------------------------

thanks!

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c52ec780e67

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2c52ec780e67