Closed Bug 1081043 Opened 5 years ago Closed 5 years ago

Reduce how often we check for crashes

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: davehunt, Assigned: davehunt)

References

Details

Attachments

(1 file, 1 obsolete file)

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
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?
Assignee: nobody → dave.hunt
Status: NEW → ASSIGNED
Flags: needinfo?(mdas)
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.
Attachment #8503141 - Flags: feedback?(mdas)
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
Attachment #8503141 - Flags: feedback?(mdas) → feedback+
Flags: needinfo?(mdas)
Also catch socket.error, and took a slightly different approach.
Attachment #8503141 - Attachment is obsolete: true
Attachment #8503970 - Flags: review?(mdas)
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!
Attachment #8503970 - Flags: review?(mdas) → review+
https://hg.mozilla.org/mozilla-central/rev/2c52ec780e67
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.