789305 - Add a b2g manager class for shared code when interacting with b2g devices

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: B2GManager class v1.0

We should create nice manager class for a b2g instance used in automation, so we can easily do something like "restart b2g" or "setup profile". Any b2g automation framework is likely to use these functions, and it'd be nice to have it all in one, easily imported place.

I've added a b2g.py file that defines the B2GManager class, which holds a bunch of useful functions.

Comment 1

[William Lachance (:wlach)]

Comment on attachment 659020 [details] [diff] [review]
B2GManager class v1.0

I know I wasn't marked as the reviewer for this, but I have some comments nonetheless:

(1) There are assumptions that we have a direct adb connection to the device which will not be valid when we switch over to using SUT. I'd prefer if we used SUT method (shell, etc.) here.
(2) I don't really like "manager" classes in general for this sort of thing. Basically the functionality in this class can be seen as B2G-specific extensions to DeviceManagerADB, as such I think inheritance is the right approach here. This would also follow the example of the DroidSUT and DroidADB classes I created for adding a few android-specific methods.
(3) This adds implicit dependencies on marionette and mozprofile, which is probably a bit much for mozdevice. I think we might want to consider creating a mozb2g module to include this (and maybe B2GEmulator, while we're at it).

Comment 2

[Jonathan Griffin (:jgriffin)]

Comment on attachment 659020 [details] [diff] [review]
B2GManager class v1.0

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

Cool, this should certainly help reduce duplication of B2G-related code in our test runners!

I'm all for making a separate mozb2g component of mozbase; I agree we don't want to add a Marionette dependency to mozdevice, and it will give us more flexibility to add other deps as needed, which might not be viable with all the environments that mozdevice is used in.

Making a separate package though means having to deal with mozdevice's strange mirroring policy (its canonical location for most files is in m-c/build/mobile, which is mirrored into the full package on github in mozbase/mozdevice).  I _think_ we'd want to adopt the same strategy if we want mozb2g to be mirrored between github and m-c, which is a little gross.  The other option is to mirror it like other mozbase components (canonical location in github, mirrored to m-c/testing/mozbase), which would mean it couldn't be used in-situ from the tree (because it couldn't find devicemanager classes), but none of the B2G automation works that way anyway - it pulls packages from pypi instead.  Will, what's your opinion?

Will's idea of making separate SUT and ADB extensions of devicemanager seems like a good idea. I think the only thing here that's completely ADB-specific is the port forwarding, which we won't do at all when using SUT.

r- only for those, but thanks for doing this, I look forward to being able to use this and not re-writing similar code in every harness that needs it.

::: mozdevice/mozdevice/b2g.py
@@ +2,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +# You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +import datetime
> +import mozprofile

This import doesn't seem to be used anywhere.

@@ +44,5 @@
> +            time.sleep(1)
> +        return False
> +
> +    def get_marionette(self):
> +        self.marionette = Marionette(self.marionette_host, self.marionette_port)

Usually methods that start with "get" return the thing they're getting.  Maybe call this "connect_marionette" instead?  I also think this method could call forward_port() and wait_for_port() automatically; I don't think there would be any negative side-effects of doing this - it doesn't hurt to forward a port that's already been forwarded.

Comment 3

[William Lachance (:wlach)]

(In reply to Jonathan Griffin (:jgriffin) from comment #2)
> Comment on attachment 659020 [details] [diff] [review]
> B2GManager class v1.0
> 
> Review of attachment 659020 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Cool, this should certainly help reduce duplication of B2G-related code in
> our test runners!
> 
> I'm all for making a separate mozb2g component of mozbase; I agree we don't
> want to add a Marionette dependency to mozdevice, and it will give us more
> flexibility to add other deps as needed, which might not be viable with all
> the environments that mozdevice is used in.
> 
> Making a separate package though means having to deal with mozdevice's
> strange mirroring policy (its canonical location for most files is in
> m-c/build/mobile, which is mirrored into the full package on github in
> mozbase/mozdevice).  I _think_ we'd want to adopt the same strategy if we
> want mozb2g to be mirrored between github and m-c, which is a little gross. 
> The other option is to mirror it like other mozbase components (canonical
> location in github, mirrored to m-c/testing/mozbase), which would mean it
> couldn't be used in-situ from the tree (because it couldn't find
> devicemanager classes), but none of the B2G automation works that way anyway
> - it pulls packages from pypi instead.  Will, what's your opinion?

