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

RESOLVED FIXED

Status

Testing Graveyard
Mozmill
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: Jeff Hammel, Assigned: Jeff Hammel)

Tracking

Details

(Whiteboard: [mozmill-2.0+])

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
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.
(Assignee)

Updated

7 years ago
Blocks: 607111
Whiteboard: [mozmill-2.0+]
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.
(Assignee)

Comment 2

7 years ago
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.
(Assignee)

Updated

7 years ago
Assignee: nobody → jhammel
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
(Assignee)

Comment 4

7 years ago
Created attachment 519481 [details] [diff] [review]
put everything at the module level that is, well, modular
Attachment #519481 - Flags: review?(ahalberstadt)
(Assignee)

Comment 5

7 years ago
(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 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.
Attachment #519481 - Flags: review?(ahalberstadt) → review+
(Assignee)

Comment 7

7 years ago
(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 :/
(Assignee)

Comment 8

7 years ago
pushed to master: https://github.com/mozautomation/mozmill/commit/0c587931ae61577067741fa186bce19911a14f1e
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.