Closed Bug 803254 Opened 8 years ago Closed 8 years ago

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

Categories

(Testing :: Marionette, defect)

defect
Not set
normal

Tracking

(firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
mozilla19
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: jgriffin, Assigned: jgriffin)

References

Details

Attachments

(2 files, 2 obsolete files)

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".
Attached 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+
(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?
Attached patch Detect emulator crashes, (obsolete) — Splinter Review
Ok, I think this is a cleaner solution
Attachment #674027 - Flags: review?(ahalberstadt)
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+
Attachment #674027 - Attachment is obsolete: true
Comment on attachment 674430 [details] [diff] [review]
Detect emulator crashes,

Carrying r+ forward
Attachment #674430 - Flags: review+
Try run was green, landed as https://hg.mozilla.org/mozilla-central/rev/b1e54bf5bd6f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Landed on ash as aurora-try: https://tbpl.mozilla.org/?tree=Ash&rev=0f8d21413c86
Whiteboard: [automation-needed-in-aurora]
Attached 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.