925398 - Refactor runtests.py to follow Mixin pattern

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Malini Das [:mdas] - Away, not checking bugmail]

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.

Comment 1

[Walter Chen[:ypwalter][:wachen]]

Taking this. Anyone who likes to do this can take this from me also.

Comment 2

[Malini Das [:mdas] - Away, not checking bugmail]

taking this for work on Bug 914983

Comment 3

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: separate out reusable bits of marionette and gaiatest runtests.py into mixins

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?

Comment 4

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: separate out reusable bits of marionette and gaiatest runtests.py into mixins

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

Comment 5

[Malini Das [:mdas] - Away, not checking bugmail]

Actually, from the looks of it, mtbf test case (https://github.com/Mozilla-TWQA/gaia/blob/mtbf-v1.2/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L831) and driver (https://github.com/Mozilla-TWQA/MTBF-Driver) don't seem to need any changes

Comment 6

[Malini Das [:mdas] - Away, not checking bugmail]

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.

Comment 7

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: separate out reusable bits of marionette and gaiatest runtests.py into mixins

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.

Comment 8

[Jonathan Griffin (:jgriffin)]

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.

Comment 9

[Malini Das [:mdas] - Away, not checking bugmail]

(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.

Comment 10

[Jonathan Griffin (:jgriffin)]

> 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.

Comment 11

[Malini Das [:mdas] - Away, not checking bugmail]

Yeah there's no python standard library for html generation, so I'll test on try with the puppetagain package.

Comment 12

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: separate out reusable bits of marionette and gaiatest runtests.py into mixins

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.

Comment 13

[Malini Das [:mdas] - Away, not checking bugmail]

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.

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 15

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: separate out reusable bits of marionette and gaiatest runtests.py into mixins

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

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 17

[Malini Das [:mdas] - Away, not checking bugmail]

(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 18

[Jonathan Griffin (:jgriffin)]

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.

Comment 19

[Jonathan Griffin (:jgriffin)]

> 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.

Comment 20

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: quality_control

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.

Comment 21

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: Gaia test runner changes

forgot to mark older patch as obsolete.

Comment 22

[Jonathan Griffin (:jgriffin)]

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)

Comment 23

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: separate out reusable bits of marionette and gaiatest runtests.py into mixins

derp, sorry, I forgot to hit qrefresh after forgetting to address your comments from before.

Comment 24

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: separate out reusable bits of marionette and gaiatest runtests.py into mixins

Now with 0.003% more new lines!

Comment 25

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 26

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Oh, please add the fix from bug 943313 to your patch, which just landed this morning.

Comment 27

[Malini Das [:mdas] - Away, not checking bugmail]

(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!

Comment 28

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(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.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Backed out for Gaia UI Test failures.
https://hg.mozilla.org/integration/b2g-inbound/rev/4e8c42397431

https://tbpl.mozilla.org/php/getParsedLog.php?id=31508355&tree=B2g-Inbound

Comment 30

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: Gaia test runner changes

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

Comment 31

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: Gaia test runner changes

Accidentally checked the 'patch' button

Comment 32

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 33

[Malini Das [:mdas] - Away, not checking bugmail]

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.

Comment 34

[Malini Das [:mdas] - Away, not checking bugmail]

Noticed an error when running tests, I updated the PR again, here are the updated links in order:

https://github.com/malini/gaia/blob/mixin/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L634
https://github.com/malini/gaia/blob/mixin/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L761

Comment 35

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 36

[Malini Das [:mdas] - Away, not checking bugmail]

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.

Comment 37

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: separate out reusable bits of marionette and gaiatest runtests.py into mixins

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

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/846553b4825a
https://hg.mozilla.org/mozilla-central/rev/0a8c1260857e

Comment 39

[Malini Das [:mdas] - Away, not checking bugmail]

Note: This change should land in 1.3 so that we can mirror changes in gaiatest/client. I'll work on that tomorrow.

Comment 40

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: uplift the gaiatest PRs to v1.3

This pr wraps up all the 0.21, 0.21.1 and 0.21.2 changes in as well.

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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?

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Oh, and as part of this, bug 949406 was also backed out from Aurora because this bug's backout conflicted with it.

Comment 43

[Malini Das [:mdas] - Away, not checking bugmail]

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

Comment 44

[Malini Das [:mdas] - Away, not checking bugmail]

This has now landed on mozilla-aurora.

Comment 45

[Malini Das [:mdas] - Away, not checking bugmail]

https://hg.mozilla.org/releases/mozilla-aurora/rev/491926e34c6b
https://hg.mozilla.org/releases/mozilla-aurora/rev/387bc39e4689