Closed Bug 975169 Opened 6 years ago Closed 6 years ago

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

Categories

(Testing :: Marionette, defect, P1, major)

defect

Tracking

(b2g-v1.3 fixed)

RESOLVED FIXED
Tracking Status
b2g-v1.3 --- fixed

People

(Reporter: mdas, Assigned: mdas)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Assignee: nobody → mdas
Note: devise a unittest for this.
Attached patch add b2g check non-invasively (obsolete) — Splinter Review
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.
Attachment #8379960 - Flags: review?(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?
Attachment #8379960 - Flags: review?(jgriffin) → review+
Attached patch add b2g check non-invasively v2 (obsolete) — Splinter Review
Addressed issues and moved r+ forward, thanks!
Attachment #8379960 - Attachment is obsolete: true
Attachment #8380669 - Flags: review+
added a setup.py change so I can bump teh version and release to pypi. asking r? from dburns
Attachment #8380669 - Attachment is obsolete: true
Attachment #8380675 - Flags: review?(dburns)
Attachment #8380675 - Flags: review?(dburns) → review+
landed on m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b3bda5a4744

will push to pypi when it lands in m-c
Summary: Marionette test runner will override textrunnerclass if running on device → Marionette test runner will override textrunnerclass if running on device, update pypi package
Whiteboard: [leave-open]
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
Attachment #8380675 - Attachment is obsolete: true
Attachment #8381577 - Flags: review?(jgriffin)
posted wrong diff
Attachment #8381577 - Attachment is obsolete: true
Attachment #8381577 - Flags: review?(jgriffin)
Attachment #8381578 - Flags: review?(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.
Attachment #8381578 - Flags: review?(jgriffin) → review+
pushed to pypi
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open]
You need to log in before you can comment on or make changes to this bug.