Closed Bug 925398 Opened 6 years ago Closed 6 years ago

Refactor runtests.py to follow Mixin pattern

Categories

(Testing :: Marionette, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla29

People

(Reporter: mdas, Assigned: mdas)

References

Details

Attachments

(3 files, 10 obsolete files)

94 bytes, text/plain
davehunt
: review+
Details
194.67 KB, patch
mdas
: review+
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
davehunt
: review+
Details | Review
From concerns raised in https://bugzilla.mozilla.org/show_bug.cgi?id=914983#c10, runtests.py is being extended in gaia-ui-tests by subclassing, and we've seen people wanting to add new features that aren't related to gaia-ui-tests or vanilla marionette, like endurance based options, and their only option is to subclass stuff from any of these runtests.py in their own runtests.py. It can be repetitive and cumbersome. 

I think a nicer plan would be to have a base runtests.py that can be extended with Mixins, so you can mix whatever you need together to get the runner you want, ie: add BaseMixin + EnduranceMixin to get marionette endurance tests, or BaseMixin + EnduranceMixin + GaiaUIMixin to get gaia-ui-test endurance tests.
Taking this. Anyone who likes to do this can take this from me also.
Assignee: nobody → wachen
Assignee: wachen → mdas
taking this for work on Bug 914983
This patch pulls out basic shared classes of runtests.py into runner/base.py. I also took the endurance and html reporting bits from gaiatest and put them into runner/mixins so they can be reused easily. You can see how they're being used in testing/marionette/client/marionette/runtests.py, and https://github.com/malini/gaia/blob/mixin/tests/python/gaia-ui-tests/gaiatest/runtests.py

I have the related gaia-ui-tests changes here: https://github.com/malini/gaia/compare/mixin. This separates out parts of gaiatest so you can reuse them as mixins, and updates runtests.py to use this mixin pattern. More commits are needed here, since there's some cleanup and a bit of reorganization to do. Davehunt, do you have any comments on these changes?
Attachment #832310 - Flags: review?(jgriffin)
Attachment #832310 - Flags: feedback?(dave.hunt)
had to add a small fix to html reporting.

When the mtbf runtests.py and other bits land in gaia, I may have to rejigger that code to work with this model
Attachment #832310 - Attachment is obsolete: true
Attachment #832310 - Flags: review?(jgriffin)
Attachment #832310 - Flags: feedback?(dave.hunt)
Attachment #832533 - Flags: review?(jgriffin)
Attachment #832533 - Flags: feedback?(dave.hunt)
jgriffin mentioned https://bugzilla.mozilla.org/show_bug.cgi?id=915672 as another mixin related patch that I'll need to look into once it lands.
I'm demoting this to an f? since I realized that some bits of html reporting should be in these changes. The new classes aren't quite Mixins since they are overriding what MarionetteTestResult does (add_test). I hope to change MarionetteTestResult to be a bit more flexible in how it handles its resultclass so you can add multiple. Here's the current patch. I'll make it a bit more robust soon.
Attachment #832533 - Attachment is obsolete: true
Attachment #832533 - Flags: review?(jgriffin)
Attachment #832533 - Flags: feedback?(dave.hunt)
Attachment #833184 - Flags: feedback?(jgriffin)
Attachment #833184 - Flags: feedback?(dave.hunt)
Comment on attachment 833184 [details] [diff] [review]
separate out reusable bits of marionette and gaiatest runtests.py into mixins

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

Cool, I like this approach; it will allow us to more easily separate out the B2G-specific stuff in the future, too.

We'll have to carefully coordinate a new pypi release with changes to gaiatest.

We should definitely run it on try first, though.

::: testing/marionette/client/marionette/runner/__init__.py
@@ +1,2 @@
> +from base import *
> +from mixins import *

Need MPL header in this file.

::: testing/marionette/client/marionette/runner/base.py
@@ +479,5 @@
> +        return (options, tests)
> +
> +class BaseMarionetteTestRunner(object):
> +
> +    textrunnerclass = MarionetteTextTestRunner # this line will have to defined by the amalgamated class 

nit: extra whitespace at EOL

What does this comment mean, precisely?

::: testing/marionette/client/marionette/runner/mixins/__init__.py
@@ +1,2 @@
> +from endurance import *
> +from reporting import *

Need MPL header in this file.

::: testing/marionette/client/marionette/runner/mixins/endurance.py
@@ +1,4 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +import os

nit: Should have a blank line after the MPL header block.

@@ +11,5 @@
> +        if options.iterations is not None:
> +            if options.checkpoint_interval is None or options.checkpoint_interval > options.iterations:
> +                options.checkpoint_interval = options.iterations
> +
> +    # verify_usage 

nit: extra whitespace at EOL

@@ +39,5 @@
> +        self.parse_args_handlers.append(self.endurance_parse_args)
> +        self.verify_usage_handlers.append(self.endurance_verify_usage)
> +
> +class EnduranceTestCaseMixin(object):
> +    def __init__(self, *args, **kwargs): 

nit: extra whitespace at EOL

::: testing/marionette/client/marionette/runner/mixins/reporting.py
@@ +177,5 @@
> +                         dest='html_output',
> +                         help='html output',
> +                         metavar='path')
> +
> +class HTMLReportingResult(TestResult):

