902679 - DeviceManagerADB expect deviceSerial but the option set by MarionetteTestOption is device_serial.

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Nicolas B. Pierron [:nbp]]

Attachment: expect an argument identical to the given named argument.

While using one computer with the gaia-ui-test to run multiple builds on multiples phones, and having one central flashing point.  I noticed that Marionette client was not using the the "-s <serial>" options when calling adb.

When multiple phones are plugged, this breaks the cleanUp phase of the gaia-ui-tests which is calling "adb shell" to remove every file contained on the sdcard.

Comment 1

[Jonathan Griffin (:jgriffin)]

Comment on attachment 787156 [details] [diff] [review]
expect an argument identical to the given named argument.

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

I'm a little confused about how this fixes anything, although I agree it can be a problem.

Marionette itself doesn't delete any media files, that's done by gaiatest:  https://github.com/mozilla/gaia-ui-tests/blob/master/gaiatest/gaia_test.py#L458

Gaiatest creates its own devicemanager instance, here:  https://github.com/mozilla/gaia-ui-tests/blob/master/gaiatest/gaia_test.py#L333

The way to get what you want is to pass a deviceSerial argument to the ctor.  So the problem becomes how to hook up the existing self.device_serial attribute of MarionetteTestRunner to either the Marionette instance or testvars object, both of which are passed to GaiaDevice.  If you need more guidance on how to do this, let me know.

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Jonathan Griffin (:jgriffin) from comment #1)
> The way to get what you want is to pass a deviceSerial argument to the ctor.
> So the problem becomes how to hook up the existing self.device_serial
> attribute of MarionetteTestRunner to either the Marionette instance or
> testvars object, both of which are passed to GaiaDevice.  If you need more
> guidance on how to do this, let me know.

The gaia-ui-test does not handle at all the --device option, which is provided by MarionetteTestOption, which value is set to device_serial.  The dictionary is given as argument of the DeviceManagerADB, which then look for some named variable.  As the option variable is named device_serial, it does not match the deviceSerial argument expected by the DeviceManagerADB.  Thus doing this variable renaming match the option given as argument of the gaia-ui-test python script, and should add the "-s <serial>" to every command as expected.

The other option would be to modify the option name in marionette, even if it does not match the underscore naming style of options.

What do you think?

Comment 3

[Jonathan Griffin (:jgriffin)]

> The dictionary is given as argument of the DeviceManagerADB

This is not true; the instance of DeviceManagerADB() which is used to delete files on the sdcard is created here, with no arguments:  https://github.com/mozilla/gaia-ui-tests/blob/master/gaiatest/gaia_test.py#L333

We need to add a deviceSerial arg here, which means figuring out how to pass it from MarionetteTestRunner (where it exists as self.device_serial) to that constructor.

Fixing this will likely require two commits then, one to Marionette, and one to gaia-ui-tests.  The Marionette commit can add a device_serial attribute to Marionette here:  http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette.py#201, along with some code to pass MarionetteTestRunner's device_serial to the Marionette ctor, here:  http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runtests.py#354.

After that lands, you'll need to make a pull request to gaia-ui-tests to add deviceSerial=marionette.device_serial to the DeviceManagerADB() constructor here:  https://github.com/mozilla/gaia-ui-tests/blob/master/gaiatest/gaia_test.py#L333

Comment 4

[Nicolas B. Pierron [:nbp]]

Indeed, I don't remember how I came up with this renaming. I have been probably miss lead by grep and some similar class names :/

Comment 5

[Nicolas B. Pierron [:nbp]]

Unassigned my-self as I am not actively working on it right now.

Comment 6

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

Attachment: bug902679.patch

Part 1: Store the device serial in Marionette.

Comment 7

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

Landed in mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78f20a417a5a

Comment 8

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

Part 2 will have to wait until a new version of the Marionette client has been released, otherwise it will cause failures in the Gaia UI tests.

Comment 9

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/78f20a417a5a

Comment 10

[Nicolas B. Pierron [:nbp]]

After checking today, this patch does not seems to solve the problem I see with the gaia-ui-tests, which is using another branch, and which does not take advantage of the device_serial of Marionette to initializae the device_manager.

I will create a follow-up bug to fix both Marionette and the gaia-ui-test.