Marionette should detect when a gecko/emulator instance it has launched has crashed

RESOLVED FIXED in Firefox 18

Status

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jgriffin, Assigned: jgriffin)

Tracking

unspecified
mozilla19
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18 fixed, firefox19 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

7 years ago
Currently, the emulator is subject to a crash while running telephony tests.  When this occurs, it is hard to tell from Marionette's log that this is what happened.

If Marionette has launched a gecko or emulator instance, it should check if this instance is alive after running each test.  If it isn't, it should output an appropriate failure and skip the rest of the tests, rather than emitting an error for each of them like "Connection refused".
Assignee

Comment 1

7 years ago
Posted patch Detect emulator crashes, (obsolete) — Splinter Review
This detects when an emulator has crashed.  It doesn't detect when a Firefox instance has crashed - that will be more work to get right across OS's, and we may have to tweak mozprocess to get it to work.  So even though we want to do that eventually, I'd like to punt on that for now, and just fix the emulator case that we need for TBPL work.
Attachment #673009 - Flags: review?(ahalberstadt)
Comment on attachment 673009 [details] [diff] [review]
Detect emulator crashes,

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

I guess this ok, but shouldn't this logic live in emulator.py? Seems like anything that uses emulators might want to know if it crashed.

r+ anyway because the check_for_process method doesn't really do anything groundbreaking. Please address the nit though.

::: testing/marionette/client/marionette/runtests.py
@@ +331,5 @@
>          testgroup.submit()
>  
> +    def check_for_process(self):
> +        if (self.marionette.emulator and self.marionette.emulator.proc and
> +                not self.marionette.emulator.is_running):

nit: This is confusing, since if the emulator or proc objects are None, check_for_process returns True, which would make the user think that the process does exist. Changing the name of the function is probably good enough.
Attachment #673009 - Flags: review?(ahalberstadt) → review+
Assignee

Comment 3

7 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #2)
> Comment on attachment 673009 [details] [diff] [review]
> Detect emulator crashes,
> 
> Review of attachment 673009 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess this ok, but shouldn't this logic live in emulator.py? Seems like
> anything that uses emulators might want to know if it crashed.
> 
> r+ anyway because the check_for_process method doesn't really do anything
> groundbreaking. Please address the nit though.
> 
> ::: testing/marionette/client/marionette/runtests.py
> @@ +331,5 @@
> >          testgroup.submit()
> >  
> > +    def check_for_process(self):
> > +        if (self.marionette.emulator and self.marionette.emulator.proc and
> > +                not self.marionette.emulator.is_running):
> 
> nit: This is confusing, since if the emulator or proc objects are None,
> check_for_process returns True, which would make the user think that the
> process does exist. Changing the name of the function is probably good
> enough.

What would you suggest instead?  The tricky part is that you can run tests on an emulator you've launched yourself, in which case Marionette cannot determine if the emulator is alive or not (since it doesn't know its pid).  In that case, emulator.proc is None.

I guess we could reverse things to do something like:

def check_if_crashed(self):
    # logic

which would always return False in the case of a manually-launched emulator, i.e., when emulator.proc is None.  Does that make more sense?
Assignee

Comment 4

7 years ago
Posted patch Detect emulator crashes, (obsolete) — Splinter Review
Ok, I think this is a cleaner solution
Attachment #674027 - Flags: review?(ahalberstadt)
Assignee

Updated

7 years ago
Attachment #673009 - Attachment is obsolete: true
Comment on attachment 674027 [details] [diff] [review]
Detect emulator crashes,

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

r+ on the condition that this gets added to the emulator.py in mozdevice as well since marionette is supposed to be using that in the near future.
Attachment #674027 - Flags: review?(ahalberstadt) → review+
Assignee

Updated

7 years ago
Attachment #674027 - Attachment is obsolete: true
Assignee

Comment 7

7 years ago
Comment on attachment 674430 [details] [diff] [review]
Detect emulator crashes,

Carrying r+ forward
Attachment #674430 - Flags: review+
Assignee

Comment 8

7 years ago
Try run was green, landed as https://hg.mozilla.org/mozilla-central/rev/b1e54bf5bd6f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Assignee

Comment 9

7 years ago
Landed on ash as aurora-try: https://tbpl.mozilla.org/?tree=Ash&rev=0f8d21413c86
Whiteboard: [automation-needed-in-aurora]
Assignee

Comment 10

7 years ago
Posted patch patch for ashSplinter Review
A modified patch that I'm landing on ash, since the original didn't apply cleanly:  https://hg.mozilla.org/projects/ash/rev/6d4dff1f9b3e
Depends on: 813305
You need to log in before you can comment on or make changes to this bug.