nit: should be 2 blank lines between class definitions

::: testing/marionette/client/marionette/runner/mixins/resources/htmlreport/main.js
@@ +1,1 @@
> +$(document).ready(function() {

We should add a standard MPL header to this file.

::: testing/marionette/client/marionette/runner/mixins/resources/htmlreport/style.css
@@ +1,1 @@
> +body {

We should add a standard MPL header to this file.

::: testing/marionette/client/setup.py
@@ +15,4 @@
>          'mozprocess >= 0.9', 'mozrunner >= 5.15',
>          'mozdevice >= 0.22', 'moznetwork >= 0.21',
>          'mozcrash >= 0.5', 'mozprofile >= 0.7',
> +        'moztest >= 0.1', 'py==1.4.14']

Generally, our policy is that we shouldn't add external dependencies to in-tree packages, unless we add the external package as well (to $topsrcdir/python).

I don't think this will immediately break anything, but it would become problematic if we ever wanted to expose HTML reporting via a mach target, for instance.  For now, I think we should file a separate bug about it, in which we can decide either to rewrite the HTML reporting to use Python's built-in minidom, or import py to the tree.
Attachment #833184 - Flags: feedback?(jgriffin) → feedback+
(In reply to Jonathan Griffin (:jgriffin) from comment #8)
> ::: testing/marionette/client/marionette/runner/base.py
> @@ +479,5 @@
> > +        return (options, tests)
> > +
> > +class BaseMarionetteTestRunner(object):
> > +
> > +    textrunnerclass = MarionetteTextTestRunner # this line will have to defined by the amalgamated class 
> 
> nit: extra whitespace at EOL
> 
> What does this comment mean, precisely?
> 
Ah, that's no longer needed, it was from a previous design. I'll remove it.

> ::: testing/marionette/client/setup.py
> @@ +15,4 @@
> >          'mozprocess >= 0.9', 'mozrunner >= 5.15',
> >          'mozdevice >= 0.22', 'moznetwork >= 0.21',
> >          'mozcrash >= 0.5', 'mozprofile >= 0.7',
> > +        'moztest >= 0.1', 'py==1.4.14']
> 
> Generally, our policy is that we shouldn't add external dependencies to
> in-tree packages, unless we add the external package as well (to
> $topsrcdir/python).
> 
> I don't think this will immediately break anything, but it would become
> problematic if we ever wanted to expose HTML reporting via a mach target,
> for instance.  For now, I think we should file a separate bug about it, in
> which we can decide either to rewrite the HTML reporting to use Python's
> built-in minidom, or import py to the tree.

Aha, thanks. Would adding it to setup.py here cause the mozharness script that runs in tbpl to try to pull down py and fail? In any case, I can take a cursory look at replacing our use of py with something built in.
> Aha, thanks. Would adding it to setup.py here cause the mozharness script that runs in tbpl to try to 
> pull down py and fail? In any case, I can take a cursory look at replacing our use of py with 
> something built in.

I _think_ it will be ok, since py (worst package name *ever*) 1.4.14 already exists in puppetagain...it will end up just getting downloaded from there.
Yeah there's no python standard library for html generation, so I'll test on try with the puppetagain package.
Changed according to feedback and I updated our TestResult classes so we can modify them according to mixins. This is illustrated by the HTMLReportingTestResultMixin.

I ran this in try against the marionette-webapi emulator tests and it looks happy: https://tbpl.mozilla.org/?tree=Try&rev=a7e514a36fbe

I'm waiting on desktop try builds here: https://tbpl.mozilla.org/?tree=Try&rev=8de558491d84 but I tested some locally without issue.
Attachment #833184 - Attachment is obsolete: true
Attachment #833184 - Flags: feedback?(dave.hunt)
Attachment #8335484 - Flags: review?(jgriffin)
Attachment #8335484 - Flags: feedback?(dave.hunt)
Comment on attachment 8335484 [details] [diff] [review]
separate out reusable bits of marionette and gaiatest runtests.py into mixins

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

Gah, there's some missing bit in the endurance mixin where it assumes it has access to a device when that's not entirely correct. Will r? when I pull out these messy bits.
Attachment #8335484 - Flags: review?(jgriffin)
Comment on attachment 8335484 [details] [diff] [review]
separate out reusable bits of marionette and gaiatest runtests.py into mixins

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

This looks good to me so far.
Attachment #8335484 - Flags: feedback?(dave.hunt) → feedback+
Blocks: 940916
This adds a preliminary b2g mixin that will be fleshed out later to hold useful functions like restart_b2g and so forth. For now, we just need this here so gaiatest can use it.

It also remodels the endurance mixin so we can define a checkpointing function, and a process function, allowing the drive() function to be really extensible. 

Running new patch against try: https://tbpl.mozilla.org/?tree=Try&rev=dea859a5bcd3
Attachment #8335484 - Attachment is obsolete: true
Attachment #8337051 - Flags: review?(jgriffin)
Attachment #8337051 - Flags: feedback+
Some of the methods currently in gaiatest are used by other packages (b2gperf, b2gpopulate, eideticker, etc). We'll need to make sure we don't break these dependencies. I think in most cases we are pinning to a specific version of gaiatest, so we should be able to manage any changes via controlled releases.
(In reply to Dave Hunt (:davehunt) from comment #16)
> Some of the methods currently in gaiatest are used by other packages
> (b2gperf, b2gpopulate, eideticker, etc). We'll need to make sure we don't
> break these dependencies. I think in most cases we are pinning to a specific
> version of gaiatest, so we should be able to manage any changes via
> controlled releases.

Yes, thanks, I see that b2gperf and b2gpopulate are pinned to gaiatest 0.19. I'll have to test and update these after gaiatest gets updated. I spoke with RyanVM about landing the patch, and we should be able to land the gecko and gaiatest patch shortly after one another, so we should be able to avoid any impact for anything that isn't pinned.

Can we start pinning the version of the marionette-client in gaiatest? That would save us this worry the next time we push out significant changes to the client.
Comment on attachment 8337051 [details] [diff] [review]
separate out reusable bits of marionette and gaiatest runtests.py into mixins

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

\o/

::: testing/marionette/client/marionette/runner/base.py
@@ +310,5 @@
> +class BaseMarionetteOptions(OptionParser):
> +    def __init__(self, **kwargs):
> +        OptionParser.__init__(self, **kwargs)
> +        self.parse_args_handlers = [] # Used by mixins
> +        self.verify_usage_handlers = [] #Used by mixins

nit: add a space after #

@@ +440,5 @@
> +                        default=False,
> +                        help='run tests in a random order')
> +
> +        self.parse_args_handlers = []
> +        self.verify_usage_handlers = []

These two are already defined at the top of __init__

::: testing/marionette/client/marionette/runner/mixins/reporting.py
@@ +19,5 @@
> +    """
> +    Name should be the name of the name of the testrunner, version should correspond
> +    to the testrunner version.
> +    html_output is the file to output to
> +    """

This is probably just inherited from copy/paste, but the docstring should be after __init__, not above it.
Attachment #8337051 - Flags: review?(jgriffin) → review+
> Can we start pinning the version of the marionette-client in gaiatest? That would save us this worry 
> the next time we push out significant changes to the client.

Actually we can't pin gaiatest, because then the TBPL jobs will break when marionette_client in gecko is bumped, since they won't be able to find the version of marionette_client specified in gaiatest's setup.py.
Attached patch quality_control (obsolete) — Splinter Review
Endurance tests should be a bit more flexible with what we can modify, so I added a few more methods to the EnduranceTestCaseMixin and unbitrotted the patch.
Attachment #8339472 - Flags: review?(jgriffin)
Attached file Gaia test runner changes (obsolete) —
forgot to mark older patch as obsolete.
Attachment #8337051 - Attachment is obsolete: true
Attachment #8339475 - Flags: review?(dave.hunt)
Comment on attachment 8339472 [details] [diff] [review]
quality_control

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

::: testing/marionette/client/marionette/runner/base.py
@@ +1,4 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +from optparse import OptionParser

nit: need newline after comment block

@@ +440,5 @@
> +                        default=False,
> +                        help='run tests in a random order')
> +
> +        self.parse_args_handlers = []
> +        self.verify_usage_handlers = []

These are defined already at the top of __init__.

::: testing/marionette/client/marionette/runner/mixins/reporting.py
@@ +19,5 @@
> +    """
> +    Name should be the name of the name of the testrunner, version should correspond
> +    to the testrunner version.
> +    html_output is the file to output to
> +    """

nit: put docstring below __init__, not above it (probably inherited from original code)
Attachment #8339472 - Flags: review?(jgriffin) → review+
derp, sorry, I forgot to hit qrefresh after forgetting to address your comments from before.
Attachment #8339472 - Attachment is obsolete: true
Attachment #8339507 - Flags: review+
Now with 0.003% more new lines!
Attachment #8339507 - Attachment is obsolete: true
Attachment #8339508 - Flags: review+
Comment on attachment 8339475 [details] [review]
Gaia test runner changes

This looks good to me. I was expecting the power draw stuff to be in it's own mixin? I haven't run anything locally with this patch applied, so my review is based only on looking over the code.
Attachment #8339475 - Flags: review?(dave.hunt) → review+
Oh, please add the fix from bug 943313 to your patch, which just landed this morning.
(In reply to Dave Hunt (:davehunt) from comment #25)
> Comment on attachment 8339475 [details] [review]
> Gaia test runner changes
> 
> This looks good to me. I was expecting the power draw stuff to be in it's
> own mixin? I haven't run anything locally with this patch applied, so my
> review is based only on looking over the code.

You're right, it should be, but I'd prefer to address that in a follow up bug since I'd like to get the major changes landed first before more of it bitrots to eternity!
(In reply to Malini Das [:mdas] from comment #27)
> (In reply to Dave Hunt (:davehunt) from comment #25)
> > Comment on attachment 8339475 [details] [review]
> > Gaia test runner changes
> > 
> > This looks good to me. I was expecting the power draw stuff to be in it's
> > own mixin? I haven't run anything locally with this patch applied, so my
> > review is based only on looking over the code.
> 
> You're right, it should be, but I'd prefer to address that in a follow up
> bug since I'd like to get the major changes landed first before more of it
> bitrots to eternity!

That makes sense to me.
Attached patch Gaia test runner changes (obsolete) — Splinter Review
The reason this patch failed to land on tbpl was because my code would attempt to create a device manager instance for b2g desktop. 

The only differences between this PR and the previous one is add_device_manager function has been added to GaiaDevice, and these two lines to add the manager if needed: https://github.com/mozilla-b2g/gaia/pull/14476/files#diff-6d544bbb7c759ffb68f344f348f7740eR758
Attachment #8339475 - Attachment is obsolete: true
Attachment #8343998 - Flags: review?(dave.hunt)
Accidentally checked the 'patch' button
Attachment #8343998 - Attachment is obsolete: true
Attachment #8343998 - Flags: review?(dave.hunt)
Attachment #8343999 - Flags: review?(dave.hunt)
Comment on attachment 8343999 [details]
Gaia test runner changes

Comments in the pull request. We're missing a return statement for the device manager and the mixin/* files are not included in this patch.
Attachment #8343999 - Flags: review?(dave.hunt) → review-
Comment on attachment 8343999 [details]
Gaia test runner changes

Ah, whoops, I've squashed my last commits which got rid of your review comments, but in response to the return statement,  I've modified https://github.com/malini/gaia/blob/2b25e687abf430b1a42e093883dadc3918ef10d5/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L634 to return the device manager if it is set, or throw an exception if the manager is not set/not an android device. I set the device manager after creating GaiaDevice if it's not an android build: https://github.com/malini/gaia/blob/2b25e687abf430b1a42e093883dadc3918ef10d5/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L761

I also added the missing gaiatest/mixins/ files.
Attachment #8343999 - Flags: review- → review?(dave.hunt)
Comment on attachment 8343999 [details]
Gaia test runner changes

Looks good. I also applied the patches locally and successfully ran a test on device and desktop client.
Attachment #8343999 - Flags: review?(dave.hunt) → review+
gaia tree is closed, so I can't land. Also, I think it would be best to wait until gaia branches, so landing this doesn't interfere with the last minute fixes trying to go in. I'll be pinging gwagner to find out when the branch will happen. It was supposed to be sometime today or tomorrow, but the tree closing might delay that.
Fixing up bitrot, and carrying r+ forward. This patch works locally (and with the gaiatest changes on device & b2g desktop) , but I'll run in try just to be sure: https://tbpl.mozilla.org/?tree=Try&rev=bb0356fe7793
Attachment #8339508 - Attachment is obsolete: true
Attachment #8345508 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/846553b4825a
https://hg.mozilla.org/mozilla-central/rev/0a8c1260857e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Note: This change should land in 1.3 so that we can mirror changes in gaiatest/client. I'll work on that tomorrow.
This pr wraps up all the 0.21, 0.21.1 and 0.21.2 changes in as well.
Attachment #8348139 - Flags: review?(dave.hunt)
This was backed out of Aurora in https://hg.mozilla.org/releases/mozilla-aurora/rev/21640bf3442d and https://hg.mozilla.org/releases/mozilla-aurora/rev/dfb969af5d44 for breaking Gaia UI tests there.

Gaia pushbot's associated push was also backed out since I doubt it'd work if it was left there without the other two changesets: https://hg.mozilla.org/releases/mozilla-aurora/rev/81edb867831b


Prior to pushbot's push, the Gu test error looked like https://tbpl.mozilla.org/php/getParsedLog.php?id=32047800&tree=Mozilla-Aurora

After pushbot pushed, the error looked like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=32050959&tree=Mozilla-Aurora

Not really sure what needs to be done here. MDas?
Flags: needinfo?(mdas)
Oh, and as part of this, bug 949406 was also backed out from Aurora because this bug's backout conflicted with it.
Zac was able to fix it by fixing the test path. Since https://bugzilla.mozilla.org/show_bug.cgi?id=946753 landed in the batch of patches I uplifted, it triggered the failure of this test since the file didn't exist. The error message was pretty unclear to me when I  saw it last night though, I wasn't sure which test was failing and exactly why, which is why I filed https://bugzilla.mozilla.org/show_bug.cgi?id=951153
Flags: needinfo?(mdas)
This has now landed on mozilla-aurora.
You need to log in before you can comment on or make changes to this bug.