Closed Bug 1340584 Opened 3 years ago Closed 3 years ago

Remove all references to devicemanagerSUT

Categories

(Testing :: Mozbase, defect)

Version 3
defect
Not set

Tracking

(firefox52 fixed, firefox53 fixed, firefox54 fixed, firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: wlach, Assigned: gbrown, Mentored)

References

Details

Attachments

(4 files, 1 obsolete file)

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.
Assignee: nobody → gbrown
Attached patch remove devicemanagerSUT (obsolete) — Splinter Review
Definitely simplifies mozdevice and doesn't seem to break anything: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dfffaa5f51d9347ce779655100ea42fe5834975
Attachment #8847435 - Flags: review?(wlachance)
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.
Attachment #8847435 - Flags: review?(wlachance)
(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.
I think I've addressed all of the other review comments in this patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd9e74116e57d2bd684e604f021a8e34661513dd
Attachment #8847435 - Attachment is obsolete: true
Attachment #8847778 - Flags: review?(wlachance)
(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 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.
Attachment #8847778 - Flags: review?(wlachance) → review+
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e73241bcb49a
Remove devicemanagerSUT and references to sutagent; r=wlach
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
Flags: needinfo?(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.
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.
Attachment #8848310 - Flags: review?(wlachance)
Note this broke autophone's unit tests as well. Example:

remotereftest.py: error: unrecognized arguments: --dm_trans=adb
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 discussed on irc. Will need to uplift to aurora, beta, and release before proceeding with autophone changes.
Attachment #8848335 - Flags: review?(bob)
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+
Attachment #8848335 - Flags: review?(bob) → review+
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12ce302b8f9b
Change reftest dm_trans default from sut to adb; r=bc
Flags: needinfo?(gbrown)
Keywords: leave-open
Attachment #8848310 - Flags: review?(wlachance) → review+
(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...
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.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta][checkin-needed-release] "patch 0" / comment 17
https://hg.mozilla.org/releases/mozilla-aurora/rev/8154f92ee3e2
Whiteboard: [checkin-needed-aurora][checkin-needed-beta][checkin-needed-release] "patch 0" / comment 17 → [checkin-needed-beta][checkin-needed-release] "patch 0" / comment 17
https://hg.mozilla.org/releases/mozilla-beta/rev/6d0b85a258c5
Whiteboard: [checkin-needed-beta][checkin-needed-release] "patch 0" / comment 17 → [checkin-needed-release] "patch 0" / comment 17
https://hg.mozilla.org/releases/mozilla-release/rev/c734b3d6b601
Whiteboard: [checkin-needed-release] "patch 0" / comment 17
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!
Attachment #8849358 - Flags: review?(bob) → review+
deployed autophone patch about 2017-03-21 10:50
Pushed by gbrown@mozilla.com:
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
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/30820b7d399b
https://hg.mozilla.org/mozilla-central/rev/e5716cc2b192
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1444421
You need to log in before you can comment on or make changes to this bug.