Closed Bug 753605 Opened 8 years ago Closed 8 years ago

Move emulator code into separate package

Categories

(Testing :: Marionette, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla17

People

(Reporter: jgriffin, Assigned: mihneadb)

Details

Attachments

(1 file, 7 obsolete files)

The emulator handling code which Marionette uses should be moved into a separate package, which could be imported by the marionette test runner.

This would allow it to be used for other purposes, like CI running jlal's gaia framework.

As part of this, wait_for_port should be moved out of emulator and into the testrunner.
Assignee: nobody → mbalaur
We discussed this briefly with Clint.  We're going to make a generic android emulator class, and a B2G specific subclass, and add them to mozdevice.
Attachment #641511 - Flags: review?(jgriffin)
Attachment #641511 - Flags: review?(ahalberstadt)
Comment on attachment 641511 [details] [diff] [review]
new base abstract class Emulator and B2GEmulator extending it

wrong file
Attachment #641511 - Attachment is obsolete: true
Attachment #641511 - Flags: review?(jgriffin)
Attachment #641511 - Flags: review?(ahalberstadt)
Attachment #641513 - Flags: review?(jgriffin)
Attachment #641513 - Flags: review?(ahalberstadt)
Comment on attachment 641513 [details] [diff] [review]
new base abstract class Emulator and B2GEmulator extending it

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

Good progress, I made some suggestions about how the base class could be made further generic.

::: build/mobile/emulator.py
@@ +38,5 @@
> +    deviceRe = re.compile(r"^emulator-(\d+)(\s*)(.*)$")
> +
> +    def __init__(self, homedir=None, noWindow=False, logcat_dir=None, arch="x86",
> +                 emulatorBinary=None, res='480x800', userdata=None,
> +                 memory='512', partition_size='512'):

homedir is relevant only for B2G; it should only be in the B2G subclass.

@@ +42,5 @@
> +                 memory='512', partition_size='512'):
> +        self.port = None
> +        self._emulator_launched = False
> +        self.proc = None
> +        self.marionette_port = None

We should rename self.marionette_port to self.local_port, since this will be used in contexts outside of Marionette.

@@ +48,5 @@
> +        self._tmp_userdata = None
> +        self._adb_started = False
> +        self.logcat_dir = logcat_dir
> +        self.logcat_proc = None
> +        self.arch = arch

We should move arch to the B2G subclass, since it isn't used in the base class.

@@ +96,5 @@
> +    def _check_for_adb(self):
> +        self.adb = os.path.join(self.homedir,
> +                                'glue/gonk/out/host/linux-x86/bin/adb')
> +        if not os.access(self.adb, os.F_OK):
> +            self.adb = os.path.join(self.homedir, 'bin/adb')

These two relative paths (glue/gonk... and bin/adb) are relevant to B2G only, so this part of this function should be in the B2G subclass.

@@ +178,5 @@
> +        if not self._emulator_launched:
> +            return
> +        self.close()
> +        self.start()
> +        return self.setup_port_forwarding(port)

