657987 - fix bcontroller to use config file, not commandline options

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

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

Attachment: bcontroller.json instead of command line parameters to bcontroller (1.0)

as we keep adding more and more options to talos, the passing of variables on the commandline from run_tests.py->bcontroller.py is becoming cumbersome and clunky.  

This patch refactors all use of bcontroller into a single function (inside utils.py) which now writes all the relevant information to a bcontroller.json file.  This file is read by bcontroller when run.  

Inside of bcontroller, I removed a lot of the variable passing between each class as that list was getting longer and longer.  Now I pass an object to each class and reference the information from that object.

Comment 1

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

Comment on attachment 533329 [details] [diff] [review]
bcontroller.json instead of command line parameters to bcontroller (1.0)

would like to get jhammel's feedback on the usage of python and anode's review on how this looks to talos.

Comment 2

[alice nodelman [:alice] [:anode]]

Comment on attachment 533329 [details] [diff] [review]
bcontroller.json instead of command line parameters to bcontroller (1.0)

I'm not seeing all of these defaults recreated with your new code, otherwise it looks like a good solution to our long command line problem.

-   command = ""
-   name = "firefox" #default
-   child_process = "plugin-container" #default
-   timeout = ""
-   log = ""
-   mod = ""
-   host = ""
-   deviceRoot = ""
-   port = 20701
-   test_timeout = 1200 #no output from the browser in 20 minutes = failure

Comment 3

[alice nodelman [:alice] [:anode]]

Also, as discussed in irc, talos uses python2.4, so you will need a different solution that doesn't involve importing simplejson.

Comment 4

[Jeff Hammel]

Comment on attachment 533329 [details] [diff] [review]
bcontroller.json instead of command line parameters to bcontroller (1.0)

Having options being passed to be kept as an (dict) attribute on
BrowserController and BrowserWaiter makes what you can pass in
intelligible only by introspecting the code.  Dereferencing dict
values, e.g. `self.options['url_mod']` is more cumbersome than
`self.url_mod` but also will throw a run-time error if the key is not
passed or misspeld.

As an alternative, the defaults could live on the classes and be
populated from **options.  E.g.


class BrowserWaiter(threading.Thread):
  # instance defaults
  defaults = {'command': None,
              'timeout': 1200,
              'log': None,
              'url_mod': None}

  def __init__(self, deviceManager=None, **options):
    self.deviceManager = deviceManager
    for key, value in self.defaults.items():
      setattr(self, key, options.get(key, value))

I'm not really sure if the defaults should live at the class level or
the module global level. You'd have to hang on to options in
BrowserController since you instantiate BrowserWaiter in
BrowserController.run

Alternatively, if you wanted to keep the defaults as living on the
ctors, you could use the inspect module to do this.  As example code:

import inspect
def instantiate(cls, **options):
  args = inspect.getargspec(cls.__init__).args[1:] # ignore 'self'
  kw = dict([options[arg] for arg in args if arg in options])
  return cls(**kw) # could pass other things too

I somewhat prefer the defaults-type approach as otherwise the ctor still
has to do:

 self.command = command
 self.mod = mod
 self.anothervariable = anothervariable
 ...

If we don't use getopt, the import should be removed.

+    if (self.options['host'] <> ''):

regardless of whether this is self.host or self.options['host'], let's
fix this while touching:: `if self.host:`

+        self.options['command'] = self.options['command'] + eval(self.options['url_mod'])

self.options['command'] += eval(self.options['url_mod'])

+      if (self.options['env'] is not ''):
if self.options['env']:

+   fHandle = open('bcontroller.json', 'r')
+   data = fHandle.read()
+   fHandle.close()
+   options = json.loads(data)
It would be nice if this was specifiable from the command-line vs
hardcoded. Also, I'd condense this to

options = json.loads(file('bcontroller.json', 'r').read())

but not important

+   for e in options['env']:
+     if (first == False):
+       env += ','
+     else:
+       first = False
+     env += str(e) + '=' + str(options['env'][e])

I find this much more readable
options['env'] = ','.join(['%s=%s' % (str(key), str(value))
                           for key, value in options.get('env', {}).items()])
+   if options['command'] and options['browser_wait'] and
options['browser_log']:
+     bcontroller = BrowserController(options)
      bcontroller.run()
    else:
      print "\nFAIL: no command\n"
      sys.stdout.flush()

