1340584 - Remove all references to devicemanagerSUT

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[William Lachance (:wlach)]

We have a lot of references and code which uses the deprecated devicemanagerSUT interface. This was intended to be a protocol for controlling remote devices (like Android phones), but we wound up deprecating it in favor of using adb. To reduce confusion, we should remove all references to it.

This includes:

* Tests and code in testing/mozbase/ (be careful to preserve devicemanagerADB, which is still used, although deprecated)
* Documentation in testing/mozbase/docs (actually remove reference to devicemanager here altogether, as we don't want people using it for new code -- if they really need to know something, they can look at the source)
* Any references/usage of it in the rest of the tree (search for devicemanagerSUT). Same proviso applies to preserving devicemanagerADB functionality.

This isn't a good first bug, but might be an ok task for someone who has a patch or two to mozilla-central under their belt and has try access.

Comment 1

[Geoff Brown [:gbrown]]

Attachment: remove devicemanagerSUT

Definitely simplifies mozdevice and doesn't seem to break anything: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dfffaa5f51d9347ce779655100ea42fe5834975

Comment 2

[William Lachance (:wlach)]

Comment on attachment 8847435 [details] [diff] [review]
remove devicemanagerSUT

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

This is good! I would also remove the various options to pick a devicemanager type from the scripts: the modularity serves no purpose when there is only one option (and I don't foresee us adding it back in the forseeable future). I'd like to have one more look at this after that change, so cancelling review for now.

::: build/mobile/remoteautomation.py
@@ +327,3 @@
>              try:
>                  newLogContent = self.dm.pullFile(self.proc, self.stdoutlen)
>              except DMError:

I suspect we should let this exception surface at some point (though not in this patch), as a DMError here now means something has gone seriously wrong. If you agree, could you file a followup?

::: js/src/jit-test/jit_test.py
@@ +145,2 @@
>                    help='The transport to use to communicate with device:'
> +                  ' [adb]; default=adb')

Let's remove this option altogether. May need to double check callers, but it should be ok.

::: js/src/tests/lib/jittests.py
@@ +669,2 @@
>  
>      if options.device_transport == 'adb':

Can remove this logic as well.

::: layout/tools/reftest/reftestcommandline.py
@@ +457,2 @@
>                            help="the transport to use to communicate with device: "
> +                               "[adb]; default=adb")

Let's remove this option as well.

::: layout/tools/reftest/remotereftest.py
@@ +343,5 @@
>              dm_args['deviceSerial'] = options.deviceSerial
>          dm_cls = mozdevice.DroidADB
> +    else:
> +        print("Automation Error: unknown device manager type")
> +        return 1

Can remove this logic.

::: testing/mochitest/mochitest_options.py
@@ +878,3 @@
>            "default": "adb",
>            "help": "The transport to use for communication with the device [default: adb].",
>            "suppress": True,

Again, let's remove this.

::: testing/mozbase/docs/mozdevice.rst
@@ +22,1 @@
>  .. autoclass:: DeviceManager

I don't think we should bother documenting DeviceManager anymore (even the adb variant), since :bc's device class is much better. I'd just take all of this out.

::: testing/mozbase/mozdevice/mozdevice/devicemanager.py
@@ +46,4 @@
>      applications from the device.
>  
>      Never instantiate this class directly! Instead, instantiate an
> +    implementation of it like DeviceManagerADB.

Could you add a comment about this class being deprecated?

::: testing/mozbase/mozdevice/mozdevice/dmcli.py
@@ +175,3 @@
>                              "to DM_TRANS environment variable, if "
>                              "present, or adb)",
>                              default=os.environ.get('DM_TRANS', 'adb'))

Again, we can get rid of this.

@@ +216,2 @@
>          else:
>              self.parser.error("Unknown device manager type: %s" % type)

And this.

::: testing/mozbase/mozversion/mozversion/mozversion.py
@@ +199,1 @@
>              dm = mozdevice.DeviceManagerADB(deviceSerial=device_serial,

Could you get rid of the dm_type stuff from this thing too?

::: testing/remotecppunittests.py
@@ +169,5 @@
>  
>          self.add_option("--dm_trans", action="store",
>                          type = "string", dest = "dm_trans",
> +                        help = "the transport to use to communicate with device: [adb]; default=adb")
> +        defaults["dm_trans"] = "adb"

Once again, no need for this option anymore.

::: testing/xpcshell/mach_commands.py
@@ +155,4 @@
>              else:
>                  dm = mozdevice.DroidADB(packageName=None, deviceRoot=remote_test_root)
>          else:
> +            raise Exception("unknown device manager type")

Same thing, no need for this option.

::: testing/xpcshell/remotexpcshelltests.py
@@ +584,5 @@
>          else:
>              dm = mozdevice.DroidADB(packageName=None, deviceRoot=options.remoteTestRoot)
>      else:
> +        print "Error: unknown device manager type"
> +        sys.exit(1)

Let's remove the device type here too.

::: testing/xpcshell/xpcshellcommandline.py
@@ +128,5 @@
>                          default=20701, help="port of remote device to test")
>  
>      parser.add_argument("--dm_trans", action="store", type=str, dest="dm_trans",
> +                        choices=["adb"], default="adb",
> +                        help="the transport to use to communicate with device: [adb]; default=adb")

