566761 - Refactor the usage of properties vs. ctor arguments

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Automation, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

+++ This bug was initially created as a clone of Bug #563523 +++

As given by Jeff:

For instance, you have:
{{{
    run = BftTestRun()
    run.binaries = args
    run.logfile = options.logfile
    run.report_url = options.report
    run.run()
}}}

BftTestRun inherits from TestRun.  TestRun__init__() takes *args and **kwargs
but does not use them:
http://github.com/whimboo/mozmill/blob/firefox-automation/scripts/firefox-automation/libs/testrun.py#L53

Instead of setting having expected attributes to be set on the class, why not
pass these into the constructor? e.g.

{{{
class TestRun(object):
  def __init__(self, binaries=None, logfile=None, report_url=None):
    self.binaries = binaries or []
    self.logfile = logfile
    self.report_url = report_url
    ...
}}}

In this way, what the API is documented in the code instead of dependent upon
certain attributes being set from extrinsic sources.

==============

So what about that:

In such a case you would have to create a new instance of any of the testrun
classes if one of the options are changing.

But what I can do is to add options to the ctor and leave the properties for
later usage. Would that be a better approach?

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Jeff, would be nice to get an update to the last comment from you. Thanks.

Comment 2

[Jeff Hammel]

> As given by Jeff:
>
> For instance, you have:
> {{{
>     run = BftTestRun()
>     run.binaries = args
>     run.logfile = options.logfile
>     run.report_url = options.report
>     run.run()
> }}}
>
> BftTestRun inherits from TestRun.  TestRun__init__() takes *args and
> **kwargs
> but does not use them:
> http://github.com/whimboo/mozmill/blob/firefox-automation/scripts/firefox-automation/libs/testrun.py#L53
>
> Instead of setting having expected attributes to be set on the
> class, why not
> pass these into the constructor? e.g.
>
> {{{
> class TestRun(object):
>   def __init__(self, binaries=None, logfile=None, report_url=None):
>     self.binaries = binaries or []
>     self.logfile = logfile
>     self.report_url = report_url
>     ...
> }}}

I'll note that I made a mistake in this logic.  Should be

`self._binaries = binaries or []`

> In this way, what the API is documented in the code instead of
> dependent upon
> certain attributes being set from extrinsic sources.
>
> ==============
>
> So what about that:
>
> In such a case you would have to create a new instance of any of the
> testrun
> classes if one of the options are changing.
I'm not sure if I understand what you mean here.  Can you elaborate?

The ctor generally serves to:
 - accept arguments to setup the instance
 - setup the instance to do what it needs to do
 - serve as API for the class such that callers know what is expected
   to be passed

In python, you can of course easily set attributes on an instance.
But to have it expected that `run.binaries`, `run.logfile`, and
`run.report` can and should be set before invoking `run.run()` isn't
intuitive and must be determined from code inspection.  My main
objection is that these attributes are expected to be added from
outside the class. If these are passed as function arguments,
preferably with a docstring defining what arguments are expected and
what they do, then it is fairly well understood how `BftTestRun` works
without having to read the code.

In other words, if I am looking at testrun.py, I have no idea how
e.g. the logfile and binaries get set.  I have to introspect the code
to determine the API (i.e. setting the attributes from the invoker).

In some cases, it is impracticle or impossible to fully do the setup
in the ctor, but in this case you are merely specifying

There is no reason to specify `*args, **kwargs` since they aren't used
in TestRun.__init__

What I would do is the follows:
 - change TestRun as above (adding the options you need/want to pass)
 - BftTestRun doesn't have to be changed at all
 - UpdateTestRun.__init__ becomes
{{{
def __init__(self, binaries=None, logfile=None, report_url=None,
             channel=None, no_fallback=False):
    TestRun.__init__(binaries, logfile, report_url)
    self.channel = channel
    self.no_fallback = no_fallback
}}}

> But what I can do is to add options to the ctor and leave the
> properties for later usage. Would that be a better approach?

I'm not sure if I understand what you're proposing here.  Can you
write an example?

I'll also note that the purpose of my objection is about writing
extensible, reusable code with an API.  There is no functional
difference (to the machine) between what I propose and your original code.

====

While I would, here, just pass `binaries`, `logfile`, and `report_url`
as arguments to the ctor, I will note that there are numerous other
ways to achieve more modularity:

 - default attributes:  have the defaults of a class be assigned in a
   dictionary.  Then, the attributes on the function are set from this
   dictionary from `**kwargs`.  The parser arguments may also be set
   from these defaults, if desired (or not):
   class TestRun(object):
     defaults = { 'binaries': (),
                  'logfile': None,
                  'report_url': None }
     def __init__(self, **kwargs):
       for key in self.defaults:
         setattr(self, key, kw.get(key, self.defaults[key])

   class UpdateTestRun(TestRun):
     defaults = TestRun.defaults.copy()
     defaults.update(dict(channel=None,
                          no_fallback=False)

   [I'll post the code to introspect parser variables from defaults,
    but it's complicated and probably overkill]

 - serious introspection:  using `inspect` to get the values of
   `TestRun.__init__` arguments for the parser

I think both of these options are overkill here, but I'm happy to talk about them and give more code for them if desired.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Everything covered by this bug has been fixed by Marks patch on bug 604644.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Move of Mozmill related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.