975169 - Marionette test runner will override textrunnerclass if running on device, update pypi package

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

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

This manifested itself here: https://github.com/mozilla-b2g/gaia/pull/16460#issuecomment-35672405 and is fallout from https://bugzilla.mozilla.org/show_bug.cgi?id=959520

What's going on is, regardless of the TextTestRunner you want to use (in this case, we want GaiaTextTestRunner), if we're running against a device, then we set it to B2GMarionetteTextTestRunner. The offending line is http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runner/base.py#767. The original reasoning behind this was to allow a single marionette test to be reusable between a device and desktop. If we run a test against a device, then we'll add special functions to detect if b2g crashes, etc. If we're on desktop, we don't want these functions added. So this was to autodetect the environment, and add the special functions to it automatically. 

The problem here is that it arbitrarily sets the texttestrunner to B2GMarionetteTextTestRunner when instead, it should add the b2g functions to the current runner. We'll have to find a way to integrate this behaviour better.

Comment 1

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

Note: devise a unittest for this.

Comment 2

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

Attachment: add b2g check non-invasively

the socket.timeout patch was accidentally forcing us to use B2GMarionetteTextTestRunner regardless of the TextTestRunner we specify in our tests if we were running against the device.

To fix this, I removed this class, and made the runner add the b2g checker dynamically without changing its class. In order to get this to work, it also dynamically adds (not aggressively replaces!) the B2GTestResultMixin class to whatever test resultclass is being used. I've tested this patch using marionette and gaiatest, and I don't see any issues.

Comment 3

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8379960 [details] [diff] [review]
add b2g check non-invasively

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

::: testing/marionette/client/marionette/runner/base.py
@@ +267,5 @@
> +        else:
> +            return self.resultclass(self.stream,
> +                                    self.descriptions,
> +                                    self.verbosity,
> +                                    marionette=self.marionette)

Can we just set self.b2g_pid to None by default, and so avoid having to pass a different set of parameters to self.resultclass depending on whether we're B2G or not?

Comment 4

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

Attachment: add b2g check non-invasively v2

Addressed issues and moved r+ forward, thanks!

Comment 5

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

Attachment: add b2g check non-invasively v2, plus version bump

added a setup.py change so I can bump teh version and release to pypi. asking r? from dburns

Comment 6

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

landed on m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b3bda5a4744

will push to pypi when it lands in m-c

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for breaking Marionette-webapi.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3e92264b4eb

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

Comment 8

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

Attachment: add b2g check non-invasively v3, plus version bump

So the bug was due to trying to add a base classes of MarionetteTestResult that already exists (since we've added it before). I didn't realize that by changing __class__ we were changing the class for all MarionetteTestResult. I've changed it to conditionally add the class if we don't have it already.

While it sounds awful (and kind of is), this patch works for us. We will only use one testrunner against one environment, according to the --type parameter, ie: when we run against a device, we will change __class__ to include the B2GTestResultMixin, if we're in any other environment, we don't modify the class. We never run a suite against multiple environments, so we're safe from having this negatively affect any test runs.

If this is r+'d, then I'll open a new bug to revise the current Mixin implementation. I’m not happy with this pseudo multi-inheritance mixin thing. we don’t need to call __init__ most of the time. I'd like to investigate using class function like B2GMixin.add_stuff_to_me(some_object) to add/modify the *instance* instead of inheriting and modifying the class. I wanted to do that here, but it's outside of the scope.

Try run for desktop:
https://tbpl.mozilla.org/?tree=Try&rev=39f9a799396e

For emulator:
https://tbpl.mozilla.org/?tree=Try&rev=0a9325e3c17e

and for G/Gu desktop, since I forgot to add it before:
https://tbpl.mozilla.org/?tree=Try&rev=e144246b6d86

Comment 9

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

Attachment: add b2g check non-invasively v3, plus version bump

posted wrong diff

Comment 10

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8381578 [details] [diff] [review]
add b2g check non-invasively v3, plus version bump

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

I agree this is a little gross.  I'm not really sure that dynamically adding methods/classes to an instance is less gross; at least this is fairly readable.

Comment 11

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

filed https://bugzilla.mozilla.org/show_bug.cgi?id=976832

landing in m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cf3762073dc

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6cf3762073dc

Comment 13

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

pushed to pypi