649681 - update devicemanager to be what we are using in production

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: devicemanager.py and updated tests (1.0)

devicemanager.py needs to be updates in 3 branches:
1) remote-testing (including tests)
2) mozilla-central
3) talos

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: mozilla central devicemanager update (1.0)

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: updated for talos bits (1.0)

Comment 3

[Mark Côté [:mcote]]

Comment on attachment 525712 [details] [diff] [review]
devicemanager.py and updated tests (1.0)

   def processExist(self, appname):
     pid = None
 
-    #remove the environment variables in the cli if they exist
+    #filter out extra spaces
     parts = filter(lambda x: x != '', appname.split(' '))
+    appname = ' '.join(parts[:])

Why are you making a temporary copy of the array here?

-    if len(parts[0].strip('"').split('=')) > 1:
-      appname = ' '.join(parts[1:])
+    #filter out the quoted env string if it exists
+    parts = appname.split('"')
+    if (len(parts) > 2):
+      appname = ' '.join(parts[2:]).strip()

I realize the intention of this piece of code is unchanged, but more comments about exactly what you are trying to do here would be useful.  Specifically, why do you only expect at most one environment variable, and why must the value of that variable be quoted?
   
+    if (ipAddr is not None):
+    #create update.info file:
+      try:
+        destname = '/data/data/com.mozilla.SUTAgentAndroid/files/update.info'
+        data = "%s,%s\rrebooting\r" % (ipAddr, port)
+        retVal = self.verifySendCMD(['push ' + destname + ' ' + str(len(data)) + '\r\n', data], newline = False)
+      except(DMError):
+        return None
+
+      ip, port = self.getCallbackIpAndPort(ipAddr, port)
+      cmd += " %s %s" % (ip, port)
+      # Set up our callback server
+      callbacksvr = callbackServer(ip, port, self.debug)
+
     try:
-      status = self.sendCMD([cmd])
-    except DMError:
+      status = self.verifySendCMD([cmd])
+    except(DMError):
       return None

Are we doing anything with status?  Doesn't look like it, in which case I would omit the assignment.
 
-    if (wait == True):
-      #this sleeps up to 5 minutes in 30 second intervals
-      count = 0
-      while (count < 10):
-        if (self.debug >= 4): print "DEBUG: sleeping 30 seconds while waiting for reboot"
-        time.sleep(30)
-        waitstatus = self.getDeviceRoot()
-        if (waitstatus is not None):
-          break
-        self.retries = 0
-        count += 1
+    if (ipAddr is not None):
+      callbacksvrstatus = callbacksvr.disconnect()
 
-      if (count >= 10):
-        return None
-
-    if (self.debug >= 3): print "INFO: rebt- got status back: " + str(status)
-    return status
+    if (self.debug > 3): print "INFO: rebt- got status back: " + str(callbacksvrstatus)
+    return callbacksvrstatus

I assume there's a good reason for getting rid of the pingback stuff...
 
diff --git a/tests/test_resolution.py b/tests/test_resolution.py
--- a/tests/test_resolution.py
+++ b/tests/test_resolution.py
@@ -16,17 +16,19 @@ class ResolutionTestCase(DeviceManagerTe
         if (width == w and height == h):
             return True
         return False
 
     def runTest(self):
         """This tests the adjustResolution() function.
         """
 
+        self.localIP = self.nettools().getLanIp()
         self.screentype = 'hdmi' #can also be vga|crt, but we should have hdmi images
+        self.screentype = 'crt' #can also be vga|crt, but we should have hdmi images

Why are you assigning to self.screentype twice in a row?

r+ with the above addressed.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

mcote:
"Why are you making a temporary copy of the array here?", I am reassigning the value of appname which was passed in so we can use it in the function.  

I added a comment in the code outlining the env vars.  Basically the env vars as defined need to be in a "a=b;c=d;" format with no spaces and surrounded by quotes.

I cleaned up the status return code and intentionally removed the pingback code because that is the intended behavior we decided on for managing reboots.

also good catch on my screen type.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

remote-testing repo:
http://hg.mozilla.org/automation/remote-testing/pushloghtml?changeset=7c39f92964e2

Comment 6

[Mark Côté [:mcote]]

W.r.t. the temporary array copy, my point is that instead of doing this

  appname = ' '.join(parts[:])

you can just do this

  appname = ' '.join(parts)

The [:] means you are making a copy of parts, which is unnecessary here.

Everything else seems fine.

Comment 7

[cmtalbert]

Comment on attachment 525713 [details] [diff] [review]
mozilla central devicemanager update (1.0)

># HG changeset patch
># Parent 5ca439a086fb1abfbb9c1fe4babb152acb1a5b2b
>
>diff --git a/build/mobile/devicemanager.py b/build/mobile/devicemanager.py
>--- a/build/mobile/devicemanager.py
>+++ b/build/mobile/devicemanager.py
>@@ -567,21 +567,24 @@ class DeviceManager:
>   # iterates process list and returns pid if exists, otherwise None
>   # external function
>   # returns:
>   #  success: pid
>   #  failure: None
>   def processExist(self, appname):
>     pid = None
> 
>-    #remove the environment variables in the cli if they exist
>+    #filter out extra spaces
>     parts = filter(lambda x: x != '', appname.split(' '))
>+    appname = ' '.join(parts[:])
As Cote said, you can just do ' '.join(parts), I don't think making an extra copy of parts is going to kill us here though.

>@@ -991,42 +994,46 @@ class DeviceManager:
>       return None
> 
>     return data
> 
>   # external function
>   # returns:
>   #  success: status from test agent
>   #  failure: None
>-  def reboot(self, wait = False):
>-    cmd = 'rebt'
>-    if (self.debug >= 3): print "INFO: sending rebt command"
>-    
>+  def reboot(self, ipAddr=None, port=30000):
>+    cmd = 'rebt'   
>+
>+    if (self.debug > 3): print "INFO: sending rebt command"
>+    callbacksvrstatus = None    
>+
>+    if (ipAddr is not None):
>+    #create update.info file:
>+      try:
>+        destname = '/data/data/com.mozilla.SUTAgentAndroid/files/update.info'
I know we talked about putting in some hooks in here so that we could determine what OS we're running against and gate some of our functionality based on that.  This is very android specific (obviously) and it'd be nice to gate on knowing that we have an android OS underneath us.  I'm happy with doing that in a follow on bug though.

> class myServer(SocketServer.TCPServer):
> 
>   def disconnect(self, step = 60, timeout = 600):
>     t = 0
>     if (self.debug >= 3): print "Calling disconnect on callback server"
>     while t < timeout:
>       if (gCallbackData):
>         # Got the data back
>         if (self.debug >= 3): print "Got data back from agent: " + str(gCallbackData)
>         break
>+      else:
>+        if (self.debug >= 0): print '.',
>       time.sleep(step)
>       t += step
> 
>     try:
>       if (self.debug >= 3): print "Shutting down server now"
>       self.server.shutdown()
>     except:
>-      print "Unable to shutdown callback server - check for a connection on port: " + str(self.port)
>+      if (self.debug >= 1): print "Unable to shutdown callback server - check for a connection on port: " + str(self.port)
>+    time.sleep(60)
What's this ^ for?  Where did the magic minute come from?

>     return gCallbackData

r+ with the nits addressed, and the follow on bug filed.  I'm very suspicious of the sleep(60) though.  Is there no more robust way to deal with that?  If the devices slow down after being in use, for example, I worry this is going to end up biting us in the a$$.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=ef293f1bc91d

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

and the talos changes:
http://hg.mozilla.org/build/talos/pushloghtml?changeset=4eac5cb5106a