You'll get a key error if these aren't defined so you probably won't
see the else (unless someone very sillyly sets these to empty-string
or other non-true value). I would have BrowserController be in charge
of such things.  Probably better to `print >> sys.stderr` since A. its
an error and B. stderr is non-buffered (usually) so you wouldn't have
to flush.

+        with open("bcontroller.json", "w") as config:
IIRC, this will not work until python 2.5 or maybe 2.6. Why not
`config = open("ahopefullyconfigurablefilename.json", "w")`?

+        b_cmd = 'python bcontroller.py'
+        return b_cmd
silly :) `return 'python ahopefullyconfigurablefilename.json'`

I'm guessing the command line builder in InitializeNewProfile should
be refactored, but here probably isn't the time to do it.

+#TODO: when we upgrade to python 2.6, just use
json.dumps(browser_config)
+def jsonDump(obj, vals = []):

I'm not sure why this is the case?  Doesn't simplejson in 2.4 have a
.dumps or am I crazy? If we really have to serialize JSON ourselves,
this makes me wonder if it is a good format to have.  Similarly, jsonString

As mentioned before, simplejson is not available OOTB on 2.4 or 2.5.
Do we have this setup for the appropriate environment?

f=mostly good? But I'd like to see a few improvements above.

Comment 5

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

Attachment: bcontroller.yml instead of command line parameters to bcontroller (2.0)

updated patch to use yaml instead of json.  Also updated to have a configurable intermediate filename coming from the sample.config (or other .config).

Comment 6

[Jeff Hammel]

Comment on attachment 534499 [details] [diff] [review]
bcontroller.yml instead of command line parameters to bcontroller (2.0)

+defaults = {'endTime': -1, 'returncode': -1,

Probably better to put these on one line each. There is no shortage of
newline characters.

+  def __init__(self, options, deviceManager = None):
I would pass options like this:

def __init__(self, deviceManager=None, **options):

This clearly indicates that options is a dict and also allows
instantiation via:

BrowserWaiter(deviceManager=myDeviceManager, test_timeout=1500)

vs.

BrowserWaiter({'test_timeout': 1500}, deviceManager=myDeviceManager)

+    if (self.command == defaults['command'] or
+        self.browser_wait == defaults['browser_wait'] or
+        self.browser_log == defaults['browser_log']):
+      print >> sys.stderr, "\nFAIL: incorrect parameters to
bcontroller\n"
+      return

I would check these in the constructor.  I probably would not check
them vs defaults but against real values.  I would probably make the
default None (or something similar) and do something like

if [undefined for undefined in
    (self.command, self.browser_wait, self.browser_log)
    if undefined is None]: ...

Also, does a `print >> stderr` + return suffice? or is it better to
raise an e.g. AssertionError?

+class BControllerOptions(optparse.OptionParser):
+    """Parses BController commandline options."""

Nice :)

+        defaults["configFile"] = ''
...though maybe this should be None?

   if 'test_timeout' in yaml_config and yaml_config['test_timeout'] !=
   None and int(yaml_config['test_timeout']) > 0:

Not part of the patch, but i just have to say:

    if int(yaml_config.get('test_timeout', -1)) > 0:

Similarly, and is part of the patch:

+  if 'bcontroller_config' in yaml_config:
+      browser_config['bcontroller_config'] =
yaml_config['bcontroller_config']
+  else:
+      browser_config['bcontroller_config'] = 'bcontroller.yml'

should be:

browser_config['bcontroller_config'] =
yaml_config.get('bcontroller_config', 'bcontroller.yml')

[These assume that yaml_config is a dict; if not, disregard]

Overall this looks like a big improvement. f+

Comment 7

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

Attachment: bcontroller.yml instead of command line parameters to bcontroller (2.1)

updated with jhammel's latest feedback.  It was minor cleanup, but I ended up doing a lot of yaml_config.get() work to clean up run_tests.py.

:anode, if you could give this your scrutiny when you have a few cycles, I would appreciate that.

Comment 8

[alice nodelman [:alice] [:anode]]

Comment on attachment 535359 [details] [diff] [review]
bcontroller.yml instead of command line parameters to bcontroller (2.1)

This looks good to me, but definitely needs some time in the testing environment to ensure that it works as expected.

Ah yaml, last refuge of the damned. :)

Comment 9

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

http://hg.mozilla.org/build/talos/rev/52656a53e695