I think using the same mirroring approach we use with mozdevice makes a lot of sense. Lately I've taken to landing patches in both places at the same time (using a sed script to modify the paths of the patches), which seems to work great (we get to use the changes from both places, and we preserve history). 

The problem of finding the devicemanager classes can be fixed with bug 723107, which is something I'd been meaning to do for a while anyway. Thanks for giving me the extra motivation to finally take care of it. ;)

Comment 4

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to William Lachance (:wlach) from comment #1)
> Comment on attachment 659020 [details] [diff] [review]
> B2GManager class v1.0
> 
> I know I wasn't marked as the reviewer for this, but I have some comments
> nonetheless:
> 
> (1) There are assumptions that we have a direct adb connection to the device
> which will not be valid when we switch over to using SUT. I'd prefer if we
> used SUT method (shell, etc.) here.
> (2) I don't really like "manager" classes in general for this sort of thing.
> Basically the functionality in this class can be seen as B2G-specific
> extensions to DeviceManagerADB, as such I think inheritance is the right
> approach here. This would also follow the example of the DroidSUT and
> DroidADB classes I created for adding a few android-specific methods.
> (3) This adds implicit dependencies on marionette and mozprofile, which is
> probably a bit much for mozdevice. I think we might want to consider
> creating a mozb2g module to include this (and maybe B2GEmulator, while we're
> at it).

I agree with all points, and will revise the patch accordingly

Comment 5

[Andrew Halberstadt [:ahal]]

(In reply to William Lachance (:wlach) from comment #1)
> (3) This adds implicit dependencies on marionette and mozprofile, which is
> probably a bit much for mozdevice. I think we might want to consider
> creating a mozb2g module to include this (and maybe B2GEmulator, while we're
> at it).

I'm not sure I agree with this. If we have a top level mozb2g module, then why not mozfirefox, mozfennec and mozthunderbird? In my mind B2G is a device and should be a subclass of something in devicemanager.

Though I realize there are tradeoffs either way and that I am just bikeshedding, so carry on! :)

Comment 6

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: Add mozb2g dir and b2gmixin v1.0

So this adds the mozb2g package to mozbase, with a B2GMixin class for use with devicemanager.

I've tested the mixin for devicemanagerADB and devicemanagerSUT, and the commands work well.

Comment 7

[William Lachance (:wlach)]