I think we shouldn't call setup_port_forwarding here, since it's mostly a Marionette-specific use case. (This also means we don't need a port parameter to this method.) Instead, when we update Marionette to use this class, we should call setup_port_forwarding from Marionette where we call restart().

@@ +206,5 @@
> +        self.port = int(list(online)[0])
> +
> +
> +    # must be overridden in extending class
> +    def _check_for_os(self):

I think this might better be named _locate_files, since that's what it's really doing.

@@ +207,5 @@
> +
> +
> +    # must be overridden in extending class
> +    def _check_for_os(self):
> +        pass

For methods like this that must be overridden, they should raise NotImplementedError in the base class, or use the @abstractmethod decorator.

@@ +218,5 @@
> +        qemu_args = self.args[:]
> +        if self.copy_userdata:
> +            # Make a copy of the userdata.img for this instance of the emulator
> +            # to use.
> +            self._tmp_userdata = tempfile.mktemp(prefix='marionette')

We should use a generic 'emulator' prefix here.
Attachment #641513 - Flags: review?(jgriffin) → review-
Attached patch modified according to feedback (obsolete) — Splinter Review
Attachment #641513 - Attachment is obsolete: true
Attachment #641513 - Flags: review?(ahalberstadt)
Attachment #642026 - Flags: review?(jgriffin)
Comment on attachment 642026 [details] [diff] [review]
modified according to feedback

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

Looks good!  I think we'll probably want to move B2GEmulator into its own file.

::: build/mobile/emulator.py
@@ +90,5 @@
> +    def _check_for_adb(self):
> +        if not os.access(self.adb, os.F_OK):
> +            adb = subprocess.Popen(['which', 'adb'],
> +                                                   stdout=subprocess.PIPE,
> +                                                   stderr=subprocess.STDOUT)

should line these up with the previous argument
Attachment #642026 - Flags: review?(jgriffin) → review+
Comment on attachment 642026 [details] [diff] [review]
modified according to feedback

Joel, you can provide feedback or not on this as time and interest dictate.
Attachment #642026 - Flags: feedback?(jmaher)
Comment on attachment 642026 [details] [diff] [review]
modified according to feedback

Mark, I understand you may be using the android emulator in some autophone stuff, so this class might help you.
Attachment #642026 - Flags: feedback?(mcote)
Attachment #642026 - Attachment is obsolete: true
Attachment #642026 - Flags: feedback?(mcote)
Attachment #642026 - Flags: feedback?(jmaher)
Attachment #642638 - Flags: review?(jgriffin)
Comment on attachment 642638 [details] [diff] [review]
moved b2gemulator to a separate file; alligned args

Thanks!
Attachment #642638 - Flags: review?(jgriffin)
Attachment #642638 - Flags: review+
Attachment #642638 - Flags: feedback?(jmaher)
Attachment #642638 - Flags: feedback?(mcote)
Comment on attachment 642638 [details] [diff] [review]
moved b2gemulator to a separate file; alligned args

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

overall this looks pretty good.

::: build/mobile/b2gemulator.py
@@ +13,5 @@
> +import socket
> +import subprocess
> +from telnetlib import Telnet
> +import tempfile
> +import time

you don't use multiprocessing, shutil, socket, subprocess, Telnet, tempfile, time in here

@@ +33,5 @@
> +        if self.homedir is not None:
> +            self.homedir = os.path.expanduser(homedir)
> +
> +    def _check_file(self, filePath):
> +        if not os.access(filePath, os.F_OK):

why not os.exists?

::: build/mobile/emulator.py
@@ +7,5 @@
> +from mozprocess import ProcessHandlerMixin
> +import multiprocessing
> +import os
> +import re
> +import platform

I don't believe you use platform in here.

@@ +262,5 @@
> +        """ Set up TCP port forwarding to the specified port on the device,
> +            using any availble local port, and return the local port.
> +        """
> +
> +        import socket

we import this at the top
Attachment #642638 - Flags: feedback?(jmaher) → feedback+
I preserved the imports, I figured they weren't used before as well so I thought it was somewhat of an in-house convention.

Will modify. :)
Attachment #642638 - Attachment is obsolete: true
Attachment #642638 - Flags: feedback?(mcote)
Attachment #643216 - Flags: review?(jmaher)
Comment on attachment 643216 [details] [diff] [review]
removed unused imports, switched to os.path.exists, fixed some whitespace

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

::: build/mobile/b2gemulator.py
@@ +29,5 @@
> +                             'B2G_HOME correctly?') % filePath)
> +
> +    def _check_for_adb(self):
> +        self.adb = os.path.join(self.homedir,
> +                                'glue/gonk/out/host/linux-x86/bin/adb')

my adb doesn't live in a place like this, I assume that is where the adb for the emulator lives?

