639951 - results should not take version as an argument but instead determine it

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Jeff Hammel]

Currently the TestResults class takes version as an argument:

https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/__init__.py#L61

This is supplied by the CLI controller.  Instead, the version and
maybe all of the package metadata should live outside of the CLI
class.  Making the declarable means mozmill is impossible to use
correctly as an API.

Comment 1

[Andrew Halberstadt [:ahal]]

This is where we currently get the metadata: https://github.com/mozautomation/mozmill/blob/master/mozrunner/mozrunner/runner.py#L309

It needs to live in the CLI class so we can use the --version and --info arguments.

I don't mind having the TestResults class determine the version itself, but it means duplicate code.

Comment 2

[Jeff Hammel]

We have a similar, and bigger, problem that we have to solve for e.g. jsbridge port so that we can write a better create_runner front-end than that from mozrunner (another bug).  

I think the general pattern is this:  we keep all of the defaults and data we need in multiple places at the module level.  So for this case, get_metadata_from_egg would be a module level function that would be called to put the data at module scope.

Comment 3

[Andrew Halberstadt [:ahal]]

Can you add a .lstrip() to 'value' on this line?  There is a space after the
colon which I didn't take into account:
https://github.com/mozautomation/mozmill/blob/master/mozrunner/mozrunner/runner.py#L319

Comment 4

[Jeff Hammel]

Attachment: put everything at the module level that is, well, modular

Comment 5

[Jeff Hammel]

(In reply to comment #3)
> Can you add a .lstrip() to 'value' on this line?  There is a space after the
> colon which I didn't take into account:
> https://github.com/mozautomation/mozmill/blob/master/mozrunner/mozrunner/runner.py#L319

Done; i just strip all keys and values

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 519481 [details] [diff] [review]
put everything at the module level that is, well, modular

>+package_metadata = get_metadata_from_egg('mozrunner')

I guess we can't really help but having these in the global scope.

>+        version = self.metadata.get('Version')
>+        kwargs = {}
>+        if 'Version':
>+            kwargs = dict(version="%prog " + version)

I assume this is supposed to be 'if version:'

>-        if self.options.info:
>+        if getattr(self.options, 'info', None):

Not sure what the point of this change is, the old way seems easier to read.

r+ if you address the second comment. I know what you mean when you say you aren't too happy with this patch, but I'm also not sure what else we could do (barring something more complicated) and it is better than what we currently have.

Comment 7

[Jeff Hammel]

(In reply to comment #6)
> Comment on attachment 519481 [details] [diff] [review]
> put everything at the module level that is, well, modular
> 
> >+package_metadata = get_metadata_from_egg('mozrunner')
> 
> I guess we can't really help but having these in the global scope.

Alternatively, we don't store the data at all, we just call the function when we need it and optionally stash it at the module level. ::shrug:: But we have to have it somewhere accessible to the API.

> >+        version = self.metadata.get('Version')
> >+        kwargs = {}
> >+        if 'Version':
> >+            kwargs = dict(version="%prog " + version)
> 
> I assume this is supposed to be 'if version:'

Yeah, oops :(

> 
> >-        if self.options.info:
> >+        if getattr(self.options, 'info', None):
> 
> Not sure what the point of this change is, the old way seems easier to read.

I only add the --info option in if package_metadata is defined -- if you're running uninstalled, it won't be.  While there are probably other things broken if you run uninstalled, as far as I know nothing has to be, so I'd like to move in the direction where running uninstalled works (i.e. not depending on setuptools for install).  Keeping mozmill flexible helps if we have to face situation where users can't -- or won't -- use setuptools.
 
> r+ if you address the second comment. I know what you mean when you say you
> aren't too happy with this patch, but I'm also not sure what else we could do
> (barring something more complicated) and it is better than what we currently
> have.

Indeed :/

Comment 8

[Jeff Hammel]

pushed to master: https://github.com/mozautomation/mozmill/commit/0c587931ae61577067741fa186bce19911a14f1e