And here.

Comment 3

[Geoff Brown [:gbrown]]

(In reply to William Lachance (:wlach) (use needinfo!) from comment #2)
> ::: build/mobile/remoteautomation.py
> @@ +327,3 @@
> >              try:
> >                  newLogContent = self.dm.pullFile(self.proc, self.stdoutlen)
> >              except DMError:
> 
> I suspect we should let this exception surface at some point (though not in
> this patch), as a DMError here now means something has gone seriously wrong.
> If you agree, could you file a followup?

I'm not sure that a DMError here is cause for serious concern (I wouldn't be surprised if there are low-frequency intermittent, temporary failures via adb). The DMError is translated into a False return code, which IMO is sufficient in this context. If the caller can recover, I'm comfortable with that; I don't think there is need for follow-up.

Comment 4

[Geoff Brown [:gbrown]]

Attachment: remove devicemanagerSUT

I think I've addressed all of the other review comments in this patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd9e74116e57d2bd684e604f021a8e34661513dd

Comment 5

[William Lachance (:wlach)]

(In reply to Geoff Brown [:gbrown] from comment #3)
> (In reply to William Lachance (:wlach) (use needinfo!) from comment #2)
> > ::: build/mobile/remoteautomation.py
> > @@ +327,3 @@
> > >              try:
> > >                  newLogContent = self.dm.pullFile(self.proc, self.stdoutlen)
> > >              except DMError:
> > 
> > I suspect we should let this exception surface at some point (though not in
> > this patch), as a DMError here now means something has gone seriously wrong.
> > If you agree, could you file a followup?
> 
> I'm not sure that a DMError here is cause for serious concern (I wouldn't be
> surprised if there are low-frequency intermittent, temporary failures via
> adb). The DMError is translated into a False return code, which IMO is
> sufficient in this context. If the caller can recover, I'm comfortable with
> that; I don't think there is need for follow-up.

Ok, I'll defer to your judgement on this. :) I've grown paranoid about changing anything to do with the behaviour of our mobile harnesses.

Comment 6

[William Lachance (:wlach)]

Comment on attachment 8847778 [details] [diff] [review]
remove devicemanagerSUT

If try is happy with this, then I'm happy. Thanks for doing this work! Anything to make our mobile testing easier to understand is a big win.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e73241bcb49a
Remove devicemanagerSUT and references to sutagent; r=wlach

Comment 8

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

Windows build failures like https://treeherder.mozilla.org/logviewer.html#?job_id=84365227&repo=mozilla-inbound started happening when this landed. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/cfe7fdc401e0

Comment 9

[Geoff Brown [:gbrown]]

(In reply to Wes Kocher (:KWierso) from comment #8)

Those failures look like:

[task 2017-03-16T16:51:01.461485Z]  1:39.37 TEST-UNEXPECTED-FAIL | /home/worker/checkouts/gecko/testing/mozbase/mozversion/tests/test_b2g.py | SourcesTest.test_b2g_fallback_when_no_binary, line 74: NO MESSAGE
[task 2017-03-16T16:51:01.462466Z]  1:39.37 ERROR: test_b2g_fallback_when_no_binary (__main__.SourcesTest)
[task 2017-03-16T16:51:01.462568Z]  1:39.37 Traceback (most recent call last):
[task 2017-03-16T16:51:01.462679Z]  1:39.37   File "/home/worker/checkouts/gecko/testing/mozbase/mozversion/tests/test_b2g.py", line 74, in test_b2g_fallback_when_no_binary
[task 2017-03-16T16:51:01.464373Z]  1:39.37     self.assertRaises(errors.RemoteAppNotFoundError, get_version)
[task 2017-03-16T16:51:01.465016Z]  1:39.37   File "/usr/lib/python2.7/unittest/case.py", line 473, in assertRaises
[task 2017-03-16T16:51:01.465306Z]  1:39.37     callableObj(*args, **kwargs)
[task 2017-03-16T16:51:01.465721Z]  1:39.37   File "/home/worker/checkouts/gecko/testing/mozbase/mozversion/mozversion/mozversion.py", line 280, in get_version
[task 2017-03-16T16:51:01.466162Z]  1:39.37     device_serial=device_serial)
[task 2017-03-16T16:51:01.466926Z]  1:39.37   File "/home/worker/checkouts/gecko/testing/mozbase/mozversion/mozversion/mozversion.py", line 200, in __init__
[task 2017-03-16T16:51:01.468169Z]  1:39.37     serverPort=adb_port)
[task 2017-03-16T16:51:01.468444Z]  1:39.37   File "/home/worker/checkouts/gecko/testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py", line 74, in __init__
[task 2017-03-16T16:51:01.468496Z]  1:39.37     self._verifyADB()
[task 2017-03-16T16:51:01.468773Z]  1:39.37   File "/home/worker/checkouts/gecko/testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py", line 760, in _verifyADB
[task 2017-03-16T16:51:01.468825Z]  1:39.37     "and adb is in your $PATH" % err)
[task 2017-03-16T16:51:01.468888Z]  1:39.37 DMError: unable to execute ADB ([Errno 2] No such file or directory): ensure Android SDK is installed and adb is in your $PATH