(In reply to Andrew Halberstadt [:ahal] from comment #5)
> (In reply to William Lachance (:wlach) from comment #1)
> > (3) This adds implicit dependencies on marionette and mozprofile, which is
> > probably a bit much for mozdevice. I think we might want to consider
> > creating a mozb2g module to include this (and maybe B2GEmulator, while we're
> > at it).
> 
> I'm not sure I agree with this. If we have a top level mozb2g module, then
> why not mozfirefox, mozfennec and mozthunderbird? In my mind B2G is a device
> and should be a subclass of something in devicemanager.

Yes, I agree with this. The main justification for a new module is that we really don't want mozdevice to accumulate too many dependencies at present, as it makes integrating it with talos more difficult (because of the way we integrate it with mozbase in production).

Creating a new module is more a practical call. If it makes you feel better, think of mozb2g as being an extension of mozdevice. :)

Comment 8

[William Lachance (:wlach)]

Comment on attachment 665480 [details] [diff] [review]
Add mozb2g dir and b2gmixin v1.0

f- just because I think there are some areas where the patch could be improved and I'd like to see a bit of iteration (or at least discussion). On the whole this is very good.

A few random high-level thoughts:

* You use the foo_bar style for method naming, which I normally prefer, but we inherit from a class with the fooBar style. I think it's best to be consistent with the parent.
* At some point it would be nice to have unit tests for some of these methods, just as a sanity check. See the example unit tests in mozdevice for info on how to mock an agent, etc.

>diff --git a/mozb2g/mozb2g/__init__.py b/mozb2g/mozb2g/__init__.py
>new file mode 100644
>index 0000000..833dd04
>--- /dev/null
>+++ b/mozb2g/mozb2g/__init__.py
>@@ -0,0 +1,6 @@

>+    def __init__(self):
>+        self.profile_dir = tempfile.mkdtemp()
>+        self.user_js = "/data/local/user.js"
>+        self.marionette_host = None
>+        self.marionette_port = 2828
>+        self.marionette = None

For declarations like this, I usually put them at the top of the class. E.g.:

class Foo(object):
   bar = 1

   def __init__(self):

Also, you should make sure to clean up the temporary directory for the profile when we destroy this class (though see my comments below on mozprofile).

>+    def set_user_js(self, path):
>+        """
>+        Set the path to the user.js file on the device
>+        """
>+        self.user_js= path

Is there any value to making this a method as opposed to setting it directly?

>+    def set_marionette_info(self, host, port):
>+        """
>+        Set the host/port of the marionette server
>+        """
>+        self.marionette_host = host
>+        self.marionette_port = port
>+        self.marionette = None

Wouldn't it make sense to just set this in the constructor?

>+    def wait_for_port(self, timeout):
>+        """
>+        Wait for the marionette server to respond.
>+        Timeout parameter is in seconds
>+        """
>+        print "waiting for port"
>+        starttime = datetime.datetime.now()
>+        while datetime.datetime.now() - starttime < datetime.timedelta(seconds=timeout):
>+            try:
>+                sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
>+                print "trying %s %s" % (self.marionette_port, self.marionette_host)
>+                sock.connect((self.marionette_host, self.marionette_port))
>+                data = sock.recv(16)
>+                sock.close()
>+                if '"from"' in data:
>+                    return True
>+            except:

Blanket "except" is not good, as it can mask things like syntax errors. Just catch the specific exceptions that you care about (I guess socket errors in this case)

>+                import traceback
>+                print traceback.format_exc()

I don't think we want to include this in production code

>+            time.sleep(1)
>+        return False

I wonder if throwing an exception (a DMError) in the case we can't connect might not be better than a return value? Something to think about.

>+    def restart_b2g(self):
>+        """
>+        Restarts the b2g process on the device
>+        """
>+        #restart b2g so we start with a clean slate
>+        self.shellCheckOutput(['stop', 'b2g'])
>+        # Wait for a bit to make sure B2G has completely shut down.
>+        time.sleep(10)

I think it would be better (and faster) to check if the B2G process has stopped on some interval.

>+    def setup_profile(self, prefs=None):
>+        """
>+        Sets up the user profile on the device,
>+        The 'prefs' is a string of user_prefs to add to the profile.
>+        If it is not set, it will default to a standard b2g testing profile.
>+        """
>+        if not prefs:
>+            prefs = """
>+user_pref("power.screen.timeout", 999999);
>+user_pref("devtools.debugger.force-local", false);
>+            """
>+        #remove previous user.js if there is one
>+        if not self.profile_dir:
>+            self.profile_dir = tempfile.mkdtemp()
>+        our_user_js = os.path.join(self.profile_dir, "user.js")
>+        if os.path.exists(our_user_js):
>+            os.remove(our_user_js)
>+        #copy profile
>+        try:
>+            output = self.pullFile(self.user_js)
>+            backup_pref = open(our_user_js, 'w')
>+            backup_pref.write(output)
>+            backup_pref.close()
>+        except subprocess.CalledProcessError:
>+            pass
>+        #if we successfully copied the profile, make a backup of the file
>+        if os.path.exists(our_user_js):
>+            self.shellCheckOutput(['dd', 'if=%s' % self.user_js, 'of=%s.orig' % self.user_js])
>+        print "opening userjs"
>+        user_js = open(our_user_js, 'a')
>+        print "Writing: %s" % prefs
>+        user_js.write("%s" % prefs)
>+        print "closing"
>+        user_js.close()
>+        self.pushFile(our_user_js, self.user_js)
>+        self.restart_b2g()

A few things here:

(1) What is backup_pref and how is it used?
(2) Could we streamline this stuff to use an instance of mozprofile instead?
(3) It would be good to use "with" for writing the backup_pref. e.g.:

with open(our_user_js, 'w') as backup_pref:
    backup_pref.write(output)

(for python 2.5 compatibility which we probably need for now, import the with_statement from future)

(3) The debugging logs seem a little verbose to me (e.g. "closing")

>+    def get_ip(self, conn_type='eth0'):
>+        """
>+        Gets the IP of the device, or None if no connection exists.
>+        """
>+        match = re.match(r"eth0: ip (\S+)", self.shellCheckOutput(['ifconfig', conn_type]))
>+        if match:
>+            return match.group(1)
>+        output.close()

I wonder if this wouldn't actually go better in mozdevice. We could certainly use such a method on Android.

Comment 9

[Jeff Hammel]

> 
> >+    def __init__(self):
> >+        self.profile_dir = tempfile.mkdtemp()
> >+        self.user_js = "/data/local/user.js"
> >+        self.marionette_host = None
> >+        self.marionette_port = 2828
> >+        self.marionette = None
> 
> For declarations like this, I usually put them at the top of the class. E.g.:
> 
> class Foo(object):
>    bar = 1
> 
>    def __init__(self):

As a matter of some practicality, putting them at the class level allows them to be (albeit, very globally) modified, whereas putting them in __init__ basically does not.  So I'm going to have to side with :wlach here

Comment 10

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: Add mozb2g dir and b2gmixin v2.0

(In reply to William Lachance (:wlach) from comment #8)

I've updated the patch according to your feedback, and responded inline to some comments:

> >+            time.sleep(1)
> >+        return False
> 
> I wonder if throwing an exception (a DMError) in the case we can't connect
> might not be better than a return value? Something to think about.
> 
Yeah that makes more sense to throw an exception

> >+    def restart_b2g(self):
> >+        """
> >+        Restarts the b2g process on the device
> >+        """
> >+        #restart b2g so we start with a clean slate
> >+        self.shellCheckOutput(['stop', 'b2g'])
> >+        # Wait for a bit to make sure B2G has completely shut down.
> >+        time.sleep(10)
> 
> I think it would be better (and faster) to check if the B2G process has
> stopped on some interval.

Sure, I've replaced this to check for the b2g process at most 10 times, waiting 1 second before each check. It will throw an exception if it doesn't stop in time. Thoughts?

> A few things here:
> 
> (1) What is backup_pref and how is it used?

What was known as 'backup_pref' was the original user.js file found on the device. I removed that variable, but the process is the same. We keep a backup of it as /data/local/user.js.orig because in order to add the testing prefs, we append our preferences to a copy of this file, and use that. When we're done, we restore the original user.js file. We do it this way because the stuff in user.js is dependent on the b2g build. There are new prefs that get added when b2g gets build, so instead, we just use the profile given to us and append prefs as needed.

> (2) Could we streamline this stuff to use an instance of mozprofile instead?

Once there's a clear set of prefs for b2g, this would make sense to do, yes.


Also, I've created Bug 795435 to track unit-test development. I'd like to get unit tests, but I'd like to lay the brickwork for upcoming eideticker changes first.

Comment 11

[William Lachance (:wlach)]

Comment on attachment 666020 [details] [diff] [review]
Add mozb2g dir and b2gmixin v2.0

f+ with the big caveat that I don't think B2GMixin should be a publically exported class -- the mixin is an implementation detail that we shouldn't bother people with. Instead, I would suggest creating B2GADB and B2GSUT classes, similar to what we do in mozdevice:

https://github.com/mozilla/mozbase/blob/master/mozdevice/mozdevice/droid.py#L79

Comment 12

[William Lachance (:wlach)]

(In reply to Malini Das [:mdas] from comment #10)

> > >+    def restart_b2g(self):
> > >+        """
> > >+        Restarts the b2g process on the device
> > >+        """
> > >+        #restart b2g so we start with a clean slate
> > >+        self.shellCheckOutput(['stop', 'b2g'])
> > >+        # Wait for a bit to make sure B2G has completely shut down.
> > >+        time.sleep(10)
> > 
> > I think it would be better (and faster) to check if the B2G process has
> > stopped on some interval.
> 
> Sure, I've replaced this to check for the b2g process at most 10 times,
> waiting 1 second before each check. It will throw an exception if it doesn't
> stop in time. Thoughts?

Sounds good to me!

Comment 13

[Jonathan Griffin (:jgriffin)]

Comment on attachment 666020 [details] [diff] [review]
Add mozb2g dir and b2gmixin v2.0

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

I think this is a good starting point.  We can iterate on it as we begin to utilize it in the other test harnesses.

Comment 14

[Malini Das [:mdas] - Away, not checking bugmail]

Landed with wlach's recommended export changes in:

https://github.com/mozilla/mozbase/commit/65b9e661f516ec0c02508e67ffb4a5c7204c5c99

Comment 15

[Jeff Hammel]

Can this be closed?

Comment 16

[Malini Das [:mdas] - Away, not checking bugmail]

Landed in m-c!