1278590 - [mozrunner] Create a FennecRunner

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Create a FennecEmulatorRunner that launches an Android emulator (or connects to an existing emulator) and runs Fennec on that emulator. It should provide behaviour expected in existing Android automation, in particular starting an emulator with an AVD.

(This is motivated by Bug 787203, where I want Marionette's Python harness to be able to launch Fennec. In automation, I expect that the existing android_emulator_unittest.py mozharness script will take care of launching an appropriate emulator (this step does not use mozrunner), and then then Marionette runner will use mozrunner to connect to that emulator and start-up Fennec.)

Comment 1

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Bug 1278590 - Create a FennecRunner;

Add FennecEmulatorRunner (for convenience), FennecRunner, FennecContext and EmulatorAVD.

Common behaviour is defined in BaseEmulator and RemoteContext, to distinguish
from B2G and Fennec specifics. I've tried to decouple ArchContext from B2GContext, as well.

The emulator/adb commands FennecRunner and EmulatorAVD runner are intended to match the
behaviour seen in current Android automation (e.g. mochitest).

Review commit: https://reviewboard.mozilla.org/r/58258/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58258/

Comment 2

[William Lachance (:wlach)]

Comment on attachment 8760806 [details]
Bug 1278590 - Create a FennecRunner;

I'm not a great person to review this, it's been quite some time since I've been working in this area.

Comment 3

[Andrew Halberstadt [:ahal]]

Comment on attachment 8760806 [details]
Bug 1278590 - Create a FennecRunner;

https://reviewboard.mozilla.org/r/58258/#review55152

This is really awesome, thanks for taking the time to separate everything into separate classes! This looks good, just a couple nits and questions to answer. That being said, I'm not familiar at all with how the android emulator works, so I didn't really review those aspects for correctness, I mostly reviewed for overall architecture. I'll let you decide whether you want to go ahead and land or wait for gbrown to get back depending on how confident you are with this patch.

::: testing/mozbase/mozrunner/mozrunner/devices/emulator.py:48
(Diff revision 1)
>  
>          if binary:
>              self.binary = binary
>  
> +        if extra_args:
> +            self.extra_args = extra_args

Did you mean to overwrite self.extra_args, or should this be extended?

::: testing/mozbase/mozrunner/mozrunner/devices/emulator.py
(Diff revision 1)
>          if self.proc:
>              self.proc.kill()
>              self.proc = None
>  
>          # Remove temporary files
> -        self.userdata.close()

Why did this get removed?

::: testing/mozbase/mozrunner/mozrunner/runners.py:96
(Diff revision 1)
> +                          adb_path=None,
> +                          avd_home=None,
> +                          logdir=None,
> +                          serial=None,
> +                          binary=None,
> +                          app='org.mozilla.fennec',
> +                          **kwargs):

nit: extra space in front of these

::: testing/mozbase/mozrunner/mozrunner/runners.py:107
(Diff revision 1)
> +    :param avd: an AVD from among those used in Mozilla automation.
> +        Defaults to 'mozemulator-4.3'

Is there a link to the possible options we could include here?

::: testing/mozbase/mozrunner/mozrunner/runners.py:113
(Diff revision 1)
> +    :param binary: emulator binary to run
> +    :param app: name of Fennec app (often org.mozilla.fennec_$USER)

nit: Capitilize first word for consistency

Comment 4

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/58258/#review55152

> Did you mean to overwrite self.extra_args, or should this be extended?

I was ambivalent about this, but it's probably more useful to extend.

> Why did this get removed?

Because I forgot to move it to a clean_up method in Emulator. :)

> Is there a link to the possible options we could include here?

Judging by `android_device.py`, the only other option might be `mozemulator-x86`, but I don't actually know. Let's ask gbrown.

Comment 5

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8760806 [details]
Bug 1278590 - Create a FennecRunner;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58258/diff/1-2/

Comment 6

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

gbrown, there's an extra question for you in the mozreview comments.

(In reply to Maja Frydrychowicz (:maja_zf) from comment #4)
> https://reviewboard.mozilla.org/r/58258/#review55152
> > Is there a link to the possible options we could include here?
> 
> Judging by `android_device.py`, the only other option might be
> `mozemulator-x86`, but I don't actually know. Let's ask gbrown.

Comment 7

[Geoff Brown [:gbrown]]

Currently in automation, we run an arm-based Android 4.3 emulator for most tests and a very-limited set of tests on an x86-based Android 4.2 emulator. 

'mach android-emulator' provides parallel options which use the same AVD definitions used in automation: 'mach android-emulator' or 'mach android-emulator --version 4.3' uses the arm Android 4.3 AVD; 'mach android-emulator --version x86' uses the x86 Android 4.2 AVD.

AVD definitions are shared between mach and automation via tooltool downloads and are named mozemulator-4.3 and mozemulator-x86, as you have used.

We could -- and likely will eventually -- add automation/mach support for more AVDs/Android flavors in future, but for now, that's all there is.

Developers can of course create their own AVDs and name them whatever they like. For mach, my thinking has been that if someone can create their own AVD, they can figure out how to run it without mach -- no need to add more options and code to support advanced uses like that.

I think you have it right.

Comment 8

[Geoff Brown [:gbrown]]

Comment on attachment 8760806 [details]
Bug 1278590 - Create a FennecRunner;

https://reviewboard.mozilla.org/r/58258/#review56166

Thanks for your patience!

I have not tested this, but it all looks right to me.

::: testing/mozbase/mozrunner/mozrunner/runners.py:100
(Diff revision 2)
> +def FennecEmulatorRunner(avd='mozemulator-4.3',
> +                         adb_path=None,
> +                         avd_home=None,
> +                         logdir=None,
> +                         serial=None,
> +                         binary=None,

Would it be possible and useful to provide a default for binary, binary='emulator'?

Comment 9

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/58258/#review56166

> Would it be possible and useful to provide a default for binary, binary='emulator'?

For consistency, I'm going to keep the default `binary=None`, which in turn triggers the default behaviour in ArchContext (which tries to figure out where the emulator binary is by checking the PATH and falls back to 'emulator' otherwise). The fix-up patch I'm about to push tries to make that clearer.

Comment 10

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8760806 [details]
Bug 1278590 - Create a FennecRunner;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58258/diff/2-3/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49bc0b96cde2
Create a FennecRunner; r=ahal,gbrown

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/49bc0b96cde2