586336 - Update TestRun class to allow modifying report data

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Automation, defect)

Description

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

With bug 562822 fixed we have a rich set of meta data for each kind of mozmill test-run. This bug should cover the basic work for the TestRun class to allow derived classes to also modify those data before it get sent to brasstacks.

Comment 1

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

Attachment: Patch v1

This patch adds the possibility to modify the report data before it gets send to the report server. Each kind of test-run can specify it's report_type which is needed for the dashboard to separate the different results. We have added 'firefox-' as prefix to all of the existing classes, because that way we are open for other applications too, i.e. Thunderbird.

Comment 2

[Jeff Hammel]

Comment on attachment 464873 [details] [diff] [review]
Patch v1

>+class MozmillRestartWrapper(mozmill.MozMillRestart, MozmillWrapper):
>+    """ Wrapper class for the MozMillRestart class. """
>+
>+    def __init__(self, *args, **kwargs):
>+        mozmill.MozMillRestart.__init__(self, *args, **kwargs)
>+        MozmillWrapper.__init__(self, *args, **kwargs)
>+
>+

This is fairly odd logic and one of the reasons I don't like using *args and **kwargs as they will prove difficult for multiple inheritence.  MozmillRestart is __init__ed with *args and kwargs.  Then MozmillWrapper is as well, which will init MozMill with *args and **kwargs.  I'm not sure if you'll get conflicts at the moment doing this, but conflicts are certainly possible.  Since the ctor of MozmillWrapper only does one thing (set report_callback) in its ctor, I would repeat that logic here and not bother calling the __init__.  In general, passing *args and **kwargs makes it confusing to figure out what is actually desired behaviour.  I understand why you do it here -- because the Wrapper classes should be pass-throughs -- but still it makes it very hard to determine what is supposed to happen.


> 
>+        self.report_callback = None
>+
>     def _get_addon_list(self):
>         """ Returns the location of add-ons which will be installed. """
>         return self.addons
> 
>     def _set_addon_list(self, value):
>         """ Sets the location of add-ons which will be installed. """
>         self.addons = value
> 
>@@ -198,19 +228,23 @@ class MozmillWrapperCLI(mozmill.CLI):
>     def _set_use_code(self, value):
>         """ Sets if the code module should be used. """
>         self.options.usecode = value
> 
>     use_code = property(_get_use_code, _set_use_code, None)
> 
>     def run(self, *args, **kwargs):
>         """ Start the test-run. """
>-        mozmill.CLI._run(self, *args, **kwargs)
>+        self.mozmill.report_callback = self.report_callback

Then you set it here again, in addition to the places above.

>+        mozmill.CLI.run(self, *args, **kwargs)
> 
> 
> class MozmillWrapperRestartCLI(mozmill.RestartCLI, MozmillWrapperCLI):
>     """ Wrapper class for the Mozmill RestartCLI class. """
> 
>+    mozmill_class = MozmillRestartWrapper
>+
>     parser_options = copy.copy(MozmillWrapperCLI.parser_options)
> 
>     def run(self, *args, **kwargs):
>         """ Starts the test-run. """
>-        mozmill.RestartCLI._run(self, *args, **kwargs)
>+        self.mozmill.report_callback = self.report_callback

And again.

>

If this patch makes things easier for now, then its okay to push.  Since the classes here depend very deeply on not just the API of Mozmill but also the internals being kept in a static state, this method of wrapping will be brittle. There are several places where this will break going forward.  I hope moving -> mozmill 2.0 allows some of this to be architected in a more robust manner.

Comment 3

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

I'm aware of those items and I'm looking forward to simplify those constructors too. But I will have to wait for your refactoring of the Mozmill classes. Once this is done I hope that I will not need the wrapper class at all.  Also bug 566761 will cover the refactoring of the parameters in the constructors. I will hopefully come to it in the next couple of weeks.

Comment 4

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

Landed as:
http://hg.mozilla.org/qa/mozmill-automation/rev/72df907b620c

Comment 5

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

Sorry, forgot to remove the temporary repository location before pushing the patch. Landed the follow-up to restore the original location:

http://hg.mozilla.org/qa/mozmill-automation/rev/6d65a6b7c2ef

Comment 6

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

Move of Mozmill related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.