709773 - Move around some code of Mozmill 1.5 to make its API better usable

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

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

While trying to figure out how to get rid of our own MozmillWrapper Python class in the automation scripts to prepare everything for Mozmill 2, I have seen a lot of improvements since the beginnings of the project. But in some cases it's not possible to use the available API. I would propose some small changes to the code which would offer a way better API support. For now I don't see a situation where the API would have to be changed, it's all about moving code around.

I will come up with a proposal patch. Jeff or Clint, I would leave the decision of taking it up to you.

Comment 1

[Jeff Hammel]

I would say the API update == mozmill 2.0

Comment 2

[cmtalbert]

Henrik,

what is the purpose of this bug?  I don't understand what you're talking about in comment 0.

Comment 3

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

As said in comment 0 some code has been misplaced and needs to be moved around so that consumers of the API can correctly set properties. Given that most of the initialization happens in the init method, there is no way to override inject other values.

Again, this will not change the API of Mozmill 1.5, only makes it usable for consumers. Right now I'm not sure if we can solve all cases. But at least the most important ones for us should be doable, so a transition to Mozmill 2 will be easier.

Comment 4

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

Chatted with Clint lately and he now understands my intention. Would be good to get this shuffled into a Mozmill 1.5.9 release.

Comment 5

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

Attachment: Testcase

Comment 6

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

Attachment: Test script

Comment 7

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

Ok, so lets update the bug because David probably wants to have a look at it. Here is what I have found so far:

So if you want to make use of Mozmill via an API you should work with the Mozmill class:

https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozmill/mozmill/__init__.py#L112

That really doesn't work for us because there is code in MozmillCLI which is necessary for us without having to implement it again. Means some options passed via optparse are not available in the Mozmill class.

While doing this we hit another problem. Because our script should not support e.g. the -t option it has been removed in the example script above. Sadly this causes a failure in the CLI.__init__ method:

https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozmill/mozmill/__init__.py#L751

Because we have to call this method as the first one in our subclass there is no way for us to update the self.options.test property with our custom code. IMO preparing the test files should be moved into the run() method.

The same probably also applies to all the other code in CLI.__init__ which makes use of 'options'. 

That was just the first code I have tested, so using other options as done in our scripts probably reveals other issues in Mozmill, Mozrunner, or JSBridge. 

At the end and as already mentioned before we really should only move code around but not make any changes to the API.

Something I also have noticed is the call of sys.exit() in two cases:
https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozmill/mozmill/__init__.py#L821
https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozmill/mozmill/__init__.py#L527

This is kinda bad for an API. I'm sure we want to raise specific exceptions for it.

Comment 8

[David Guo [:davidg]]

After a decent inspection, it does look like the test argument check prevents some use cases of mozmill 1.5.

I agree with Henrik that low impact moves can make the API more usable. In particular, these lines should be moved from __init__() to run():
https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozmill/mozmill/__init__.py#L744-759 .

The rest of the arguments in mozmill,jsbridge and mozrunner can be ignored since they either have default values or are not used in the init.

As for the call of sys.exit, maybe the intention is to suppress the traceback?

Comment 9

[cmtalbert]

(In reply to David Guo [:davidg] from comment #8)
> After a decent inspection, it does look like the test argument check
> prevents some use cases of mozmill 1.5.
> 
> I agree with Henrik that low impact moves can make the API more usable. In
> particular, these lines should be moved from __init__() to run():
> https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozmill/mozmill/
> __init__.py#L744-759 .
> 
You guys are the last folks using the 1.5.x API as far as I know. So, you're free to move stuff around as you see fit. Happy to review patches.

> The rest of the arguments in mozmill,jsbridge and mozrunner can be ignored
> since they either have default values or are not used in the init.
> 
> As for the call of sys.exit, maybe the intention is to suppress the
> traceback?
Hahaha, First rule of Mozmill on the 1.5.x branch - do not assume that there is a design.
If you think there is a design...please see the first rule of mozmill.

Actually I think the sys.exit there in both those cases is because those are error states where Mozmill can hang indefinitely. You're probably already running inside a try/catch (from mozrunner) and if you remove the sys.exit then you may end up with a silent failure and mozrunner may continue on as though nothing happened.

The entire stopping sequence of mozmill on the 1.5.x branch is fraught with problems. I would abandon the sinking ship and move to 2.0.x. One of the major re-designs we did on 2.0.x was to address this very problem.

I'd say fix as much as you need to get your wrappers into a state where you can move them to 2.0, then spend your energy moving them to 2.0. If you start fixing all the bugs in 1.5.x, you'll be here a while :)