test_b2g_fallback_when_no_binary was previously agnostic to the presence of adb. I changed the test to always use the adb devicemanager, where it had previously allowed for sut, or none. Perhaps I should not have eliminated the option of no devicemanager.

Comment 10

[Geoff Brown [:gbrown]]

Attachment: remove the failing test

I think that removing the dm_type parameter from get_version() was appropriate, but doing so makes it impossible to avoid instantiating devicemanagerADB in get_version(). That makes test_b2g_fallback_when_no_binary dependent on the presence of adb: get_version() (without a binary) may succeed if adb is present and will throw DMError if adb is present. I don't see value in testing this, especially in the context of b2g, so let's just remove the test.

Comment 11

[Bob Clary [:bc] (inactive)]

Note this broke autophone's unit tests as well. Example:

remotereftest.py: error: unrecognized arguments: --dm_trans=adb

Comment 12

[Geoff Brown [:gbrown]]

Thanks Bob, I almost missed that.

It looks like the only autophone issue is https://github.com/mozilla/autophone/blob/master/tests/runtestsremote.py#L332. Dealing with that seems tricky. We could make the --dm_trans in remotereftest.py default to adb, land that on m-c, then remove the option from autophone and get that landed and deployed, then proceed with the main patch here. Is there a better way?

Comment 13

[Geoff Brown [:gbrown]]

Attachment: patch 0 - first change reftest dm_trans default to adb

As discussed on irc. Will need to uplift to aurora, beta, and release before proceeding with autophone changes.

Comment 14

[Bob Clary [:bc] (inactive)]

Comment on attachment 8848335 [details] [diff] [review]
patch 0 - first change reftest dm_trans default to adb

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

lgtm! r+

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12ce302b8f9b
Change reftest dm_trans default from sut to adb; r=bc

Comment 16

[William Lachance (:wlach)]

(In reply to Geoff Brown [:gbrown] from comment #12)
> Thanks Bob, I almost missed that.
> 
> It looks like the only autophone issue is
> https://github.com/mozilla/autophone/blob/master/tests/runtestsremote.
> py#L332. Dealing with that seems tricky. We could make the --dm_trans in
> remotereftest.py default to adb, land that on m-c, then remove the option
> from autophone and get that landed and deployed, then proceed with the main
> patch here. Is there a better way?

As an aside, this is a good argument for passing this kind of state via environment variables rather than command line arguments. If we had done that, we could have avoided a fair bit of hassle (since we could have just started to silently ignore a `DM_TRANS` environment variable). Too late now, but something to keep in mind for the future...

Comment 17

[Geoff Brown [:gbrown]]

I would appreciate help getting the low-risk, test-only "patch 0" on aurora, beta, and release. Other patches here cannot land until "patch 0" is on aurora/beta/release.

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/12ce302b8f9b

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8154f92ee3e2

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6d0b85a258c5

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/c734b3d6b601

Comment 22

[Geoff Brown [:gbrown]]

Attachment: remove dm_trans reference from autophone

Comment 23

[Bob Clary [:bc] (inactive)]

Comment on attachment 8849358 [details] [diff] [review]
remove dm_trans reference from autophone

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

Give me a heads up before you push your other patch so I can deploy this, but go ahead and push this to github.

Thanks!

Comment 24

[Geoff Brown [:gbrown]]

https://github.com/mozilla/autophone/commit/d5e25ba83a571ae4fedc5bcaa3c5cf97abcbf502

Comment 25

[Bob Clary [:bc] (inactive)]

deployed autophone patch about 2017-03-21 10:50

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30820b7d399b
Remove devicemanagerSUT and references to sutagent; r=wlach
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5716cc2b192
Remove test_b2g_fallback_when_no_binary; r=wlach

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/30820b7d399b
https://hg.mozilla.org/mozilla-central/rev/e5716cc2b192