631945 - python callbacks refactor

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Jeff Hammel]

python callbacks should be refactored in order to have a simpler and
more flexible API.  Currently, the entirity of the python callbacks
logic lives in the core Mozmill dispatcher:

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

The current implementation has several deficiencies:

- there is no documentation python callbacks ABICT
- python callbacks aren't used at all, as best I can tell, at least in
  the primary consumer, http://hg.mozilla.org/qa/mozmill-tests
- the functionality of python callbacks is different for `mozmill` and
  `mozmill --restart`. For `mozmill`, the callback module is tied to
  the test name:
  https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/__init__.py#L195 
  and callbacks are always fired immediately.
  For `mozmill --restart`, the callbacks file is a hardcoded
  `callbacks.py` file in the same directory as the current test:
  https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/__init__.py#L427 
  and unless the additional flag, `firenow` in the event object
  dictates if the callback is fired now or at the end of the test
- python callbacks are given a single argument.  This forces python
  callable signatures to match this spec rather than allowing 
  arbitrary callables

Python callbacks should be handled in a consistent and more flexible
way.  What is to be done:

- the object passed via the 'mozmill.firePythonCallback' event will
  change signature.  Instead of having a filename based on convention,
  the filename will be passed through the event object.  Unless the
  path is absolute, the path is assumed relative to the directory of
  the current test[*]. Additionally, instead of specifying 'arg' in
  the object, two keys will be passed: 'args' and 'kwargs', which are
  assumed to be a list and a dictionary repsectively:

  obj = {'file': 'pathtofile.py', 'method': 'foo', 
         'args': ['ordered', 'args'], 
         'kwargs': {'keyword': 'arguments'}}

  The callable will then be called like `foo(*args, **kwargs)`

- the logic will be moved entirely to `python_callbacks.py` and tie
  into the pluggable event system.  This won't affect user-facing
  functionality, just allow the code to be treated as a consumer
  vs. the dispatcher for the event system and put all relavent logic
  in a single place

- as such, there should be no need for the 'firenow' vs the end test
  event.  This will be made to work with the event system
  
----

[*] If needed in the future, you could additionally pass a `module` key
alternatively to the 'filename` key, that would load the module from
its dotted path instead of by filesystem path

Comment 1

[cmtalbert]

Very good overview of where we are.  I think we ought to provide a mechanism for the tests to call back into python.  Considering that:
1. these aren't used
2. they ought to be part of the overall event system in mozmill
3. they stand in the way of merging restart vs non-restart tests

I'd say we should eliminate them completely on the mozmill 2.0 master.  We should look into a way to wire them back in so that there is *some mechanism* for JS to call back to Python.  But, until we know more about what we want those events to do, then I'm leery of adding them in or just trying to blindly move this functionality directly into 2.0.

So I propose, we drop the pythonCallbacks entirely for 2.0 right now.  We should look at a way to add them back later and prioritize it accordingly.  For example, I think for the first release of 2.0, they won't be needed.

Comment 2

[Jeff Hammel]

Attachment: refactor python callbacks

Baseline reimplementation of python callbacks.  If we don't want this, lemme know and I'll file another bug for removal of python callbacks and we can use this bug for their disparate reinclusion.  In either case, we need to do something with them before bug 584464 can be resolved

Comment 3

[cmtalbert]

Comment on attachment 510374 [details] [diff] [review]
refactor python callbacks

Good patch, this really diminishes the complexity around these things and gets them out of our way for the big restart test refactor.  Let's take this on 2.0.  I had anticipated it would be a much bigger issue to continue supporting these thigns.

Comment 4

[Jeff Hammel]

Its very much a baseline refactor.  It doesn't do the firenow vs fire later thing (all events are fire now), but its a much cleaner implementation.  And since nothing uses it currently and now we at least have a passing test for it, why not?

Comment 5

[Jeff Hammel]

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

Comment 6

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

We don't have docs at all yet on MDC. Would be great to get those added now.