Let me know if you need advice/reviews.

Comment 10

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

David, do you have an update for us here? I expected to see a patch and a check-in if changes I have proposed earlier will help us to make our libs easier to use. 

Clint, the Thunderbird guys are already using Mozmill 2.0? Are you sure? I haven't heard about such a change.

Also there is still a sys.exit() on master, but that's clearly a different bug:
https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/__init__.py#L671

Comment 11

[Jeff Hammel]

(In reply to Henrik Skupin (:whimboo) [away 02/17 - 02/26] from comment #10)
> Also there is still a sys.exit() on master, but that's clearly a different
> bug:
> https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/
> __init__.py#L671

This is by design -- the CLI program should exit non-zero when tests fail or an error is encountered. Not sure what the bug would be

Comment 12

[David Guo [:davidg]]

Henrik, apologies for not being explicit earlier. I intended to say that it is possible for the removal of the wrapper once the changes in Mozmill 1.5 land. During my investigation of both the wrapper and mozmill 1.5 code, I only did find the one blocker. Essentially, comment #8 is a summary of the important bits. 

My investigation consisted of going through the arguments and seeing that none of them are executed in the inits of the mozmill code. As long as this is the case, you can pop/override them.

For what its worth, the blocks of code that exhibit the behavior:
- https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozmill/mozmill/__init__.py#L744-759
- https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozrunner/mozrunner/__init__.py#L602-604

options.manifest should not present an issue as it will skip execution if not set
options.info should also not present an issue as it defaults to false
options.test would be the only thing worth moving.

Additionally, I think that the removal of the wrapper and Mozmill 1.5 changes should be done in parallel.

Comment 13

[David Guo [:davidg]]

Attachment: Pointer to Github pull request: https://github.com/mozautomation/mozmill/pull/11#

Pointer to Github pull-request

Comment 14

[David Guo [:davidg]]

Attachment: Pointer to Github pull request: https://github.com/mozautomation/mozmill/pull/11#

Pointer to Github pull-request

Comment 15

[cmtalbert]

Comment on attachment 602043 [details]
Pointer to Github pull request: https://github.com/mozautomation/mozmill/pull/11#

reviewed in pull request

Comment 16

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

Fixed by merge of pull request:
https://github.com/mozautomation/mozmill/pull/11

Comment 17

[David Guo [:davidg]]

Attachment: Pointer to Github pull request: https://github.com/mozautomation/mozmill/pull/23

Pointer to Github pull-request

Comment 18

[David Guo [:davidg]]

Right now, if an invalid test file is specified, the application will not close. The new pull fixes this bug.

Comment 19

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

Attachment: Pointer to Github pull request: https://github.com/mozautomation/mozmill/pull/23

Pointer to Github pull-request

Comment 20

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

Before we want to release another version of Mozmill 1.5.x we have to make sure to deeply test it also with the logging code and manifest files. Both are set in __init__ too and cannot be overridden by our code.

Comment 21

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

Putting on the list of bugs for the 1.5.11 release.

Comment 22

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

Landed on hotfix-1.5:
https://github.com/mozautomation/mozmill/commit/1027c0b2c0723a2232a41bce15b521190b01f356

Comment 23

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

Comment on attachment 615581 [details]
Pointer to Github pull request: https://github.com/mozautomation/mozmill/pull/23

Clearing outstanding review request.