Closed Bug 607111 Opened 12 years ago Closed 12 years ago

mozmill should be usable as an API

Categories

(Testing Graveyard :: Mozmill, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: k0scist, Assigned: k0scist)

References

Details

(Whiteboard: [mozmill-2.0+])

Attachments

(5 files)

As it stands, Mozmill is very difficult to use as an API.  While a fairly massive command line refactor was done as part of pluggable events, it still requires an intimate knowledge of the code to instantiate a mozmill object in the correct way.  This is because our workflow is tricky and requires a high level of arcane knowledge to get all of the parameters correct.

Rather than make a massive summary of the overall issue, I will present the various aspects in comments so that they can be individually replied to and addressed.
Adding Mark from the Thunderbird project to the CC list. I think they will have a good portion of comments to give too.
Starting at the bottom of the stack, the runner requires a profile, but the profile requires a binary to create it (if the path isn't given, as is the usual case), and the binary is found by the runner.  I've worked around this by introducing a rather strange looking workflow in a create_runner factory (http://github.com/mozautomation/mozmill/blob/master/mozrunner/mozrunner/runner.py#L224)

"""
def create_runner(profile_class, runner_class,
                  binary=None, profile_args=None, runner_args=None):
    """Get the runner object, a not-very-abstract factory"""
    profile_args = profile_args or {}
    runner_args = runner_args or {}
    binary = runner_class.get_binary(binary)
    profile = profile_class(binary=binary,
                            **profile_args)
    runner = runner_class(binary=binary,
                          profile=profile,
                          **runner_args)
    return runner 
"""

Using this pattern, the profile_args and runner_args must be passed in as they will be needed upstream (in jsbridge and mozmill).  I'm not sure if there is a better general pattern for this.
While less general, the usual case is to use FirefoxProfile, and FirefoxRunner or ThunderbirdProfile, ThunderbirdRunner.  An abstraction was written to allow the user to pass only the app (http://github.com/k0s/mozmill/blob/pluggable-events-cleanup/mozrunner/mozrunner/runner.py#L241):

"""
classes = { 'firefox': (FirefoxProfile, FirefoxRunner),
            'thunderbird': (ThunderbirdProfile, ThunderbirdRunner), }

def create_app_runner(app, binary=None, profile_args=None, runner_args=None):
    profile_class, runner_class = classes[app]
    return create_runner(profile_class, runner_class, binary,
                         profile_args, runner_args)
"""

This should suffice for the CLI classes and most API usage.
Stepping sideways, it would be nice if all application specific data was available in the same place.  Note that I do this in a primitive way in comment 3, but I'm not sure if this is good enough or not.

Looking at what we have, FirefoxProfile and ThunderbirdProfile contain only default preferences.  Since Profile should be a generic class (e.g. agnostic to mozmill), do we really want to keep these preferences in MozProfile or do they belong upstream (e.g. in mozrunner or mozmill or jsbridge)?

FirefoxRunner and ThunderbirdRunner are also data-only classes (they are just data containers, they don't have any functionality).  Again, I'm not sure what (if anything) is gained by having these be runner objects instead of some-other-kind of object.

Currently, we keep distinct classes in each of several packages to
deal with the difference between Firefox and Thunderbird.  This is bad
for a few reasons:

 - if one were to add another application, one would need to add it in
   several (undocumented) places
 - (similarly, if one wished to look at all the specializations, one
   would have to look in several places)
 - there is no way of meaningfully extending this information;  that
   is to say, if a given controller[*] wished to deal with the various
   different applications, it has to deal with each of them
   individually.  which is to say, one multiplies the amount of places
   where the various application-specific information lives!

Currently, the various bits of application-specific code live at:

* MozProfile:
  - {Firefox,Thunderbird}Profile: preferences

* MozRunner:
  - {Firefox,Thunderbird}Runner: app_name,names

Originally the profile_class lived with {Firefox,Thunderbird}Runner.  I removed this as we were using it badly (instantiating the profile and then touching the data inside of it, ignoring the API). But maybe I should put it back and just not do this.  Note that this does nothing for making it easier to construct a profile and runner pair, it just associates the information.  I'm on the fence on this one.

I think the correct solution is to have data for each application all living in the same classes. I would dissuade from putting any default preferences in -- this should live at the mozmill,jsbridge level, IMHO.
Ideally, an API would look something like the following:

"""
def my_handler(eventType, test):
  """do something here"""

from mozmill import MozMill
m = MozMill(handlers=[my_handler])
m.run()
"""

However, this neglects the runner, which is passed into mozmill.start (required) which also requires profile information.  You *could* use 

I can think of three possible solutions to this problem, one of which I've implemented:
 
 - write a function that takes the possible parameters to construct mozmill + a runner.  This was straight-forward, so this is what I did to demo: http://github.com/k0s/mozmill/blob/pluggable-events-cleanup/mozmill/mozmill/__init__.py#L484  I'll talk about this more subsequently.  The important thing to know at the high-level is that it is dependent on knowing what arguments need to be passed to the profile and the runner.  In other words, this is just a convenience function for those that don't have profile_args and runner_args they need memorized.  This could be added as a class method to MozMill such that it is more closely associated with what it makes (in the usual python style)

 - write a formal factory class.  Like the cascading CLI class, the mozrunner will just create the runner, jsbridge's controller will add needed parts, etc.  Maybe this is overkill?  It won't really give anything over the above function, but possibly easier to extend and understand.  It kinda bothers me that this would be necessary, but...for reasons described above, maybe it is?

 - somehow get MozMill to intelligently take/construct the runner from its arguments.  this would be the best and best match the example above, but for the like of me I don't know how to do this and make it robust, legible, general-purpose, and maintainable.  

I'm leaning towards the @classmethod constructor, but if I had much confidence, I probably wouldn't be writing this bug ;)
The real goal of this bug is to make mozmill-automation (http://hg.mozilla.org/qa/mozmill-automation) more extensible, easier to read, and easier to maintain.
What purpose is the else clause in mozmill.CLI.run(): http://github.com/k0s/mozmill/blob/pluggable-events-cleanup/mozmill/mozmill/__init__.py#L599 ?  Is this needed/useful for something in production?  Can we special case this or just take it out?
The current workflow for mozmill is

mozmill = MozMill(<args>)
mozmill.start(runner)
mozmill.run(test)

Is there any reason that it couldn't be

mozmill = MozMill(<args>, runner)
mozmill.run() # run now calls start

If we need the legacy case, as for comment 7, we could just special case it, e.g.:

mozmill = MozMill(<args>, runner)
mozmill.start()
# do whatever, man
So I'm not sure I've had enough coffee to digest all of this yet, whilst the suggestions for simplifying the API seem good, I think you're largely talking about wrapper functions?

You may want to take a look at what we currently use in our test harness for Thunderbird's MozMill tests. It is based against 1.4.2 as we've not updated to the most recent yet:

http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/runtest.py

Some highlights:

- We have a stack of default prefs that are specific to our MozMill tests
- create_new_profile override is aimed at getting the profile in a consistent location, followed by an override not to delete the profile. This is very useful for developers so we don't have to go hunting for it if we're trying to resolve an issue and need to look in the profile.
- The VNC server stuff is mainly for Linux and I'm told it makes the tests more stable to run.
- We do post-processing of results and print them in a nice format for Tinderbox/buildbot to parse and use.
- Likewise we also add in crash processing.

I'm not saying that we need to include all of these in core MozMill, but we do need to keep the ability to do these and I thought it'd be a good example of how we extend MozMill currently to suit our needs.
fix for making a factory create method on mozmill and associated cleanup to allow more APIish functionality . This doesn't really address all the issues, but is one possible first-pass fix.  Alternatively, a formal controller could be made.  In any case, suggestions welcome, as I'm only kinda happy with it (its better, but is it better enough?)
Attachment #486100 - Flags: review?(fayearthur+bugs)
(In reply to comment #10)
> Created attachment 486100 [details] [diff] [review]
> first-round fix for making API-ish
> 
> fix for making a factory create method on mozmill and associated cleanup to
> allow more APIish functionality . This doesn't really address all the issues,
> but is one possible first-pass fix.  Alternatively, a formal controller could
> be made.  In any case, suggestions welcome, as I'm only kinda happy with it
> (its better, but is it better enough?)
Yes, it's better, but it's not better enough.  I think that the class method is still too much complexity for really no gain.  All of that nice defaulting stuff can be placed in the initialization method of Mozmill, and we can place a main function in the mozmill.py (currently __init__.py, but that will change) which does exactly what your test does to start mozmill.  Everything that interacts with Mozmill would do so through well-defined interfaces, and likewise, Mozmill would interact with its other components through similar interfaces.  This way both things that wrap Mozmill and Mozmill itself would call those interfaces the same way.  We would converge onto one code path and simplify the rats nest of this code.

This way, we can also remove all "start" logic from the CLI method and make it a simple class that extends optparse's parser.  Then each thing that needs a CLI could extend the "mozmill CLI" as well.  And I really don't think that jsbridge needs its own CLI at all.  I think all that can be removed, further simplifying things, and jsbridge can become an object that mozmill instantiates and uses.

I think we're going to have to do this on a whiteboard and post some pics to the bug.  

Essentially, I see a set of very simple objects that are basic atomic python module building blocks that Mozmill uses:
* CLI
* Runner
* Handlers
Then there is a complex object that Mozmill uses, but which really doesn't need any of these to function, and that's jsbridge.

Runner is a controller object which exposes the profile for manipulation.  It has its own CLI can can be called independently. It uses:
* Profile (Types of default profiles are derived from here) 
* Info
* Process

If we keep the interactions relegated to well-defined interfaces on these objects then I really don't see why this needs to be all that complicated.  I think we're letting the rats nest of the current codebase cloud our thinking about what we're trying to achieve.  At the very basic level all the python Mozmill harness does is this:
1. It sets up the required state of the profile and the application command line.
2. It then launches the application with the jsbridge command line parameter.
3. It listens for events coming back from the jsbridge
4. It shuts down the application and outputs any results it obtained from jsbridge.
(In reply to comment #9)
> So I'm not sure I've had enough coffee to digest all of this yet, whilst the
> suggestions for simplifying the API seem good, I think you're largely talking
> about wrapper functions?

Wrapper functions or otherwise, I'm talking about ways to make mozmill easier to run programmatically.  Traditionally, there were/are many setup steps whose magical necessity was hidden and basically only going through the CLI was a robust way of invoking.  However, the CLI as stands is a poor controller.

> You may want to take a look at what we currently use in our test harness for
> Thunderbird's MozMill tests. It is based against 1.4.2 as we've not updated to
> the most recent yet:
> 
> http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/runtest.py

Thanks.

> Some highlights:
> 
> - We have a stack of default prefs that are specific to our MozMill tests

Yeah, I'd like to make this easier.

> - create_new_profile override is aimed at getting the profile in a consistent
> location, followed by an override not to delete the profile. This is very
> useful for developers so we don't have to go hunting for it if we're trying to
> resolve an issue and need to look in the profile.

I wrote a bug about this, bug 579929 ; I closed for lack of interest and consensus, but am reopening as per your comment.

> - The VNC server stuff is mainly for Linux and I'm told it makes the tests more
> stable to run.
> - We do post-processing of results and print them in a nice format for
> Tinderbox/buildbot to parse and use.
> - Likewise we also add in crash processing.

For 2.0, these should be pluggable events listeners.  Parsing output should not be MozMill's job nor should one have to override to do these. (I would argue that even accumulating results is not MozMill's job, but that's a further off consideration....it is MozMill's job for now).
 
> I'm not saying that we need to include all of these in core MozMill, but we do
> need to keep the ability to do these and I thought it'd be a good example of
> how we extend MozMill currently to suit our needs.
Assignee: nobody → jhammel
Whiteboard: [mozmill-2.0?] → [mozmill-2.0+]
Depends on: 639951
Attached file api case
This is currently the shortest way to use Mozmill as an API.  Several deficiencies:

* having to pass a version into test results: bug 639951
* the complication + magic of setting up runner and profiles; as suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=639951#c2 , what needs to be done is having all interface-agnostic defaults live at the module scope and then the CLI class can use them as argument defaults.  there should be a mozmill.create_runner analagous to to the mozrunner.create_runner that uses the arguments necessary for mozmill

These are the top two.  If we get these done for 2.0, we're probably in pretty good shape to close this bug and ticket subsequent desires separated as guided by e.g. mozmill-tests
Comment on attachment 517883 [details]
api case

changing mimetypes, silly silly Firefox
Attachment #517883 - Attachment mime type: text/x-python → text/plain
Depends on: 641956
So I'm doing this a bit incrementally to give some chance for feedback as well as trying to hammer things into a decent shape.  There's a lot of cruft and not much control flow in e.g. mozrunner and mozmill wrt this bug that I'd ultimately like to go away.  Probably most if not all will go away with this bug, but my primary goal is to make is easier and more transparent for the end-user so we'll see what this turns into.

You can see my WIP at https://github.com/k0s/mozmill/tree/api . Currently, I'm introducing a create_runner function parallel to that in mozrunner in mozmill's __init__ module.  And I've made MozMill.run() call MozMill.stop() and return the TestResults instance.  So you should be able to do something like:

import mozmill
runner = mozmill.create_runner() # app = 'firefox by default
m = mozmill.MozMill(runner=runner) # note: no handlers; you supply these
results = m.run([{'path': 'path/to/my/test'}])
results.stop(my_handlers)

Its certainly not ideal but, eh, five lines...not horrible.  

I'm going to go through and see what of the underlying infrastructure is needed or wanted at the mozrunner and mozmill level.  I have a feeling we depend on things we don't actually need to depend on anymore.

Ideally, you could just generate a MozMill instance directly and not have to create the runner yourself:

m = mozmill.MozMill(...)
results = m.run(...)

For specific cases you might need to do more footwork, but... In any case this isn't possible yet.  And we don't want to throw away the abilities to customize runner/profile creation we already have (though we also don't have to make it any more custom): the API should provide all the ways to do this and the CLI should just be (mostly) a front-end to the API.  We might want/need a factory class/function...we might not.  Anyway, just wanted to ping the bug and give people a chance to weigh in on what will become a future vital aspect of MozMill python-side.
(In reply to comment #15)

> You can see my WIP at https://github.com/k0s/mozmill/tree/api . Currently, I'm
> introducing a create_runner function parallel to that in mozrunner in mozmill's
> __init__ module.  And I've made MozMill.run() call MozMill.stop() and return
> the TestResults instance.  So you should be able to do something like:
> 
> import mozmill
> runner = mozmill.create_runner() # app = 'firefox by default
> m = mozmill.MozMill(runner=runner) # note: no handlers; you supply these
> results = m.run([{'path': 'path/to/my/test'}])
> results.stop(my_handlers)
I think this looks like a clear improvement over the 1.5.x solution, so I'm happy to take this for 2.0.  And it is an easy upgrade to the way that people have used mozmill in the past, so I think it's a fine way forward.

> I'm going to go through and see what of the underlying infrastructure is needed
> or wanted at the mozrunner and mozmill level.  I have a feeling we depend on
> things we don't actually need to depend on anymore.
Good.
> 
> Ideally, you could just generate a MozMill instance directly and not have to
> create the runner yourself:
> 
> m = mozmill.MozMill(...)
> results = m.run(...)
> 
Yeah, I'd be interested to see how much refactoring is needed to support case 1 above versus this ideal case.  Then we can decide if we can take it for 2.0.
test case included
Attachment #527875 - Flags: feedback?(ctalbert)
So this is a much better API than what we currently have....which is essentially none.  I've gotten rid of the free-standing pseudo-factory functions and made real factory methods on the relevent classes. I've taken defaults out of the CLI and put them at module level, trying to avoid as much repeat of code as possible. And I've associated a Profile class with a runner class so that selecting e.g. mozrunner.runners['firefox'] will give you a clean association with a single string.  

A few other things of note:

* you *can* go completely insane and have complete control with this model.  Nothing is lost and quite a bit is gained.  Doing easier things is easier though.

* there is probably more that can be done, but this is a pretty good start, such as....

* there is a weirdiosity, let's say, for the results class.  Results is a class as 1. there is no reason that the MozMill event-dispatcher/test-runner class should particularly process results; and 2. if you want to carry results across runs (i.e. different calls to MozMill.run()), which may be important if your runner/profile/etc instance(s) changes.  IMHO, 2. is pretty important, but it does carry the horrible artefact that you have to pass the handlers you want to the results.finish() method. I'm open to ideas about how to make this smoother

* there could be a mozmill create_runner method, which is how i was originally doing it.  There would be minimal change in code.  But in the spirit of possible YAGNI, I didn't do it that way as I'd rather do it as response to a feature need
Attachment #527875 - Flags: feedback?(ctalbert) → review?(ctalbert)
Comment on attachment 527875 [details] [diff] [review]
like a real API now. probably bitrotted

Review of attachment 527875 [details] [diff] [review]:

Overall, this looks really great.  I just have a couple of nits.  Thanks, Jeff.

r=ctalbert

::: mozmill/mozmill/__init__.py
@@ +62,5 @@
+# defaults
+addons = [extension_path, jsbridge.extension_path]
+jsbridge_port = 24242
+jsbridge_timeout = 60. # timeout for jsbridge
+

nit: I'd prefer global constants like this to be in UPPER_CASE.

::: mozmill/mozmill/logger.py
@@ -187,5 @@
       levelname_color = self.COLOR_SEQ % fore_color + levelname + self.RESET_SEQ
       record.levelname = levelname_color
     return logging.Formatter.format(self, record)
-
-    

Ok, I think this is just a whitespace change in the logger.py file?  On the raw diff, it doesn't look like you removed the class ColorFormatter line (but it does on the splinter diff)

::: mozmill/test/test_api.py
@@ +1,4 @@
+#!/usr/bin/env python
+
+"""
+illustrate use of mozmill as an API

I think this test just needs to be in python's unittest format, but otherwise it is very nice and very illustrative.
Attachment #527875 - Flags: review?(ctalbert) → review+
::: mozmill/mozmill/logger.py
@@ -187,5 @@
       levelname_color = self.COLOR_SEQ % fore_color + levelname +
self.RESET_SEQ
       record.levelname = levelname_color
     return logging.Formatter.format(self, record)
-
-    

> Ok, I think this is just a whitespace change in the logger.py file?  On the raw
> diff, it doesn't look like you removed the class ColorFormatter line (but it
does on the splinter diff)

Yeah, just a whitespace change.
(In reply to comment #20)
> ::: mozmill/mozmill/logger.py
> @@ -187,5 @@
>        levelname_color = self.COLOR_SEQ % fore_color + levelname +
> self.RESET_SEQ
>        record.levelname = levelname_color
>      return logging.Formatter.format(self, record)
> -
> -    
> 
> > Ok, I think this is just a whitespace change in the logger.py file?  On the raw
> > diff, it doesn't look like you removed the class ColorFormatter line (but it
> does on the splinter diff)
> 
> Yeah, just a whitespace change.

In fact, I won't include in the final patch since it has nothing to do with this bug and i have to unbitrot anyway
So another thing before i push finish up this patch....

The example code that shows how to use the API, inaptly named test_api.py since we don't have examples, only tests, needs access to some test -- any, really, as long as it doesn't touch a server besides localhost.  This can be be rewritten as a unittest, but probably the example code is more useful as explanation without having to understand what == test harness and what == API.

In either case, it will need a test.  These are now moved so now python tests do not live with JS tests. This makes my job hard.  Should I just code some joke test directly into the python? Should I make an example directory?  Should I...?
I think what I'll do, unless there are any objections, is put the example in the mozmill/mozmill directory (sibling to setup.py) and the unittest in the appropriate directory and have them both just write out strings to test files in both of these that live directly in the .py file. Kinda hacky, but in the example case its a good illustration (you *can* make dynamic tests....I don't know if you ever *should*) and in the test case it doesn't matter
Attached patch unbitrot patchSplinter Review
unbitrot of reviewed attachment 527875 [details] [diff] [review]. I'm currently failing on this line: 

results['addons'] = json.loads(mozmill.addons)

from https://github.com/mozautomation/mozmill/commit/4d11cd2f238a19b050b04540d8767064b2c2166b

Not sure what's going on here :(
Are you having the latest changeset? That was work done by Heather on bug 636773. I wasn't able yet to verify its behavior. If it is broken for you, we have to reopen the bug again.
pushed to master: https://github.com/mozautomation/mozmill/commit/4c9a2ce09be8b913a22d967e173c08070dbaa952 

I did not push the example or test yet because of the problem in comment 24. This is a problem before and after this change.  Once that's sorted out I'll add the test and example.  I'll leave this bug open until then
(In reply to comment #25)
> Are you having the latest changeset? That was work done by Heather on bug
> 636773. I wasn't able yet to verify its behavior. If it is broken for you, we
> have to reopen the bug again.

Turns out its just a red herring.  One addon had a pariticular ASCII charset name revealing bug 652952 . So unrelated either to this or reporting addons (except that addons can have non-unicode names)
Attachment #486100 - Flags: review?(fayearthur+bugs)
the python test may be redundant with usemozmill.py; also, i took out logging to foo.txt there as this will leave this file. instead, use a tempfile
Attachment #533436 - Flags: review?(fayearthur+bugs)
Comment on attachment 533436 [details] [diff] [review]
test and example for api usage

Review of attachment 533436 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine.

::: mozmill/api_example.py
@@ +26,5 @@
> +# let's try the logging handler:
> +from mozmill.logger import LoggerListener
> +logger = LoggerListener()
> +m = mozmill.MozMill.create(results=results, handlers=(logger,))
> +results = m.run(dict(path=path))

This API seems pretty verbose
Attachment #533436 - Flags: review?(fayearthur+bugs) → review+
(In reply to comment #29)
> Comment on attachment 533436 [details] [diff] [review] [review]
> test and example for api usage
> 
> Review of attachment 533436 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> This looks fine.

Pushed to master: https://github.com/mozautomation/mozmill/commit/0617797c7b77b7b8763ba652a1f27c8c4a0f0ff7

> 
> ::: mozmill/api_example.py
> @@ +26,5 @@
> > +# let's try the logging handler:
> > +from mozmill.logger import LoggerListener
> > +logger = LoggerListener()
> > +m = mozmill.MozMill.create(results=results, handlers=(logger,))
> > +results = m.run(dict(path=path))
> 
> This API seems pretty verbose

I can't tell....Is that a good or bad thing?
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.