Closed Bug 565733 Opened 14 years ago Closed 13 years ago

Tracking bug: audit mozrunner and figure out potential directions for improvement

Categories

(Testing :: Mozbase, defect)

Product:
Testing
Component:
Mozbase
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED INVALID

People

(Reporter: k0scist, Assigned: k0scist)

Details

Mozmill is currently undergoing a 1.4.2 release.  The purpose of this bug is to figure out post 1.4.2 directions for MozRunner (consumed by mozmill).  The code should be audited, the audit discussed, and takeaways roadmapped, triaged, and included in the mozrunner code going forward.

This is a tracking bug.  Portions of the audit and associated discussions can go here.
A stab at the audit:

Conceptually
------------

- mozrunner does several things (see categories below)
  I would break this up based on intent:

  - utilities: generic utility functions.  I find it a bit awful
    to have a "utility dumping ground", but sometimes you need generic
    functions.  I would advocate some effort into pushing these 
    upstream (-> python std lib)
    * findInPath

  - process: objects/methods dealing with manipulating processes.
    These could conceivably live with utilities, though there is 
    a common enough shared concept to keep these together and we do a
    bunch of this
    * def get_pids(name): get all PIDs by process name
      - this has a bug in that it just searches for the string;
        this is not very robust, as 'ps' would match 'zipsplit' (e.g.)
    * def kill_process_by_name
    * killableprocess.py, winprocess.py

  - profiles:  the Profile class in __init__.py

  - firefox/app: objects/methods directly dealing with Firefox and
    other mozilla apps
    * class Runner
    * def find_binary(path, app_name): could be a standalone function?

- a nice-to-have would be a README used as the long_description in
  setup.py:

{{{
desc = """Reliable start/stop/configuration of Mozilla Applications (Firefox, Thunderbird, etc.)"""
try:
  summ = file(os.path.join(os.path.dirname(__file__), 'README.txt')).read()
except IOError: 
  summ = desc
}}}


Proposed Restructure
--------------------

* Move some stuff around:

profiles:
 - __init__.Profile -> profile.py
 - __init__.FirefoxProfile,__init__.ThunderbirdProfile -> profile.py

processes:
 - __init__.{get_pids,kill_process_by_name} -> process.py
 - __init__.runcommand -> [refactor] -> process.py
 - {wpk.py,winprocess.py} -> winprocess.py (could go in process.py too)
   - and qijo.py? or should this be kept separate?
 - killableprocess.py -> process.py
 
utilities:
 - findInPath -> utils.py

runners:
  - __init__.*Runner -> runner.py

 -> /dev/null:
    - getoutput
    - NaN
    - makedirs


* figure out what to do with CLI.  Make whatever needs to be
  consumable...consumable, but perhaps not via inheritence

* empty out __init__.py (save, except, for some convenience imports)

* Write some tests!

* Write a README

Code
----

- http://github.com/mikeal/mozrunner/tree/master/tests/ : what is the
  purpose of this directory?  It doesn't seem to be usable tests.  My
  recommendation: to wipe.  My further recommendation: write tests!

- The first half of 
http://github.com/mikeal/mozrunner/blob/master/mozrunner/__init__.py
is utility functions that are not specific to mozrunner.  Some of
these, really, should be in the standard lib (e.g. findInPath, which I
would probably call `which`).  Is it worth separating these out and
making one or more packages for them?  Is it worth pinging the python
community about these functions?  At the very least, it's probably
best to put them in their own utils.py file.
     - in the same way, all of the process-related stuff probably goes together

- pwd is never used
{{{
if sys.platform != 'win32':
    import pwd
}}}

- getoutput : what is the purpose of this?  why not use
  .communicate()? is there a buffer issue? why not use  (c)StringIO?
  I would nix this function or replace it with a more general
  front-end to subprocess that returns (exit code, stdout, stderr)

- silly abstraction:
{{{
stdout = sys.stdout
stderr = sys.stderr
stdin = sys.stdin
}}}

- def NaN: this doesn't seem to be used, so -> /dev/null (plus it's silly)

- def makedirs:  I'm not sure if I understand the purpose of this
  function.  Is this the same as os.makedirs or is there some subtlty there?

- class Profile: this is really more of a profile manager class (I think)
  - binary is passed but never used -> /dev/null