@@ +30,5 @@
> +
> +    def _check_for_adb(self):
> +        self.adb = os.path.join(self.homedir,
> +                                'glue/gonk/out/host/linux-x86/bin/adb')
> +        if not os.access(self.adb, os.F_OK):

another os.access here, can we replace all instances with os.path.exists?

::: build/mobile/emulator.py
@@ +86,5 @@
> +        else:
> +            return self.port is not None
> +
> +    def _check_for_adb(self):
> +        if not os.access(self.adb, os.F_OK):

os.path.exists?

@@ +234,5 @@
> +        """ Rotate a logfile, by recursively rotating logs further in the sequence,
> +            deleting the last file if necessary.
> +        """
> +        destlog = os.path.join(self.logcat_dir, 'emulator-%d.%d.log' % (self.port, index))
> +        if os.access(destlog, os.F_OK):

os.path.exists?
Attachment #643216 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #15)
> Comment on attachment 643216 [details] [diff] [review]
> removed unused imports, switched to os.path.exists, fixed some whitespace
> 
> Review of attachment 643216 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/mobile/b2gemulator.py
> @@ +29,5 @@
> > +                             'B2G_HOME correctly?') % filePath)
> > +
> > +    def _check_for_adb(self):
> > +        self.adb = os.path.join(self.homedir,
> > +                                'glue/gonk/out/host/linux-x86/bin/adb')
> 
> my adb doesn't live in a place like this, I assume that is where the adb for
> the emulator lives?

I think so, I'm not sure. This was there before and I did not interfere with it. It is only used when doing connect(), which I don't use when I run the tests via Marionette. Maybe jgriffin can tell us more about it.
Attachment #643216 - Attachment is obsolete: true
Attachment #643244 - Flags: review?(jmaher)
Attachment #643244 - Flags: review?(jgriffin)
Comment on attachment 643244 [details] [diff] [review]
switched to os.path.exists all over

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

when I give you an r+ with a comment to change something, feel free to carry over my r+ unless you feel that your changes are significant.
Attachment #643244 - Flags: review?(jmaher) → review+
Sorry, I didn't know. Thanks!
(In reply to Mihnea Dobrescu-Balaur from comment #16)
> (In reply to Joel Maher (:jmaher) from comment #15)
> > Comment on attachment 643216 [details] [diff] [review]
> > removed unused imports, switched to os.path.exists, fixed some whitespace
> > 
> > Review of attachment 643216 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: build/mobile/b2gemulator.py
> > @@ +29,5 @@
> > > +                             'B2G_HOME correctly?') % filePath)
> > > +
> > > +    def _check_for_adb(self):
> > > +        self.adb = os.path.join(self.homedir,
> > > +                                'glue/gonk/out/host/linux-x86/bin/adb')
> > 
> > my adb doesn't live in a place like this, I assume that is where the adb for
> > the emulator lives?
> 
> I think so, I'm not sure. This was there before and I did not interfere with
> it. It is only used when doing connect(), which I don't use when I run the
> tests via Marionette. Maybe jgriffin can tell us more about it.

Yes, this is path of adb relative to the B2G directory.  Note that this only true on linux; we should add code similar to http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/emulator.py#77 so that we select the right relative path on Mac as well.  Is isn't possible to run B2G emulators on windows, so no need to worry about that.
Comment on attachment 643244 [details] [diff] [review]
switched to os.path.exists all over

r+ with the change suggested in comment #20
Attachment #643244 - Flags: review?(jgriffin) → review+
carrying forward r+ from :jmaher :jgriffin
Attachment #643244 - Attachment is obsolete: true
Attachment #643444 - Flags: review+
Thanks Mihnea.  Can you update this patch one last time with the changes from bug 775308, which just landed?  I can then land this one.
Attached patch add -gpu onSplinter Review
carrying forward r+ from :jmaher :jgriffin
Attachment #643444 - Attachment is obsolete: true
Attachment #643656 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/2585b74429d0

Will file a separate github pull request for merging this into mozbase.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.