620981 - devicemanager.py needs to be updated with m-c and talos

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

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

Attachment: remote-testing patch for master copy of devicemanager.py (1.0)

this is a bug to manage the merge of devicemanager.py.  First round is automation/remote-testing, then mozilla-central.

Comment 1

[Mark Côté [:mcote]]

Comment on attachment 499320 [details] [diff] [review]
remote-testing patch for master copy of devicemanager.py (1.0)

>-  def launchProcess(self, cmd, outputFile = "process.txt", cwd = ''):
>+  def launchProcess(self, cmd, outputFile = "process.txt", cwd = '', env = ''):
>     cmdline = subprocess.list2cmdline(cmd)
>     if (outputFile == "process.txt" or outputFile == None):
>       outputFile = self.getDeviceRoot() + '/' + "process.txt"
>       cmdline += " > " + outputFile
>+    
>+    # Prepend our env to the command 
>+    cmdline = ('%s ' % self.formatEnvString(env)) + cmdline

Personally I would have done

    cmdline = '%s %s' % (self.formatEnvString(env), cmdline)

since I found yours a bit hard to parse, but not a big deal.

>   def poll(self, process):
>     try:
>-      if (self.processExist(process) == None):
>+      if (self.processExist(process) == ''):
>         return None
>       return 1
>     except:
>       return None
>     return 1

Hm I noticed that processExist() will still return None if procList == None.  processExist() should be fixed to always return the same value on error--if for some reason it fails to get a process list, poll will return 1!

>   
>   # iterates process list and returns pid if exists, otherwise ''
>   def processExist(self, appname):
>     pid = ''
>+
>+    #remove the environment variables in the cli if they exist
>+    parts = appname.split(' ')
>+    for p in parts:
>+      if (p is ''):
>+        parts.remove(p)

Why do you hate filter()? :P

>+
>+    if len(parts[0].strip('"').split('=')) > 1:
>+      envvars = parts[0].strip('"').split(',')
>+      for e in envvars:
>+        env = e.split('=')
>+        if (len(env) > 1):
>+          os.environ[env[0]] = str(env[1])
>+      appname = ' '.join(parts[1:])
>+

Uhhh I don't think we should be setting those environment variables locally, unless I'm missing something... please remove or justify.

>+  """
>+    Returns a properly formatted env string for the agent.
>+    Input - env, which is either None, '', or a dict
>+    Output - a quoted string of the form: '"envvar1=val1,envvar2=val2..."'
>+    If env is None or '' return '""' (empty quoted string)
>+  """
>+  def formatEnvString(self, env):
>+    if (env == None or env == ''):
>+      return '""'
>+
>+    envstr = '"'
>+    # TODO: I believe this is inefficient for large dicts
>+    for k, v in env.items():
>+      envstr += ('%s=%s,' % (k, v))

It is inefficient.  Prefer env.iteritems().

>+    
>+    # kill the trailing comma, add the last quote
>+    envstr = envstr.rstrip(',')
>+    envstr += '"'
>+
>+    return envstr
>+

Actually you can do all of that in one line:

envstr = '"' + ','.join(map(lambda x: '%s=%s' % (x[0], x[1]), env.iteritems())) + '"'

And it eliminates the need to remove the trailing comma.  Some people find that a bit hard to read, though I personally love map(), filter(), et al.

Of the above, I think processExist() should really be fixed to return consistent error values, and the os.environ() stuff should be removed.  Both easy.  Up to you if you want to change anything else.

r+ with those two changes.

Comment 2

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

Attachment: remote-testing patch for master copy of devicemanager.py (1.1)

updated patch with all feedback incorporated

Comment 3

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

http://hg.mozilla.org/automation/remote-testing/pushloghtml?changeset=e983a9b21cb7

Comment 4

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

Attachment: mozilla-central patch for master copy of devicemanager.py (1.0)

updated patch with all devicemanager.py changes to date.

Comment 5

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

accidentally resolved this, this bug needs to be open for the 3 places devicemanager.py lives.

Comment 6

[cmtalbert]

Comment on attachment 499338 [details] [diff] [review]
mozilla-central patch for master copy of devicemanager.py (1.0)

This looks good.  Should we also take the Reuse socket address that Mark suggested in: https://bugzilla.mozilla.org/show_bug.cgi?id=612792#c6

as well on this patch?

r+ on this existing stuff.

Should we also take Mark's suggestion here, or do we want to go live with this since this is what's currently in production?

Comment 7

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

Attachment: mozilla-central patch for master copy of devicemanager.py (1.1)

updated with socket_reuse and filter(lambda...)

Comment 8

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

Attachment: mozilla-central patch for master copy of devicemanager.py (1.2)

fixed typo, ran manual tests of talos, unittests and a few tests around processExists()

Comment 9

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

Attachment: mozilla-central patch for master copy of devicemanager.py (1.3)

updated callbackserver to implement reuse_address properly.

Comment 10

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

Attachment: remote-testing patch for master copy of devicemanager.py 2 (1.0)

second patch to get m-c changes into r-t.

Comment 11

[Mark Côté [:mcote]]

Comment on attachment 499369 [details] [diff] [review]
remote-testing patch for master copy of devicemanager.py 2 (1.0)

Yessir, that would be an unconditional r+.

Comment 12

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

http://hg.mozilla.org/automation/remote-testing/pushloghtml?changeset=e983a9b21cb7