: class Profile.__init__
  - this line has no purpose:   self.preferences = copy.copy(self.preferences)`
  - use non-mutable function argument defaults (addons=[], preferences={}):
    mutable arguments are bad for defaults.  Example: tmp.py
{{{
class Foo(object):
  def __init__(self, arg=[]):
    arg.append(len(arg))
    print arg

a = Foo()
b = Foo()

--

> python tmp.py 
[0]
[0, 1]
}}}
    The general thing-to-do is to set the default as None and then e.g.
    `if addons is None: addons = []` 
    - this also applies to `class Runner`

   - create_new and profile (path) are both given as arguments;  these
     are redundant, as told by this unnecessary logic:
{{{
        if profile is not None and create_new is True:
            raise Exception('You cannot set the profie location if you want mozrunner to create a new one for you.')
        if create_new is False and profile is None:
            raise Exception('If you set create_new to False you must provide the location of the profile you would like to run')
}}}

        should be "if profile is None self.create()"

: class Profile.install_addon
  - any reason not to use Zipfile.extractall here? (if so, should be documented) 
{{{
            compressed_file = zipfile.ZipFile(addon, "r")
            for name in compressed_file.namelist():
                if name.endswith('/'):
                    makedirs(os.path.join(tmpdir, name))
                else:
                    if not os.path.isdir(os.path.dirname(os.path.join(tmpdir, name))):
                        makedirs(os.path.dirname(os.path.join(tmpdir, name)))
                    data = compressed_file.read(name)
                    f = open(os.path.join(tmpdir, name), 'w')
                    f.write(data) ; f.close()
            addon = tmpdir 
}}}

: class FirefoxProfile, class ThunderbirdProfile: `names` should be documented
  - also, identical names exist in these classes and in FirefoxRunner, ThunderbirdRunner
  - afaict, the *Profile.names are never used; -> /dev/null

: class Runner.__init__
  - self.env = copy.copy(os.environ): no need for this;
    should be `self.env = os.environ.copy()`
  - self.env.update({'MOZ_NO_REMOTE':"1",}): this is silly for a
    single entry: `self.env['MOZ_NO_REMOTE'] = '1'
  - if (sys.platform == 'linux2') or (sys.platform in ('sunos5',
  'solaris')):
    silly; `if sys.platform in ('linux2', 'sunos5', 'solaris')`
: class Runner.find_binary
  - move logic to where it is needed:
{{{
                binary = findInPath(name)
                if sys.platform == 'cygwin':
                    program_files = os.environ['PROGRAMFILES']
                else:
                    program_files = os.environ['ProgramFiles']

                if binary is None:
                    for bin in [(program_files, 'Mozilla Firefox', 'firefox.exe'),
                                ]:
}}}
     should become
{{{
binary = findInPath(name)
if binary is None:
    if sys.platform == 'cygwin':
        program_files = os.environ['PROGRAMFILES']
    else:
        program_files = os.environ['ProgramFiles']

    for bin in [(program_files, 'Mozilla Firefox', 'firefox.exe'),
                                ]:
}}}

: class Runner.start, Runner.wait, Runner.kill:
  - Runner.wait and Runner.kill depend on self.process_handler being set
    in Runner.start.  If this is not invoked correctly, these will fail
    with an AttributeError.  At the very least, this should be checked for

