If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

get_device_manager relies on a session existing

RESOLVED FIXED in mozilla33

Status

Testing
Marionette
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: zac, Assigned: davehunt)

Tracking

unspecified
mozilla33
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
Since bug 1019043 landed, get_device_manager in B2GTestCaseMixin relies on a marionette session being open, but generally the test framework relies on the device manager to be able to stop/start/manipulate the device into the state of being ready to start a session.

If get_device_manager could work without requiring a session (ie it could decipher the desktopb2g from the command line args) then we'd reduce this inter-dependency.
Can we pass a device_manager instance to the TestCase object (if appropriate), so we don't have to instantiate one in the test itself?
(Reporter)

Updated

3 years ago
Blocks: 1033975
(Reporter)

Comment 2

3 years ago
Dave, now you're back, do you have an ideas about this?

It blocks fixing of the crash handling of gaiatest (which is currently broken).

I've also worked around this in gaiatest but it's not pretty (see me for the commit), it'd definitely be better to fix this higher up in the framework.
Flags: needinfo?(dave.hunt)
(Assignee)

Comment 3

3 years ago
The short-term solution could be to assume we're running against a device if we can't determine otherwise. Something like the following should work:

> def get_device_manager(self, *args, **kwargs):
>     capabilities = self.marionette.session and self.marionette.session_capabilities or {}
>     if not self._device_manager and capabilities.get('device') != 'desktop':
>         self._device_manager = get_dm(self.marionette, **kwargs)
>     return self._device_manager

Longer-term, I'm wondering if we should switch to the new B2GDeviceRunner in mozrunner for managing our devices. This doesn't have a dependency on Marionette, and should be able to replace some of our GaiaDevice methods. Andrew: I'm keen to hear your thoughts.
Flags: needinfo?(dave.hunt) → needinfo?(ahalberstadt)
Yeah, using B2GDeviceRunner seems like a good idea to me. It should be useable now, though there's a few patches I have yet to land that should fix some profile related setup problems (e.g bug 1036982).

For now, why can't you just create a new dm instance in that file instead of grabbing it from marionette? The wouldn't know about the detecting whether it's 'desktop' or not part though.
Flags: needinfo?(ahalberstadt)
s/The/I
(Assignee)

Comment 6

3 years ago
Created attachment 8456366 [details] [diff] [review]
Remove dependency on Marionette session from get_device_manager. v1.0

Here's the short-term fix. This works well for me locally with a test I put together to cause a crash. The longer-term fix should be covered by bug 1038868 and bug 1038870.
Assignee: nobody → dave.hunt
Status: NEW → ASSIGNED
Attachment #8456366 - Flags: review?(jgriffin)
(Assignee)

Comment 7

3 years ago
Created attachment 8456370 [details]
test_crash.py

Test for simulating a crash. Run at least one test after this to test the crash recovery, or run with --repeat=1.
(Assignee)

Comment 8

3 years ago
Created attachment 8456371 [details] [diff] [review]
Fix crash recovery in gaiatest.

This patch fixes the crash recovery in gaiatest, and can be used to test the marionette client patch.
Comment on attachment 8456366 [details] [diff] [review]
Remove dependency on Marionette session from get_device_manager. v1.0

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

I'm not sure how this will impact B2G desktop Gip runs, but if it works there, this seems like a good short-term fix.
Attachment #8456366 - Flags: review?(jgriffin) → review+
(Reporter)

Comment 10

3 years ago
Did you do a Try run Dave?
(Assignee)

Comment 11

3 years ago
Not yet, I wanted a review first. I'll kick off a try run shortly.
(Assignee)

Comment 12

3 years ago
Try: https://tbpl.mozilla.org/?tree=Try&rev=50b4aab1b6dd
(Assignee)

Updated

3 years ago
Flags: needinfo?(dave.hunt)
(Assignee)

Comment 13

3 years ago
Try was green.
Flags: needinfo?(dave.hunt)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/b76e4fdff4da
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/a0364999f064
https://hg.mozilla.org/mozilla-central/rev/a0364999f064
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/b76e4fdff4da
You need to log in before you can comment on or make changes to this bug.