: class CLI 
  - I'm not even sure where to begin. I'm not sure what having this as a 
  class, versus a function, buys us at all; purely for downstream
  inheritence?  I would break this up into functions, e.g. get_parser, 
  etc, rather than having a class, move most of the logic into the cli
  function, and get rid of the unnecessary abstractions (<snip/> lots
  of details not worth going into here)

  - self.addons = self.options.addons.split(','): since we're using 
    OptionParser anyway, why not let it do the work?
{{{
'-a', "--addons",): dict(dest="addons",
                         action="append",
                         help="Addons paths to install.",
                         )
}}}

    (see http://docs.python.org/library/optparse.html#standard-option-actions)
    (the bare try: except: with no warning to user is pretty silly
    too, but the above fix eliminates this problem)
    

: wpk.py: missing license (and documentation)
  - could make __main__ a usable utility instead of just a test
  - there is kill_process_by_name() in __init__.py and in wpk.py and
    the former does not consume the latter
  - likewise, there is get_pids in both places;  any way to do
    better reflection?

: winprocess.py: where is this from?  is this a package we can/should
  depend on?  should we package it?

: qijo.py: missing license (and documentation)

: killableproces.py:  I didn't look at this too thoroughly.  The one
thing that I superficially noticed is that there are if mswindows all
over the place.  Maybe have one if check at the top of the file (and
maybe keeping all windows stuff in winprocess.py) is the way to go
(that is, process.py contains all of the unixy code.  If it's windows,
then just import the relavent code from winprocess.py, which if it
used __all__ to define what it has, you can just do `from winprocess
import *`)
Next steps:  audit automation.py with respect to mozrunner and see what additional functionality can/should be included
as alice pointed out, xml.etree is new to python 2.5 (see http://docs.python.org/library/xml.etree.elementtree.html ).  since talos (and other infrastructure) uses python 2.4, she backported this to use xml.dom (see http://docs.python.org/library/xml.dom.html ) which is available in python 2.0 and greater.  This should probably be done in mozmill as long as mozilla uses python < 2.5
Jeff, that's another awesome comment and I agree with most of it! Lets also CC Mikeal on that bug.

Lets give some short comments...

> * figure out what to do with CLI.  Make whatever needs to be
>   consumable...consumable, but perhaps not via inheritence

Can you be a bit more specific on that part? What I really don't like is the usage of the option parser and using that data. We should have properties in the base Mozrunner class which will be set by the CLI class or function. Just like that:

http://github.com/whimboo/mozmill/blob/firefox-automation/scripts/firefox-automation/testrun_bft.py#L46

> * Write some tests!

+++

> - http://github.com/mikeal/mozrunner/tree/master/tests/ : what is the
>   purpose of this directory?  It doesn't seem to be usable tests.  My
>   recommendation: to wipe.  My further recommendation: write tests!

We definitely have to leave it there and write tests. That's a lot better than removing that folder, which never hasn't been touched again.

> - def makedirs:  I'm not sure if I understand the purpose of this
>   function.  Is this the same as os.makedirs or is there some subtlty there?

I don't remember. Mikeal, can you help us?

>   - if (sys.platform == 'linux2') or (sys.platform in ('sunos5',
>   'solaris')):
>     silly; `if sys.platform in ('linux2', 'sunos5', 'solaris')`

Right. That was silly from me. Haven't seen that when I fixed it a while back.

> : class Runner.find_binary
>   - move logic to where it is needed:
> {{{
>                 binary = findInPath(name)
>                 if sys.platform == 'cygwin':
>                     program_files = os.environ['PROGRAMFILES']
>                 else:
>                     program_files = os.environ['ProgramFiles']
> 
>                 if binary is None:
>                     for bin in [(program_files, 'Mozilla Firefox',
> 'firefox.exe'),
>                                 ]:

That's only for Windows, right? We should get rid of that code in favor of using the registry (bug 554709). This bug is already targeted for the next Mozmill release.

> : class CLI 
>   - I'm not even sure where to begin. I'm not sure what having this as a 
>   class, versus a function, buys us at all; purely for downstream
>   inheritence?  I would break this up into functions, e.g. get_parser, 
>   etc, rather than having a class, move most of the logic into the cli
>   function, and get rid of the unnecessary abstractions (<snip/> lots
>   of details not worth going into here)

Having the capability to inherit from a CLI class would be great. That way none of the custom modules / scripts would have to create all those parser options again and again and only have to append their own options.

(In reply to comment #2)
> Next steps:  audit automation.py with respect to mozrunner and see what
> additional functionality can/should be included

Which automation.py? Do you mean my test-run classes?
Hardware: Other → All
I've implemented an initial refactor: http://github.com/k0s/mozrunner
Anyone wants to review?

Notes below on what is implemented and what is not.

(In reply to comment #1)

> - mozrunner does several things (see categories below)
>   I would break this up based on intent:
> 
>   - utilities: generic utility functions.  I find it a bit awful
>     to have a "utility dumping ground", but sometimes you need generic
>     functions.  I would advocate some effort into pushing these 
>     upstream (-> python std lib)
>     * findInPath
> 
>   - process: objects/methods dealing with manipulating processes.
>     These could conceivably live with utilities, though there is 
>     a common enough shared concept to keep these together and we do a
>     bunch of this
>     * def get_pids(name): get all PIDs by process name
>       - this has a bug in that it just searches for the string;
>         this is not very robust, as 'ps' would match 'zipsplit' (e.g.)
>     * def kill_process_by_name
>     * killableprocess.py, winprocess.py
> 
>   - profiles:  the Profile class in __init__.py
> 
>   - firefox/app: objects/methods directly dealing with Firefox and
>     other mozilla apps
>     * class Runner
>     * def find_binary(path, app_name): could be a standalone function?

I did this a bit differently.  It looks like winprocess.py isn't our code, so I left this alone.  I also left killableprocess it's own file.  It could go in process.py, if desired

> - a nice-to-have would be a README used as the long_description in
>   setup.py:
> 
> {{{
> desc = """Reliable start/stop/configuration of Mozilla Applications (Firefox,
> Thunderbird, etc.)"""
> try:
>   summ = file(os.path.join(os.path.dirname(__file__), 'README.txt')).read()
> except IOError: 
>   summ = desc
> }}}

I stubbed this.  Nothing in the README yet, really.

> 
> Proposed Restructure
> --------------------
> 
> * Move some stuff around:
> 
> profiles:
>  - __init__.Profile -> profile.py
>  - __init__.FirefoxProfile,__init__.ThunderbirdProfile -> profile.py
Yep.

> processes:
>  - __init__.{get_pids,kill_process_by_name} -> process.py
>  - __init__.runcommand -> [refactor] -> process.py
Yep.

>  - {wpk.py,winprocess.py} -> winprocess.py (could go in process.py too)
>    - and qijo.py? or should this be kept separate?
>  - killableprocess.py -> process.py
Nope.
 
> utilities:
>  - findInPath -> utils.py
Yep.

> runners:
>   - __init__.*Runner -> runner.py
Yep.

>  -> /dev/null:
>     - getoutput
>     - NaN
>     - makedirs
Yep.

> 
> * figure out what to do with CLI.  Make whatever needs to be
>   consumable...consumable, but perhaps not via inheritence
Punted on this for now.  I cleaned up a bit and made it accept command-line arguments, but otherwise left it as-is

> * empty out __init__.py (save, except, for some convenience imports)
Yep.

> * Write some tests!
Barely.

> * Write a README
Again, only in the literal sense of the word.  Needs to be further filled out.

> Code
> ----
> 
> - http://github.com/mikeal/mozrunner/tree/master/tests/ : what is the
>   purpose of this directory?  It doesn't seem to be usable tests.  My
>   recommendation: to wipe.  My further recommendation: write tests!
I didn't wipe.  I did put tests here.  If anyone knows better python testing practices than me, please feel free to help out (at least conceptually).
> 
> - The first half of 
> http://github.com/mikeal/mozrunner/blob/master/mozrunner/__init__.py
> is utility functions that are not specific to mozrunner.  Some of
> these, really, should be in the standard lib (e.g. findInPath, which I
> would probably call `which`).  Is it worth separating these out and
> making one or more packages for them?  Is it worth pinging the python
> community about these functions?  At the very least, it's probably
> best to put them in their own utils.py file.
>      - in the same way, all of the process-related stuff probably goes together
Yep.

> - pwd is never used
> {{{
> if sys.platform != 'win32':
>     import pwd
> }}}
Wiped.

> - getoutput : what is the purpose of this?  why not use
>   .communicate()? is there a buffer issue? why not use  (c)StringIO?
>   I would nix this function or replace it with a more general
>   front-end to subprocess that returns (exit code, stdout, stderr)
Wiped.

> - silly abstraction:
> {{{
> stdout = sys.stdout
> stderr = sys.stderr
> stdin = sys.stdin
> }}}
wiped

> - def NaN: this doesn't seem to be used, so -> /dev/null (plus it's silly)
Wiped.

> - def makedirs:  I'm not sure if I understand the purpose of this
>   function.  Is this the same as os.makedirs or is there some subtlty there?
Wiped.

> - class Profile: this is really more of a profile manager class (I think)
>   - binary is passed but never used -> /dev/null
> : class Profile.__init__
>   - this line has no purpose:   self.preferences = copy.copy(self.preferences)`
Actually, it does.  Subtle.  I left it.
>   - use non-mutable function argument defaults (addons=[], preferences={}):
>     mutable arguments are bad for defaults.  Example: tmp.py
> {{{
> class Foo(object):
>   def __init__(self, arg=[]):
>     arg.append(len(arg))
>     print arg
> 
> a = Foo()
> b = Foo()
> 
> --
> 
> > python tmp.py 
> [0]
> [0, 1]
> }}}
>     The general thing-to-do is to set the default as None and then e.g.
>     `if addons is None: addons = []` 
>     - this also applies to `class Runner`
I think I fixed all of these.

>    - create_new and profile (path) are both given as arguments;  these
>      are redundant, as told by this unnecessary logic:
> {{{
>         if profile is not None and create_new is True:
>             raise Exception('You cannot set the profie location if you want
> mozrunner to create a new one for you.')
>         if create_new is False and profile is None:
>             raise Exception('If you set create_new to False you must provide
> the location of the profile you would like to run')
> }}}
> 
>         should be "if profile is None self.create()"
Did this.
 
> : class Profile.install_addon
>   - any reason not to use Zipfile.extractall here? (if so, should be
> documented) 
> {{{
>             compressed_file = zipfile.ZipFile(addon, "r")
>             for name in compressed_file.namelist():
>                 if name.endswith('/'):
>                     makedirs(os.path.join(tmpdir, name))
>                 else:
>                     if not os.path.isdir(os.path.dirname(os.path.join(tmpdir,
> name))):
>                         makedirs(os.path.dirname(os.path.join(tmpdir, name)))
>                     data = compressed_file.read(name)
>                     f = open(os.path.join(tmpdir, name), 'w')
>                     f.write(data) ; f.close()
>             addon = tmpdir 
> }}}
I went with extractall here

> : class FirefoxProfile, class ThunderbirdProfile: `names` should be documented
>   - also, identical names exist in these classes and in FirefoxRunner,
> ThunderbirdRunner
>   - afaict, the *Profile.names are never used; -> /dev/null
wiped

> : class Runner.__init__
>   - self.env = copy.copy(os.environ): no need for this;
>     should be `self.env = os.environ.copy()`
Done.

>   - self.env.update({'MOZ_NO_REMOTE':"1",}): this is silly for a
>     single entry: `self.env['MOZ_NO_REMOTE'] = '1'
Done.

>   - if (sys.platform == 'linux2') or (sys.platform in ('sunos5',
>   'solaris')):
>     silly; `if sys.platform in ('linux2', 'sunos5', 'solaris')`
Done;  this happens elsewhere too.

> : class Runner.find_binary
>   - move logic to where it is needed:
> {{{
>                 binary = findInPath(name)
>                 if sys.platform == 'cygwin':
>                     program_files = os.environ['PROGRAMFILES']
>                 else:
>                     program_files = os.environ['ProgramFiles']
> 
>                 if binary is None:
>                     for bin in [(program_files, 'Mozilla Firefox',
> 'firefox.exe'),
>                                 ]:
> }}}
>      should become
> {{{
> binary = findInPath(name)
> if binary is None:
>     if sys.platform == 'cygwin':
>         program_files = os.environ['PROGRAMFILES']
>     else:
>         program_files = os.environ['ProgramFiles']
> 
>     for bin in [(program_files, 'Mozilla Firefox', 'firefox.exe'),
>                                 ]:
> }}}
> 
> : class Runner.start, Runner.wait, Runner.kill:
>   - Runner.wait and Runner.kill depend on self.process_handler being set
>     in Runner.start.  If this is not invoked correctly, these will fail
>     with an AttributeError.  At the very least, this should be checked for
Not done.

> : class CLI 
>   - I'm not even sure where to begin. I'm not sure what having this as a 
>   class, versus a function, buys us at all; purely for downstream
>   inheritence?  I would break this up into functions, e.g. get_parser, 
>   etc, rather than having a class, move most of the logic into the cli
>   function, and get rid of the unnecessary abstractions (<snip/> lots
>   of details not worth going into here)
Again, I punted.  I fixed up a few minor details, but left the existing structure in place.

>   - self.addons = self.options.addons.split(','): since we're using 
>     OptionParser anyway, why not let it do the work?
> {{{
> '-a', "--addons",): dict(dest="addons",
>                          action="append",
>                          help="Addons paths to install.",
>                          )
> }}}
> 
>     (see http://docs.python.org/library/optparse.html#standard-option-actions)
>     (the bare try: except: with no warning to user is pretty silly
>     too, but the above fix eliminates this problem)
Done.

> 
> : wpk.py: missing license (and documentation)
Fixed license.

>   - could make __main__ a usable utility instead of just a test
Punted.

>   - there is kill_process_by_name() in __init__.py and in wpk.py and
>     the former does not consume the latter
It does now

>   - likewise, there is get_pids in both places;  any way to do
>     better reflection?
Also fixed.
 
> : winprocess.py: where is this from?  is this a package we can/should
>   depend on?  should we package it?
Still confused.  Left alone.

> : qijo.py: missing license (and documentation)
Punted.

> : killableproces.py:  I didn't look at this too thoroughly.  The one
> thing that I superficially noticed is that there are if mswindows all
> over the place.  Maybe have one if check at the top of the file (and
> maybe keeping all windows stuff in winprocess.py) is the way to go
> (that is, process.py contains all of the unixy code.  If it's windows,
> then just import the relavent code from winprocess.py, which if it
> used __all__ to define what it has, you can just do `from winprocess
> import *`)
I put one global check for windows.  There are two classes. Since almost no code is shared, this is probably okay.  There could be a abstract base class if we want to discriminate according to platform.
(In reply to comment #4)
> Jeff, that's another awesome comment and I agree with most of it! Lets also CC
> Mikeal on that bug.
> 
> Lets give some short comments...
> 
> > * figure out what to do with CLI.  Make whatever needs to be
> >   consumable...consumable, but perhaps not via inheritence
> 
> Can you be a bit more specific on that part? What I really don't like is the
> usage of the option parser and using that data. We should have properties in
> the base Mozrunner class which will be set by the CLI class or function. Just
> like that:
> 
> http://github.com/whimboo/mozmill/blob/firefox-automation/scripts/firefox-automation/testrun_bft.py#L46

Now I'm confused.  I like that there is a main() function instead of a CLI class.  What I like less well is the magic setting of attributes in `run` from the command line.  Shouldn't these be passed in to the ctor?

> > * Write some tests!
> 
> +++
Well, I didn't do a very good job of this.  I'm at least as plussed on the idea though.  

> > - http://github.com/mikeal/mozrunner/tree/master/tests/ : what is the
> >   purpose of this directory?  It doesn't seem to be usable tests.  My
> >   recommendation: to wipe.  My further recommendation: write tests!
> 
> We definitely have to leave it there and write tests. That's a lot better than
> removing that folder, which never hasn't been touched again.
I put some stubs in.  I do believe the files in there before should be wiped, or if they are worth keeping around, documented.  They mean nothing to me.

> > - def makedirs:  I'm not sure if I understand the purpose of this
> >   function.  Is this the same as os.makedirs or is there some subtlty there?
> 
> I don't remember. Mikeal, can you help us?
> 
> >   - if (sys.platform == 'linux2') or (sys.platform in ('sunos5',
> >   'solaris')):
> >     silly; `if sys.platform in ('linux2', 'sunos5', 'solaris')`
> 
> Right. That was silly from me. Haven't seen that when I fixed it a while back.
You fixed already?

> > : class Runner.find_binary
> >   - move logic to where it is needed:
> > {{{
> >                 binary = findInPath(name)
> >                 if sys.platform == 'cygwin':
> >                     program_files = os.environ['PROGRAMFILES']
> >                 else:
> >                     program_files = os.environ['ProgramFiles']
> > 
> >                 if binary is None:
> >                     for bin in [(program_files, 'Mozilla Firefox',
> > 'firefox.exe'),
> >                                 ]:
> 
> That's only for Windows, right? We should get rid of that code in favor of
> using the registry (bug 554709). This bug is already targeted for the next
> Mozmill release.
K, cool.  I just punted on this, IIRC.

> > : class CLI 
> >   - I'm not even sure where to begin. I'm not sure what having this as a 
> >   class, versus a function, buys us at all; purely for downstream
> >   inheritence?  I would break this up into functions, e.g. get_parser, 
> >   etc, rather than having a class, move most of the logic into the cli
> >   function, and get rid of the unnecessary abstractions (<snip/> lots
> >   of details not worth going into here)
> 
> Having the capability to inherit from a CLI class would be great. That way none
> of the custom modules / scripts would have to create all those parser options
> again and again and only have to append their own options.
Sure, but the parser options are just a dict.  If the class is more of a front-end than just an option parser (which I'm not sure about, I haven't compared in detail to the jsbridge or mozmill to try to figure out a good pattern here), then inheritence might provide something.

Anyway, I mostly punted on this for now.

> (In reply to comment #2)
> > Next steps:  audit automation.py with respect to mozrunner and see what
> > additional functionality can/should be included
> 
> Which automation.py? Do you mean my test-run classes?

This one:
http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in

There is some commonality of intent between this automation.py and mozmill.  It'd be nice not to have redundancy.  

Which test-run classes do you mean?  These? http://github.com/whimboo/mozmill/tree/firefox-automation/scripts/firefox-automation

Haven't looked at those either.
(In reply to comment #6)
> > http://github.com/whimboo/mozmill/blob/firefox-automation/scripts/firefox-automation/testrun_bft.py#L46
> 
> Now I'm confused.  I like that there is a main() function instead of a CLI
> class.  What I like less well is the magic setting of attributes in `run` from
> the command line.  Shouldn't these be passed in to the ctor?

Can you please comment on bug 563523 why it would be better to pass it to the ctor? IMO I would bind the TestRun classes too much to any CLI implementations.

> You fixed already?

No. Wasn't that important. Doesn't block anything.

> Sure, but the parser options are just a dict.  If the class is more of a
> front-end than just an option parser (which I'm not sure about, I haven't
> compared in detail to the jsbridge or mozmill to try to figure out a good
> pattern here), then inheritence might provide something.

Using a dict prevents me from having option groups. It's not possible to add those that way. What options do we have to support it?

> Anyway, I mostly punted on this for now.

So we should file a new bug to move the CLI work over there?

> This one:
> http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in
> 
> There is some commonality of intent between this automation.py and mozmill. 
> It'd be nice not to have redundancy.  

Which ones do you mean in detail?

> Which test-run classes do you mean?  These?
> http://github.com/whimboo/mozmill/tree/firefox-automation/scripts/firefox-automation
> 
> Haven't looked at those either.

Yes, you should check those out! Sub-classing the Mozmill classes makes it possible to use those instead of the subprocess calls.
Assignee: nobody → jhammel
Status: NEW → ASSIGNED
(In reply to comment #3)
> as alice pointed out, xml.etree is new to python 2.5 (see
> http://docs.python.org/library/xml.etree.elementtree.html ).  since talos (and
> other infrastructure) uses python 2.4, she backported this to use xml.dom (see
> http://docs.python.org/library/xml.dom.html ) which is available in python 2.0
> and greater.  This should probably be done in mozmill as long as mozilla uses
> python < 2.5

Alice's xml.minidom implementation is available here: https://bug559929.bugzilla.mozilla.org/attachment.cgi?id=445217
(In reply to comment #7)
> (In reply to comment #6)
> > > http://github.com/whimboo/mozmill/blob/firefox-automation/scripts/firefox-automation/testrun_bft.py#L46
> > 
> > Now I'm confused.  I like that there is a main() function instead of a CLI
> > class.  What I like less well is the magic setting of attributes in `run` from
> > the command line.  Shouldn't these be passed in to the ctor?
> 
> Can you please comment on bug 563523 why it would be better to pass it to the
> ctor? IMO I would bind the TestRun classes too much to any CLI implementations.

Nor would I.  See comment at https://bugzilla.mozilla.org/show_bug.cgi?id=563523#c6

> > You fixed already?
> 
> No. Wasn't that important. Doesn't block anything.
> 
> > Sure, but the parser options are just a dict.  If the class is more of a
> > front-end than just an option parser (which I'm not sure about, I haven't
> > compared in detail to the jsbridge or mozmill to try to figure out a good
> > pattern here), then inheritence might provide something.
> 
> Using a dict prevents me from having option groups. It's not possible to add
> those that way. What options do we have to support it?

I suppose this is a case of too many ways to solve the problem.  A usual way (which I won't say yay or nay to for this software at this point) is to have a `def get_parser()` function and return the parser instance with the added options.  Then sub-implementations can do something like

{{{
def get_parser():
  parser =  mozrunner.get_parser()
  parser.add_option(local, options, i, need)
  ...
  return parser

def main(args=sys.argv[1:]):
  parser = get_parser()
  options, args = parser.parse_args(args)
  # do the rest of stuff
}}}

I agree fully with not tying the CLI front-end to be part of the API (that is, the CLI front-end should consume the API, never vice versa).  As far as implementation details, I don't have strong preferences.  It feels like what the mozrunner.CLI class does is too much of an abstraction, but I could be wrong.  I only audited it by itself, not the subclasses in jsbridge or mozmill (though I remember thinking the same thing about them, but I'll shut my mouth further until I have time to take a look).

> > Anyway, I mostly punted on this for now.
> 
> So we should file a new bug to move the CLI work over there?

Sure.  Or we can talk about this Wednesday.

> > This one:
> > http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in
> > 
> > There is some commonality of intent between this automation.py and mozmill. 
> > It'd be nice not to have redundancy.  
> 
> Which ones do you mean in detail?

The central purposes of automation.py and mozrunner are the same (though automation.py additionally runs a webserver).  They both:
 - handle processes
 - work with profiles
 - run and communicate with firefox

> > Which test-run classes do you mean?  These?
> > http://github.com/whimboo/mozmill/tree/firefox-automation/scripts/firefox-automation
> > 
> > Haven't looked at those either.
> 
> Yes, you should check those out! Sub-classing the Mozmill classes makes it
> possible to use those instead of the subprocess calls.

Will do.  Can you tell me more about this project and where it fits in to the mozmill.next effort? (either here, via irc, or email is fine with me)
(In reply to comment #9)
> The central purposes of automation.py and mozrunner are the same (though
> automation.py additionally runs a webserver).  They both:
>  - handle processes
>  - work with profiles
>  - run and communicate with firefox

We just have to make sure that we do not remove anything which is needed by others to run Mozmill automation outside of the build system.

> Will do.  Can you tell me more about this project and where it fits in to the
> mozmill.next effort? (either here, via irc, or email is fine with me)

Already did on IRC but lets cite it again:
https://wiki.mozilla.org/QA/Mozmill_Test_Automation/Release_Automation
(In reply to comment #10)
> (In reply to comment #9)
> > The central purposes of automation.py and mozrunner are the same (though
> > automation.py additionally runs a webserver).  They both:
> >  - handle processes
> >  - work with profiles
> >  - run and communicate with firefox
> 
> We just have to make sure that we do not remove anything which is needed by
> others to run Mozmill automation outside of the build system.

Absolutely.  I want to build a superset of the existing functionality and have it all live in one place, rather than duplicate it.  I would like to add more functionality going forward (on a need-driven basis).

> > Will do.  Can you tell me more about this project and where it fits in to the
> > mozmill.next effort? (either here, via irc, or email is fine with me)
> 
> Already did on IRC but lets cite it again:
> https://wiki.mozilla.org/QA/Mozmill_Test_Automation/Release_Automation

Thanks again!
I'm not sure what to do with this bug.  I mostly did http://github.com/k0s/mozrunner as an audit/example refactor.  Is there interest in incorporating these changes into the master?  Or should I close this bug as done?
You have put everything into your master on Github which makes it hard to review. Can you move all those changes into another branch and revert everything on your master? Clint and Heather would have to check everything. You will have to send pull requests.
(In reply to comment #12)
> I'm not sure what to do with this bug.  I mostly did
> http://github.com/k0s/mozrunner as an audit/example refactor.  Is there
> interest in incorporating these changes into the master?  Or should I close
> this bug as done?
We need to figure out what we're going to take from this and work them into piece-wise patches that are more manageable than a wholesale refactor. I've added this monster to my list to look into but it will take a couple of days to get to it.
Pinging again here.  Should we close this bug? The aforementioned branch no longer exists and most of these issues have either been fixed or ticketed separately.
(In reply to comment #15)
> Pinging again here.  Should we close this bug? The aforementioned branch no
> longer exists and most of these issues have either been fixed or ticketed
> separately.

Yes. Resolving invalid as these things are tracked elsewhere.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → INVALID
No longer blocks: 566314
You need to log in before you can comment on or make changes to this bug.