1440714 - Migrate remaining clients of devicemanagerADB to adb.py

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, enhancement, P1)

Description

[Geoff Brown [:gbrown]]

:bc did some great things with adb.py for autophone and we should use that for our emulator unit testing too. It may be possible to eliminate devicemanager entirely.

Differences in rooting requirements / assumptions likely make devicemanagerADB troublesome for our bitbar trial -- another reason to make the switch.

Comment 1

[Geoff Brown [:gbrown]]

Attachment: cppunit tests - first try

This is a very preliminary attempt at converting the cppunit test harness to adb.py. It works for me, locally, against an emulator, but certainly needs more testing.

Most changes are quite simple. Use of shell() is a little more complicated because I needed both the command output and exit code.

:bc -- Does this look like a sensible start? Any thoughts/guidance appreciated...

Comment 2

[Bob Clary [:bc] (inactive)]

Comment on attachment 8954450 [details] [diff] [review]
cppunit tests - first try

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

I'm a bit concerned about the need to specify root on physical devices. Currently we specify root=False on the ADBAndroid methods which requires us to override this on a per method call basis. This is not an issue with emulators since they all run with adbd as root, but going forward I am not sure our current approach is appropriate.

Perhaps we need to assume that our devices are rooted and either have adbd as root or have su available. We could possible change the constructor to take a root= parameter which would be the default value used by the methods unless the method over-rode the default setting. If we ever start to move away from requiring rooted devices, then we can use the combination of the default constructor provided value for root and the ability to override root on the methods to ween ourselves from requiring rooted devices.

Let's talk during our mobile/testing meeting today.

::: testing/remotecppunittests.py
@@ +31,5 @@
>          self.options = options
> +        self.device = ADBAndroid(adb=options.adb_path,
> +                                 adb_host=options.device_ip,
> +                                 adb_port=options.device_port,
> +                                 test_root=options.remote_test_root)

I'm confused. I believe ADBAndroid considers adb_host, adb_port to be the ip and port that the adb server is running under. For example, localhost and 5037. This results is the adb command looking like

adb -H <device ip> -P <device port>

Is this what we want?

Also, if device is not passed then ADBAndroid will expect that only one device is attached and raise an error if more than one device is attached.

This can be a problem for developers if they have more than one device. For emulators, it seems only one emulator per docker-worker is used so that is ok. For our possible third party hosting of devices, it seems ok since they set things up to only have one device visible in each container but in general I think it might be a problem to expect that only one device is visible.

@@ +43,5 @@
>      def setup_bin(self, progs):
> +        if not self.device.is_dir(self.remote_test_root):
> +            self.device.mkdir(self.remote_test_root)
> +        if self.device.is_dir(self.remote_tmp_dir):
> +            self.device.rm(self.remote_tmp_dir, recursive=True)

This pattern

if is_dir dir:
  rm dir

requires two round trips to the device. removing a directory that does not exist will result in an ADBError with No such file or directory. We *might* want to just catch the ADBError and cut down on the round trips. But that is probably premature optimization... /Roseanne Roseannadanna: nevermind.

Comment 3

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #2)
> Perhaps we need to assume that our devices are rooted and either have adbd
> as root or have su available. We could possible change the constructor to
> take a root= parameter which would be the default value used by the methods
> unless the method over-rode the default setting. If we ever start to move
> away from requiring rooted devices, then we can use the combination of the
> default constructor provided value for root and the ability to override root
> on the methods to ween ourselves from requiring rooted devices.

That seems reasonable, but it's not a high priority for me (until I hit a rooting issue!). Maybe a follow-up or separate patch?

> I'm confused.

Not at all -- *I* was confusing:

1. adb -H and -P specify the host/port of the adb server.
   devicemanager supports this through the constructor parameters 'serverHost' and 'serverPort', but the only known client to use that feature is some b2g code in mozversion.
   ADBDevice supports this through the constructor parameters 'adb_host' and 'adb_port'...but I don't think I will need to use them.

2. 'adb connect' host/port parameters for connecting to a device over tcp/ip.
   devicemanager supports this through the constructor parameters 'host' and 'port' and test harnesses typically have corresponding 'deviceIP' and 'devicePort' options.
   I think adb.py does not explicitly support this. I have never used these options and doubt their usefulness, so I am tempted to simply remove the deviceIP/devicePort options from the harnesses.

3. 'adb -s <serial>' parameter for distinguishing between multiple connected devices.
   devicemanager supports this through the constructor parameter 'deviceSerial' and test harnesses typically have a corresponding 'deviceSerial' option.
   ADBDevice allows this to be specified through the constructor parameter 'device'. I will use that.

Comment 4

[Geoff Brown [:gbrown]]

Attachment: cppunit tests - work in progress

Comment 5

[Geoff Brown [:gbrown]]

Attachment: xpcshell tests - work in progress

Comment 6

[Geoff Brown [:gbrown]]

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

Comment 7

[Geoff Brown [:gbrown]]

Attachment: support adb install -r

The robocop harness historically installs the robocop.apk with 'adb install -r robocop.apk' rather than uninstall + install. Supporting the -r install flag will be handy for that.

Comment 8

[Geoff Brown [:gbrown]]

Attachment: add ADBAndroid.get_top_activity()

Port of devicemanager getTopActivity(), 

https://dxr.mozilla.org/mozilla-central/rev/efe1f380d0279d7786b0c6340a60aaf6c76eabbe/testing/mozbase/mozdevice/mozdevice/droid.py#159

getTopActivity is used by browser tests to monitor whether fennec (or geckoview) is the focused/active application.

Comment 9

[Bob Clary [:bc] (inactive)]

Comment on attachment 8956843 [details] [diff] [review]
support adb install -r

r+ lgtm.

Comment 10

[Bob Clary [:bc] (inactive)]

Comment on attachment 8956845 [details] [diff] [review]
add ADBAndroid.get_top_activity()

also lgtm. r+

Comment 11

[Geoff Brown [:gbrown]]

Attachment: add ADBDevice.get_file()

Here's a quick port of devicemanager's pullFile(),

https://dxr.mozilla.org/mozilla-central/rev/a6a32fb286fa9e5d5f6d5b3b77423ab6b96c9502/testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py#494-509

It helps me when porting code like

https://dxr.mozilla.org/mozilla-central/rev/a6a32fb286fa9e5d5f6d5b3b77423ab6b96c9502/build/mobile/remoteautomation.py#303

Comment 12

[Bob Clary [:bc] (inactive)]

Comment on attachment 8957224 [details] [diff] [review]
add ADBDevice.get_file()

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

lgtm. r+

We should reconsider dropping all of mozdevice's documentation from in tree.

Comment 13

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] (pto 2018-03-08) from comment #12)
> We should reconsider dropping all of mozdevice's documentation from in tree.

...and I'll update the docs for my changes in bug 1444421.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8f57a9ebb8f
Add -r option to ADBAndroid.install_app(); r=bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/37bb096eb861
Add ADBAndroid.get_top_activity() to determine the focused app; r=bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfe063307abd
Add ADBDevice.get_file() to pull and read a remote file; r=bc

Comment 15

[Geoff Brown [:gbrown]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a19bb7388deef4adb5ca49b478dfe8876ac6ec73 is encouraging.

Comment 16

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b8f57a9ebb8f
https://hg.mozilla.org/mozilla-central/rev/37bb096eb861
https://hg.mozilla.org/mozilla-central/rev/bfe063307abd

Comment 17

[Geoff Brown [:gbrown]]

Mochitest and reftest looking good. robocop failing on testSessionOOMSave, but otherwise okay:

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

Comment 18

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #2)
> This pattern
> 
> if is_dir dir:
>   rm dir
> 
> requires two round trips to the device. removing a directory that does not
> exist will result in an ADBError with No such file or directory. We *might*
> want to just catch the ADBError and cut down on the round trips. But that is
> probably premature optimization... /Roseanne Roseannadanna: nevermind.

I have found I need to use that pattern quite often in the harnesses. In addition to the 2x round trips, it is a little unsightly. A try/catch would often be just as unsightly, and I usually do not want to catch the exception if it is for anything other than No such file or directory. I noticed rm() has a 'force' option that does the try/catch and error check for me -- going with that!

Comment 19

[Geoff Brown [:gbrown]]

Attachment: convert rungeckoview to adb.py

Here's the simplest harness conversion: rungeckoview, for running the geckoview smoketest.

A few notes:

 - I've opted to catch Exception where the harness previously caught DMError specifically. Usually I'm only expecting ADBError or ADBTimeoutError in these cases, but I think if something else was raised, I'd probably want the same handling.

 - I've removed the deviceIP harness option from rungeckoview, and I intend to remove deviceIP/devicePort from all the harnesses. I've never used these options. --deviceSerial is maintained, for selecting a locally connected device.

 - I've used shell_output() to launch geckoview rather than launch_application(); shell_output() was easy in this case and I didn't quite see how launch_application() could be used for this exact command.

 - I'm adding MOZ_CRASHREPORTER_SHUTDOWN to the geckoview environment 'cause I forgot this case in bug 1440719

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

Comment 20

[Geoff Brown [:gbrown]]

Attachment: convert cppunit tests to adb.py

Tested locally with mach against 4.3 and 7.0 emulators, and

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

Comment 21

[Geoff Brown [:gbrown]]

Attachment: convert xpcshell tests to adb.py

Comment 22

[Geoff Brown [:gbrown]]

Attachment: remove "remote b2g" capability from mozversion

I don't imagine anyone will miss this minor feature of mozversion for b2g.

Comment 23

[Bob Clary [:bc] (inactive)]

Comment on attachment 8959159 [details] [diff] [review]
remove "remote b2g" capability from mozversion


>+        version = LocalVersion(binary)
>+        if version._info.get('application_name') == 'B2G':
>+            version = LocalB2GVersion(binary, sources=sources)

we still need B2G here?

Comment 24

[Bob Clary [:bc] (inactive)]

I guess you're just removing the remote aspects which means we still support directly connected b2g devices for now. ok.

Comment 25

[Geoff Brown [:gbrown]]

Right. I imagine LocalB2GVersion could be removed too, but I don't want to remove it without cause.

Comment 26

[Bob Clary [:bc] (inactive)]

(In reply to Geoff Brown [:gbrown] from comment #19)

>  - I've used shell_output() to launch geckoview rather than
> launch_application(); shell_output() was easy in this case and I didn't
> quite see how launch_application() could be used for this exact command.

Autophone launches fennec via launch_fennec which uses launch_application:

https://github.com/mozilla/autophone/blob/master/phonetest.py#L1134

while it launches geckoview_example via launch_application:

https://github.com/mozilla/autophone/blob/master/tests/s1s2geckoviewtest.py#L27

Note the multiprocess support.

I'd really like to continue delegating to launch_application even if we have to wrap it to support fennec or geckoview_example. Do you think we can add launch_geckoview_example to adb_android.py along with launch_fennec and add support for specifying e10s at least for geckoview_example ?

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eda01be00c0
Add ADBError, ADBTimeoutError to mozharness retry-on-error list; r=me,a=test-only

Comment 28

[Bob Clary [:bc] (inactive)]

Comment on attachment 8958838 [details] [diff] [review]
convert rungeckoview to adb.py

Everything looks good, but I'd like to do the launch_application approach.

Comment 29

[Bob Clary [:bc] (inactive)]

Comment on attachment 8958876 [details] [diff] [review]
convert cppunit tests to adb.py


>     def setup_bin(self, progs):
>-        if not self.device.dirExists(self.remote_test_root):
>-            self.device.mkDir(self.remote_test_root)
>-        if self.device.dirExists(self.remote_tmp_dir):
>-            self.device.removeDir(self.remote_tmp_dir)
>-        self.device.mkDir(self.remote_tmp_dir)
>-        if self.device.dirExists(self.remote_bin_dir):
>-            self.device.removeDir(self.remote_bin_dir)
>-        self.device.mkDir(self.remote_bin_dir)
>-        if self.device.dirExists(self.remote_home_dir):
>-            self.device.removeDir(self.remote_home_dir)
>-        self.device.mkDir(self.remote_home_dir)
>+        if not self.device.is_dir(self.remote_test_root):
>+            self.device.mkdir(self.remote_test_root)
>+        self.device.rm(self.remote_tmp_dir, force=True, recursive=True)
>+        self.device.mkdir(self.remote_tmp_dir)
>+        self.device.rm(self.remote_bin_dir, force=True, recursive=True)
>+        self.device.mkdir(self.remote_bin_dir)
>+        self.device.rm(self.remote_home_dir, force=True, recursive=True)
>+        self.device.mkdir(self.remote_home_dir)

Do we need to preserve self.remote_test_root or can we just remove it and all of its child directories then recreate it and its children without checking and removing them individually?

Comment 30

[Bob Clary [:bc] (inactive)]

Comment on attachment 8958876 [details] [diff] [review]
convert cppunit tests to adb.py


>+    def shell(self, cmd, env, cwd, timeout):
>+        adb_process = None
>+        try:
>+            adb_process = self.device.shell(cmd, env=env, cwd=cwd,
>+                                            timeout=timeout)
>+            if adb_process.timedout:
>+                raise ADBTimeoutError("%s" % adb_process)
>+            output = adb_process.stdout_file.read().rstrip()
>+            return (adb_process.exitcode, output)
>+        finally:
>+            if adb_process and isinstance(adb_process.stdout_file, file):
>+                adb_process.stdout_file.close()
>+

I'd rather not introduce a new shell method here. It appears that ADBError is deficient and we should fix that.

Doing a quick manual test and catching the ADBError shows:

>>> e.message
'args: adb -s 501a2aa7 wait-for-device shell ls /sdcard/tests/foobar; echo rc=$?, exitcode: 1, stdout: ls: /sdcard/tests/foobar: No such file or directory'

I think we should modify ADBError to add

ADBError.command() to return the text between args: and echo rc=$?
ADBError.exitcode() to return the exitcode value
ADBError.output() to return the text following stdout:

The we could do something like

try:
   return (0, dm.shell_output('ls /sdcard/tests/foobar'))
except ADBError, e:
   return (e.exitcode(), e.output())

What do you think?

Comment 31

[Geoff Brown [:gbrown]]

Attachment: convert rungeckoview to adb.py

Now using launch_geckoview_example, as suggested in comment 26.

I'm not sure if fennec accepts the same multiprocess argument -- let's deal with that another day.

Comment 32

[Bob Clary [:bc] (inactive)]

Comment on attachment 8958878 [details] [diff] [review]
convert xpcshell tests to adb.py

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

Overall looking good. I'm not entirely happy with the lack of distinction between Exceptions especially ADBError, ADBTimeoutError and ADBRootError.

ADBTimeoutError and ADBRootError are pretty fatal. No need to attempt retries if they appear. Let's chat either on irc or vidyo.

::: testing/xpcshell/remotexpcshelltests.py
@@ +153,5 @@
> +                # Ignore this exception to simplify the error report.
> +                self.shellReturnCode = None
> +                pass
> +            else:
> +                raise e

non-nit: self.timedout in the except clause should be adb_process.timedout. Couldn't this be handled using an except ADBTimeoutError?

I'm a bit confused here. This launches a process on the device and returns immediately once that is accomplished. The process continues to run after shell is called. Any timeout here should only be due to a failure to contact the device with in the desired period and not due to the test running too long.

nit: dangling else.

        cmd = ADBDevice._escape_command_line(cmd)
        try:
            adb_process = self.device.shell(cmd, timeout=timeout+10)
            output_file = adb_process.stdout_file
            self.shellReturnCode = adb_process.exitcode
        except Exception as e:
            if not adb_process.timedout:
                raise e
            # If the test timed out, there is a good chance the shell
            # call also timed out and raised this Exception.
            # Ignore this exception to simplify the error report.
            self.timedout = True
            self.shellReturnCode = None

@@ +158,5 @@
>          # The device manager may have timed out waiting for xpcshell.
>          # Guard against an accumulation of hung processes by killing
>          # them here. Note also that IPC tests may spawn new instances
>          # of xpcshell.
> +        self.device.pkill("xpcshell")

This probably doesn't require root since it was probably started by the shell user anyway.

What if the cmd was to run xpcshell?

@@ +159,5 @@
>          # Guard against an accumulation of hung processes by killing
>          # them here. Note also that IPC tests may spawn new instances
>          # of xpcshell.
> +        self.device.pkill("xpcshell")
> +        return output_file

This doesn't match what is done in runxpcshelltests.py and the definitions for communicate, poll and kill work around that runxpcshelltests expect this to return a Popen object instead of an open file.

If we modified ADBProcess to implement the missing Popen methods, and for the kill method added an additional ADBAndroid.kill in for the first command argument, then we could just return our new ADBProcess here and drop the redefinitions below.

What do you think?

@@ +180,5 @@
>                    self, dumpDir, symbols_path, test_name)
>              self.clearRemoteDir(self.remoteMinidumpDir)
>          return crashed
>  
>      def communicate(self, proc):

drop?

@@ +188,3 @@
>          return contents, ""
>  
>      def poll(self, proc):

drop?

@@ +192,5 @@
>              return self.getReturnCode(proc)
>          # Process is still running
>          return None
>  
>      def kill(self, proc):

drop?

@@ +250,3 @@
>          # Add Android version (SDK level) to mozinfo so that manifest entries
>          # can be conditional on android_version.
> +        androidVersion = self.device.shell_output('getprop ro.build.version.sdk')

self.device._adb_version ? Maybe we should make the _adb_version "not private" and just remove the leading _ altogether.

@@ +392,5 @@
> +        self.device.rm(self.remoteTmpDir, force=True, recursive=True)
> +        self.device.mkdir(self.remoteTmpDir)
> +        remotePrefDir = posixpath.join(self.remoteBinDir, "defaults", "pref")
> +        if not self.device.is_dir(remotePrefDir):
> +            self.device.mkdir(posixpath.join(remotePrefDir, "extra"), parents=True)

What if remotePrefDir existed but didn't have an extra subdirectory?

@@ +396,5 @@
> +            self.device.mkdir(posixpath.join(remotePrefDir, "extra"), parents=True)
> +        if not self.device.is_dir(self.remoteScriptsDir):
> +            self.device.mkdir(self.remoteScriptsDir)
> +        if not self.device.is_dir(self.remoteComponentsDir):
> +            self.device.mkdir(self.remoteComponentsDir)

We can eliminate the test for is_dir if we add parents=True.

self.device.mkdir(posixpath.join(remotePrefDir, "extra"), parents=True)
self.device.mkdir(self.remoteScriptsDir, parents=True)
self.device.mkdir(self.remoteComponentsDir, parents=True)

Comment 33

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #29)
> Do we need to preserve self.remote_test_root or can we just remove it and
> all of its child directories then recreate it and its children without
> checking and removing them individually?

Good idea - I'll clean that up.

Comment 34

[Bob Clary [:bc] (inactive)]

Comment on attachment 8959159 [details] [diff] [review]
remove "remote b2g" capability from mozversion

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

lgtm. r+

Comment 35

[Bob Clary [:bc] (inactive)]

Comment on attachment 8959261 [details] [diff] [review]
convert rungeckoview to adb.py

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

testing/mozbase/mozversion/mozversion/mozversion.py:10: 'six.StringIO' imported but unused

otherwise looks good to me, r+.

Comment 36

[Geoff Brown [:gbrown]]

Attachment: convert cppunit tests to adb.py

Thanks for all your suggestions - this is definitely improving!

Comment 37

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/6eda01be00c0

Comment 38

[Bob Clary [:bc] (inactive)]

(In reply to Bob Clary [:bc:] from comment #35)
> Comment on attachment 8959261 [details] [diff] [review]
> convert rungeckoview to adb.py

I missed testing/mochitest/rungeckoview.py:107: local variable 'e' is assigned to but never used

Comment 39

[Bob Clary [:bc] (inactive)]

Comment on attachment 8959384 [details] [diff] [review]
convert cppunit tests to adb.py

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

r+ with this minor nits fixed. Thanks!

::: testing/remotecppunittests.py
@@ +16,2 @@
>  import posixpath
> +from mozdevice import ADBAndroid, ADBProcessError, ADBTimeoutError

ADBTimeoutError isn't used.

@@ +39,5 @@
>              self.setup_bin(progs)
>  
>      def setup_bin(self, progs):
> +        self.device.rm(self.remote_test_root, force=True, recursive=True)
> +        self.device.mkdir(self.remote_home_dir, parents=True)

missing self.device.mkdir(self.remote_bin_dir)

Comment 40

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #39)
> ADBTimeoutError isn't used.

Sorry - I keep forgetting to lint before requesting review!!

> missing self.device.mkdir(self.remote_bin_dir)

It turns out this mkdir isn't necessary: it is created later when we push to it.

Comment 41

[Geoff Brown [:gbrown]]

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

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bae19b274c7b
Remove devicemanager code from mozversion; r=bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8e2178f6adc
Convert rungeckoview to adb.py; r=bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d0760485016
Convert Android cppunit test harness to adb.py; r=bc

Comment 43

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #32)
> ADBTimeoutError and ADBRootError are pretty fatal. No need to attempt
> retries if they appear. Let's chat either on irc or vidyo.

I agree ADBRootError is pretty fatal. 

From time to time our aws emulator environment becomes unresponsive, typically mid-test. When that happens with adb.py, I typically see ADBTimeoutError raised. Comment 27 pushed an update to the mozharness error regex so that the task will retry when a task fails with ADBTimeoutError. I've seen that working well on try pushes.

I also included ADBError in that regex and I am less confident about the need for that -- might refine that regex later.

Comment 44

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #32)
> ::: testing/xpcshell/remotexpcshelltests.py
> @@ +153,5 @@
> > +                # Ignore this exception to simplify the error report.
> > +                self.shellReturnCode = None
> > +                pass
> > +            else:
> > +                raise e
> 
> non-nit: self.timedout in the except clause should be adb_process.timedout.
> Couldn't this be handled using an except ADBTimeoutError?
> 
> I'm a bit confused here. This launches a process on the device and returns
> immediately once that is accomplished. The process continues to run after
> shell is called. Any timeout here should only be due to a failure to contact
> the device with in the desired period and not due to the test running too
> long.

This is confusing stuff.

I disagree with "The process continues to run after shell is called": shell() calls poll() in a loop until the process completes or the timeout expires. Are you thinking of the "am start" case, where "am" completes immediately but the started application continues to run?

The special handling of self.timedout is historical. I think it is still useful, but not 100% sure. Notice that the xpcshell base class starts a Timer() before calling launchProcess(); if that Timer expires, self.timedout is set and a test timeout is reported. Meanwhile, the Android launchProcess is waiting for shell(). I expect the shell() to timeout around the same time (10 seconds later), but if the Timer timeout was already triggered, we want to ignore the ADBTimeoutError and continue normally. I'm hazy on the actual threading and order of events - I should check on that.

https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/testing/xpcshell/runxpcshelltests.py#673

Comment 45

[Bob Clary [:bc] (inactive)]

For some reason I was thinking that we returned the adb_process while the process was still running. Thanks for correcting me.

I don't think xpcshell class will be able to set its timedout until after we have already returned from the adb.shell() call since we block it. We'll see.

Regardless, we are getting there! Thanks.

Comment 46

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #32)
> @@ +250,3 @@
> >          # Add Android version (SDK level) to mozinfo so that manifest entries
> >          # can be conditional on android_version.
> > +        androidVersion = self.device.shell_output('getprop ro.build.version.sdk')
> 
> self.device._adb_version ? Maybe we should make the _adb_version "not
> private" and just remove the leading _ altogether.

No, device._adb_version is the adb version; here I want the Android version (sdk level), like "18" for Android 4.3. However, I will change this to use device.get_prop() - a little tidier.

Comment 47

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #32)
> If we modified ADBProcess to implement the missing Popen methods, and for
> the kill method added an additional ADBAndroid.kill in for the first command
> argument, then we could just return our new ADBProcess here and drop the
> redefinitions below.
> 
> What do you think?

I'm accustomed to the hack -- it doesn't bother me much -- and I'm feeling a little anxious about the scope of these changes, so how about we pursue that idea in a follow-up bug?

Comment 48

[Geoff Brown [:gbrown]]

Attachment: convert xpcshell tests to adb.py

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bb5593dc5e47a5723f225ceb7d873824875e599 should demonstrate a test timeout. It looks like the Timer callback runs in a separate thread while our shell() call is blocked. The check for self.timedout is often unnecessary, but think of it as belt-and-suspenders.

Comment 49

[Bob Clary [:bc] (inactive)]

(In reply to Geoff Brown [:gbrown] from comment #47)
> 
> I'm accustomed to the hack -- it doesn't bother me much -- and I'm feeling a
> little anxious about the scope of these changes, so how about we pursue that
> idea in a follow-up bug?

I agree. My original comment was based on my misunderstanding/misremembering how ADBProcess worked. The fact that it automatically waits for the process to complete breaks that suggestion into a million pieces.

I think I would like to follow up on the idea later though. We could make the wait explicit and have the choice of waiting for the process to complete or let it continue to run and pass the ADBProcess around to be used elsewhere.

Comment 50

[Bob Clary [:bc] (inactive)]

Comment on attachment 8959666 [details] [diff] [review]
convert xpcshell tests to adb.py

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

lgtm with nits you can ignore. r+.

Note we'll have to revisit the root issue in a follow up.

::: testing/xpcshell/remotexpcshelltests.py
@@ +153,5 @@
> +                # Ignore this exception to simplify the error report.
> +                self.shellReturnCode = None
> +                pass
> +            else:
> +                raise e

I still hate dangling elses.

@@ +233,3 @@
>  
>  
>  # A specialization of XPCShellTests that runs tests on an Android device

trailing .

@@ +382,4 @@
>              try:
>                  packageName = self.localAPKContents.read("package-name.txt")
>                  if packageName:
> +                    self.appRoot = posixpath.join("/data", "data", packageName.strip())

President Not Sure says: "I don't see why posixpath.join("/data", "data", packageName.strip()) is better than posixpath.join("/data/data", packageName.strip())".

@@ +393,5 @@
> +        self.device.mkdir(self.remoteTmpDir)
> +        remotePrefDir = posixpath.join(self.remoteBinDir, "defaults", "pref")
> +        self.device.mkdir(posixpath.join(remotePrefDir, "extra"), parents=True)
> +        self.device.mkdir(self.remoteScriptsDir, parents=True)
> +        self.device.mkdir(self.remoteComponentsDir, parents=True)

Do we need to clean up by deleting these directories before creating them?

self.device.rm(path, recursive=True, force=True)

will succeed whether the path exists or not.

BTW, this is an example where a real device will require root=True.

Comment 51

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #50)
> ::: testing/xpcshell/remotexpcshelltests.py
> @@ +153,5 @@
> > +                # Ignore this exception to simplify the error report.
> > +                self.shellReturnCode = None
> > +                pass
> > +            else:
> > +                raise e
> 
> I still hate dangling elses.

I realized the 'pass' is unnecessary and removed it, but for the dangling else issue, I don't see what I can do: happy to act on a suggestion for a solution.

> President Not Sure says: "I don't see why posixpath.join("/data", "data",
> packageName.strip()) is better than posixpath.join("/data/data",
> packageName.strip())".

Ha! Totally agree, also not sure...will probably leave it.

> @@ +393,5 @@
> > +        self.device.mkdir(self.remoteTmpDir)
> > +        remotePrefDir = posixpath.join(self.remoteBinDir, "defaults", "pref")
> > +        self.device.mkdir(posixpath.join(remotePrefDir, "extra"), parents=True)
> > +        self.device.mkdir(self.remoteScriptsDir, parents=True)
> > +        self.device.mkdir(self.remoteComponentsDir, parents=True)
> 
> Do we need to clean up by deleting these directories before creating them?

Yes, there were some issues there -- will be fixed on landing.


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

Comment 52

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/bae19b274c7b
https://hg.mozilla.org/mozilla-central/rev/d8e2178f6adc
https://hg.mozilla.org/mozilla-central/rev/0d0760485016

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ec4bc1d3720
Convert Android xpcshell test harness to adb.py; r=bc

Comment 54

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/6ec4bc1d3720

Comment 55

[Geoff Brown [:gbrown]]

I reviewed recent Android geckoview/cppunit/xpcshell results on inbound today, including comparing failure and timeout logs before/after the changes: lgtm.

Comment 56

[Geoff Brown [:gbrown]]

Attachment: convert robocop/mochitest/reftest harnesses to adb.py

This is the biggest patch, converting all the "browser" tests at once, since they all rely on common code in remoteautomation.py. It has all the same simple conversion code + some obvious cleanup and simplification of under-utilized harness options.

remoteautomation.py is a mess in general. This patch simplifies it a little, but there's certainly more to do: I intend to follow up in another bug.

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

Comment 57

[Bob Clary [:bc] (inactive)]

Comment on attachment 8960303 [details] [diff] [review]
convert robocop/mochitest/reftest harnesses to adb.py

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

missing js/src/tests/lib/jittests.py

I'm unclear on the returning of 1 from constructors especially in cases (no firefox) where the error appears to be fatal.

I included comments regarding losing the exception when printing messages about a failure of some kind. I think these will be helpful when debugging why something is failing.

We'll probably have to do all of the pkill's as root, but let's handle those when we add a configurable default value for root.

I think there is one error referencing deviceRoot in layout/tools/reftest/remotereftest.py but I think we can eliminate the use of deviceRoot altogether in favor of test_root.

::: build/mobile/remoteautomation.py
@@ +109,1 @@
>              print "Error deleting %s" % traces

Include the exception in the output?

@@ +109,2 @@
>              print "Error deleting %s" % traces
>              pass

redundant

@@ +125,1 @@
>                  print "Error pulling %s" % traces

Include the exception in the output?

@@ +146,1 @@
>                      # This may just indicate that no tombstone files are present

chmod on a non-existent directory gives a message in the exception containing "No such file or directory".
pull on a non-existent directory gives a message in the exception containing "does not exist".

We are guaranteeed that remoteDir exists since you tested for it above. I can't see a case where we get an exception unless there is a real issue here. We should treat it as an error and at least log the exception somehow.

@@ +240,3 @@
>                  self.procName = app
> +                if not self.device.shell_bool(cmd):
> +                    print "remote_automation.py failed to launch %s" % cmd

Should we fail hard here rather than letting it timeout?

::: layout/tools/reftest/remotereftest.py
@@ +192,5 @@
> +        # Check that Firefox is installed
> +        expected = options.app.split('/')[-1]
> +        if not self.device.is_app_installed(expected):
> +            self.log.error("%s is not installed on this device" % expected)
> +            return 1

I don't understand why you return 1 here. Wouldn't an exception be more appropriate? How will the fact that firefox is not installed be handled?

@@ +348,4 @@
>                          print "     %s" % item
>                  else:
>                      print "  %s: %s" % (category, devinfo[category])
> +            print "Test root: %s" % self.device.deviceRoot

self.device.test_root ?

@@ +353,1 @@
>              print "WARNING: Error getting device information"

Include the exception in the output?

::: testing/mochitest/runrobocop.py
@@ +54,5 @@
> +        # Check that Firefox is installed
> +        expected = options.app.split('/')[-1]
> +        if not self.device.is_app_installed(expected):
> +            self.log.error("%s is not installed on this device" % expected)
> +            return 1

Again, I'm unclear on returning 1 from the constructor or how this is handled.

@@ +57,5 @@
> +            self.log.error("%s is not installed on this device" % expected)
> +            return 1
> +
> +        options.logFile = "robocop.log"
> +        self.deviceRoot = self.device.test_root

Do we need to keep deviceRoot around?

@@ +110,3 @@
>          # Add Android version (SDK level) to mozinfo so that manifest entries
>          # can be conditional on android_version.
> +        androidVersion = self.device.shell_output('getprop ro.build.version.sdk')

ADBAndroid.version

@@ +146,3 @@
>          MochitestDesktop.cleanup(self, self.options)
>          if self.localProfile:
>              os.system("rm -Rf %s" % self.localProfile)

I'm always afraid of rm -Rf. Do we have any check to make sure we aren't doing an rm -Rf / or rm -Rf ~/ ?

@@ +351,1 @@
>              self.log.warning("Error getting device information")

ditto

::: testing/mochitest/runtestsremote.py
@@ +67,5 @@
> +        # Check that Firefox is installed
> +        expected = options.app.split('/')[-1]
> +        if not self.device.is_app_installed(expected):
> +            self.log.error("%s is not installed on this device" % expected)
> +            return 1

ditto.

@@ +98,5 @@
> +            self.log.warning("unable to kill %s before running tests!" % procName)
> +
> +        # Add Android version (SDK level) to mozinfo so that manifest entries
> +        # can be conditional on android_version.
> +        androidVersion = self.device.shell_output('getprop ro.build.version.sdk')

ADBAndroid.version

@@ +223,2 @@
>                  self.log.error(
>                      "Automation Error: Unable to copy test modules to device.")

ditto

@@ +250,5 @@
>  
>      def buildURLOptions(self, options, env):
> +        saveLogFile = options.logFile
> +        options.logFile = self.remoteLogFile
> +        # options.fileLevel = 'INFO'

did you want to leave this ?

@@ +305,1 @@
>              self.log.warning("Error getting device information")

ditto

@@ +375,1 @@
>              # device error cleaning up... oh well!

Log the exception? If we aren't able to clean up it may show up in later tests on the same device.

Comment 58

[Geoff Brown [:gbrown]]

Attachment: convert robocop/mochitest/reftest harnesses to adb.py

Thanks much :bc - you found some great issues there. I think I have addressed all of them in this patch. I'll handle jittests, and mach in separate patches.

Comment 59

[Geoff Brown [:gbrown]]

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

Comment 60

[Bob Clary [:bc] (inactive)]

Comment on attachment 8960691 [details] [diff] [review]
convert robocop/mochitest/reftest harnesses to adb.py

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

Looks good to me. I'm a little concerned about the stack trace printing causing unwanted oranges but I'm ok with them until they prove themselves to be a problem.

I applied your patch to my local tree for use in testing the android hardware testing. One thing I had to fix on my end was the direct specification of the device serial number since adb.py doesn't support the use of the environment variable ANDROID_SERIAL. We might want to change the ADBDevice._get_device_serial to also check if ANDROID_SERIAL is set in the environment. We can do that in a follow up.

I believe lack of root in pkill resulted in the multiple attempts to kill the browser in <https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=169354660>.

Another thing I noticed is when I had issues initially running the docker container and getting the adb usb permissions correct, that failures to contact the devices successfully did not cause failure immediately. You can see an example at <https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=169289819> where the only hint of a problem was the failure to convert the supposed sdk level to an int.

We can handle these in a follow up patch.

r+

Comment 61

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #60)
> I believe lack of root in pkill resulted in the multiple attempts to kill
> the browser in
> <https://treeherder.mozilla.org/#/jobs?repo=try&author=bclary@mozilla.
> com&selectedJob=169354660>.

I'm not sure. This looks a little like problems seen on central this morning related to bug 1444973.
 
> Another thing I noticed is when I had issues initially running the docker
> container and getting the adb usb permissions correct, that failures to
> contact the devices successfully did not cause failure immediately. You can
> see an example at
> <https://treeherder.mozilla.org/#/jobs?repo=try&author=bclary@mozilla.
> com&selectedJob=169289819> where the only hint of a problem was the failure
> to convert the supposed sdk level to an int.

That failed in mozharness, before calling the test harness, so I don't think that could be related to my patch.


There are just a few remaining devicemanager pieces now:
 - jittests
 - mach/android_device
 - marionette
 - remove devicemanager

Thanks again for all the feedback and your patience!

Comment 62

[Bob Clary [:bc] (inactive)]

(In reply to Geoff Brown [:gbrown] from comment #61)
> (In reply to Bob Clary [:bc:] from comment #60)
> > I believe lack of root in pkill resulted in the multiple attempts to kill
> > the browser in
> > <https://treeherder.mozilla.org/#/jobs?repo=try&author=bclary@mozilla.
> > com&selectedJob=169354660>.
> 
> I'm not sure. This looks a little like problems seen on central this morning
> related to bug 1444973.
>  

I'll run some additional tests to make sure what is going on.

> > Another thing I noticed is when I had issues initially running the docker
> > container and getting the adb usb permissions correct, that failures to
> > contact the devices successfully did not cause failure immediately. You can
> > see an example at
> > <https://treeherder.mozilla.org/#/jobs?repo=try&author=bclary@mozilla.
> > com&selectedJob=169289819> where the only hint of a problem was the failure
> > to convert the supposed sdk level to an int.
> 
> That failed in mozharness, before calling the test harness, so I don't think
> that could be related to my patch.
> 

Right. The android mozharness stuff makes direct calls to adb rather than using adb.py. I think at least for android hardware, I'll replace those with adb.py calls. And I'll test various adb over usb failures to see what falls out.
> 
> There are just a few remaining devicemanager pieces now:
>  - jittests
>  - mach/android_device
>  - marionette
>  - remove devicemanager
> 
> Thanks again for all the feedback and your patience!

Thank you!

Comment 63

[Geoff Brown [:gbrown]]

Attachment: conver mach commands to adb.py

This code supports 'mach android-emulator', 'mach autophone', and various other Android mach commands.

One thing that's bothering me is the ADBDevice initialization logging. Since both mach and the harnesses create an ADBDevice, you can see things like:

gbrown@mozpad:~/src$ ./mach mochitest testing/mochitest/tests/Harness_sanity
 0:02.02 adb INFO adbd running as root
 0:02.23 adb INFO su -c supported
 0:02.44 adb INFO su 0 supported
 0:02.86 adb INFO /system/bin/ls -a supported
 0:02.97 adb INFO Native cp support: True
 0:03.18 adb INFO Native chmod -R support: True
 0:05.39 adb INFO adbd running as root
 0:05.60 adb INFO su -c supported
 0:05.81 adb INFO su 0 supported
 0:06.13 adb INFO /system/bin/ls -a supported
 0:06.24 adb INFO Native cp support: True
 0:06.35 adb INFO Native chmod -R support: True
 0:12.30 INFO Android sdk version '18'; will use this to filter manifests
 0:12.30 INFO Checking for ssltunnel processes...
 0:12.32 INFO Checking for xpcshell processes...
 0:12.34 SUITE_START: mochitest-plain - running 27 tests

Thoughts on quieting those messages, at least at the mach level?

Comment 64

[Bob Clary [:bc] (inactive)]

Comment on attachment 8961598 [details] [diff] [review]
conver mach commands to adb.py

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

The only way I can think off off-hand is to make sure that we don't initialize ADBAndroid twice.

We could add something to adb*.py that would allow us to register a global, singleton for an ADBAndroid instance.  There are a number of approaches that appear in searches for python singleton but I don't know which is best.

https://michaelgoerz.net/notes/singleton-objects-in-python.html looks interesting. There are other approaches where you can override __new__ to implement a singleton.

We'd want to support the case where we could have different instances for different devices even though I don't think our current code ever uses multiple devices. Perhaps an option specifying we want a singleton and perhaps checking if the request is for a device which already has a global instance defined. I don't think we are thread-safe so if we are on different threads we have to deal with that.

Does that help?

::: testing/mozbase/mozrunner/mozrunner/devices/android_device.py
@@ +112,5 @@
> +    if device is None:
> +        adb_path = _find_sdk_exe(substs, 'adb', False)
> +        if not adb_path:
> +            adb_path = 'adb'
> +        device = ADBAndroid(adb=adb_path, verbose=verbose_logging)

This won't work in adb.py if more than one device is attached. That appears to be an existing situation however.

::: testing/mozbase/mozrunner/mozrunner/devices/autophone.py
@@ +445,5 @@
>             Check that the specified device is available and rooted.
>          """
>          try:
> +            device = ADBAndroid(adb=adb_path, device=device, timeout=10)
> +            if device._have_su or device._have_android_su or device._have_android_su:

include self.have_root_shell

Comment 65

[Bob Clary [:bc] (inactive)]

The more I think about it I'm beginning to like:

maintain a list of ADBDevice / ADBAndroid instances keyed by serial number.

override __new__ to check if a device has already been initialized and if so use that otherwise create and register a new instance.

I'm unclear on if this will really work though when creating the instances in different files.

Or we can just live with it.

Comment 66

[Geoff Brown [:gbrown]]

Attachment: convert jittest to adb.py

Android jittest doesn't run in CI currently, so it was not surprising to find that they were broken: I had to add tracking of elapsed time per test.

This harness doesn't have all the bells and whistles of mochitest; I may follow-up to add features like adb_path.

Otherwise, I think the patch is pretty straight-forward.


A variety of failures are expected when Android jittests are enabled:

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

Comment 67

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aad77f9bd093
Convert Android browser test harnesses to adb.py; r=bc

Comment 68

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/aad77f9bd093

Comment 69

[Bob Clary [:bc] (inactive)]

Comment on attachment 8961970 [details] [diff] [review]
convert jittest to adb.py

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

prior to the patch I get the following:

js/src/tests/lib/jittests.py:10: 'tempfile' imported but unused
js/src/tests/lib/jittests.py:10: 'time' imported but unused
js/src/tests/lib/jittests.py:749: 'android' imported but unused

The last android one is a false positive since it is used to raise an exception if not available but we might as well fix the others.

devicemanagerADB.chmod is automatically recursive. We need to explicitly request it in adb*.py. Otherwise lgtm. r+.

::: js/src/tests/lib/jittests.py
@@ +688,1 @@
>          # After a devicemanager error, the device is typically in a

s/devicemanager/device/

@@ +721,5 @@
> +    device.rm(jit_tests_dir, force=True, recursive=True)
> +    device.mkdir(options.remote_test_root, parents=True)
> +    push_libs(options, device)
> +    push_progs(options, device, [prefix[0]])
> +    device.chmod(options.remote_test_root)

device.chmod(options.remote_test_root, recursive=True)

Comment 70

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7db3ccb6382a
Convert jittest to adb.py; r=bc

Comment 71

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7db3ccb6382a

Comment 72

[Geoff Brown [:gbrown]]

I have a couple more patches in the works, for mach and marionette -- will try to wrap those up next week.

Comment 73

[Bob Clary [:bc] (inactive)]

Attachment: adb-chmod-sdcard-root.patch

While testing the android hardware framework, I ran into the case where a rooted device but which did not run adbd as root failed when attempting to chmod a directory on the sdcard. I confirmed locally that this would raise an exception if attempted without root but would "succeed" with root. The permissions on the sdcard can't be changed, but at least with root=True it does not throw an exception.

https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&group_state=expanded&fromchange=cdf80d5dc78a93b86b3cf91e10b942b925620739&tochange=dd7f2edf9c50a95b0846fa1637aaeb06835dbb36

shows two runs.

The first is without any patch which shows:
ADBProcessError: args: adb -s HT67R0300011 wait-for-device shell chmod -R 777 /sdcard/tests/modules; echo rc=$?, exitcode: 1, stdout: chmod: chmod '/sdcard/tests/modules' to 40777: Operation not permitted

The second shows the effect of a partial patch for mochitest.

This patch includes all other chmods on a remote directory on the sdcard that I could easily find via pss.

Comment 74

[Geoff Brown [:gbrown]]

Comment on attachment 8964355 [details] [diff] [review]
adb-chmod-sdcard-root.patch

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

Thanks Bob.

Incidentally, I cannot think of a reason for chmod'ing the reftest and mochitest profiles...we might be able to remove those calls. Perhaps in another bug...

Comment 75

[Geoff Brown [:gbrown]]

Attachment: convert mach commands to adb.py

Feeling quite good about this! It even improves on existing support for multiple devices. For a given mach session for any particular device-serial, only one ADBAndroid will be created within the mach code, but I haven't tried to consolidate that with harness code -- good enough for now.

Comment 76

[Bob Clary [:bc] (inactive)]

Comment on attachment 8964727 [details] [diff] [review]
convert mach commands to adb.py

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

android_device.py has a lot of repetition. I think a follow on bug to rewrite it would be nice. We might want to uplift some of these things to adb*.py.
r+

::: testing/mozbase/mozrunner/mozrunner/devices/android_device.py
@@ +379,3 @@
>          _log_warning("unable to launch Firefox for Android")
>          return 1
>      return 0

Seems ok but wouldn't you rather use launch_fennec?

@@ +392,1 @@
>          if sdk_level and int(sdk_level) >= 23:

already an int.

@@ +549,4 @@
>          if self.check_completed():
>              return False
>          _log_debug("Waiting for device status...")
> +        adb_path = _find_sdk_exe(self.substs, 'adb', False)

sure wish we could cache this.

Comment 77

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #76)
> Seems ok but wouldn't you rather use launch_fennec?

Yes - thanks! Will change...

Comment 78

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb8ded0a5654
Convert Android mach commands to adb.py; r=bc

Comment 79

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/cb8ded0a5654

Comment 80

[Bob Clary [:bc] (inactive)]

Attachment: adb-test-root-root.patch

make sure to use root=True when determining/creating test_root

Comment 81

[Bob Clary [:bc] (inactive)]

Attachment: remotexpcshelltests.patch

This does duplicate the initDir across the threads. I could probably figure out how not to dupe it if it is a problem.

Try run for crashtest-1, geckoview, mochitest-media-1, xpcshell-1:

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

I still need to land adb-chmod-sdcard-root.patch but I'll do that with the others if they pass muster.

Comment 82

[Geoff Brown [:gbrown]]

Comment on attachment 8966330 [details] [diff] [review]
adb-test-root-root.patch

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

I think this changes our test requirements from "some test suites may run on non-rooted devices" to "a rooted device is required". Is that correct?

I worry that some developers may want to run tests locally on non-rooted devices (no root shell, no su).

The chmod has no effect on /sdcard -- could we avoid the chmod there (to avoid the su requirement when the test root is on /sdcard)? Could we do something similar for the is_dir/mkdir/rm calls?? Is it worth it?

This is just an issue to think about; I'm OK with going ahead here.

Comment 83

[Bob Clary [:bc] (inactive)]

Yes, at least for now.

Comment 84

[Geoff Brown [:gbrown]]

Comment on attachment 8966343 [details] [diff] [review]
remotexpcshelltests.patch

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

I'm glad to see remoteClearDirScript removed and the directory creation simplified -- thanks for that.

I don't mind additional root requirements here: It seems more reasonable to require root for xpcshell (native app, /data required, etc).

Your patch comment says "set remoteTestRoot to /data/local/tests..." but I don't see any related changes. Am I missing something?

Do you like the name "initDir"? "clearDir"? "createEmptyDir"??

::: testing/xpcshell/remotexpcshelltests.py
@@ +142,5 @@
>          self.timedout = False
>          cmd.insert(1, self.remoteHere)
>          cmd = ADBDevice._escape_command_line(cmd)
>          try:
> +            adb_process = self.device.shell(cmd, env=env, timeout=timeout+10, root=True)

Why add env here? The wrapper (pushWrapper) already sets environment variables for xpcshell.

(There should probably be a comment here explaining why the env parameter is ignored.)

@@ -200,5 @@
>              return self.shellReturnCode
>          else:
>              return -1
>  
> -    def removeDir(self, dirname):

removeDir is called from a few places in the base class -- is it really safe to remove?

@@ +426,5 @@
>                                  # xz strips the ".so" file suffix.
>                                  os.rename(localFile[:-3], localFile)
>                          self.device.push(localFile, remoteFile)
>                          pushed_libs_count += 1
> +                        self.device.chmod(remoteFile, root=True)

There's an alternate, rarely-used, path for pushing the same files below -- probably want to chmod in that case too.

Comment 85

[Bob Clary [:bc] (inactive)]

(In reply to Geoff Brown [:gbrown] from comment #84)
> Comment on attachment 8966343 [details] [diff] [review]
> remotexpcshelltests.patch

> 
> Your patch comment says "set remoteTestRoot to /data/local/tests..." but I
> don't see any related changes. Am I missing something?
> 

Sorry. The original version I attached to the autophone/taskcluster bug had both, but it was in the android_hw.py mozharness config which isn't applicable yet. I'll remove that comment.

> Do you like the name "initDir"? "clearDir"? "createEmptyDir"??

I like it more than clearDir or createEmptyDir. It does a bit more by changing the permissions.

> 
> ::: testing/xpcshell/remotexpcshelltests.py
> @@ +142,5 @@
> >          self.timedout = False
> >          cmd.insert(1, self.remoteHere)
> >          cmd = ADBDevice._escape_command_line(cmd)
> >          try:
> > +            adb_process = self.device.shell(cmd, env=env, timeout=timeout+10, root=True)
> 
> Why add env here? The wrapper (pushWrapper) already sets environment
> variables for xpcshell.
> 

Good point and good catch. I'll remove it.

> (There should probably be a comment here explaining why the env parameter is
> ignored.)
> 

Will do.

> @@ -200,5 @@
> >              return self.shellReturnCode
> >          else:
> >              return -1
> >  
> > -    def removeDir(self, dirname):
> 
> removeDir is called from a few places in the base class -- is it really safe
> to remove?

Hmmm. I think it is safe but to be extra-safe I can add it back, add a root=True and log it if it is called.

> 
> @@ +426,5 @@
> >                                  # xz strips the ".so" file suffix.
> >                                  os.rename(localFile[:-3], localFile)
> >                          self.device.push(localFile, remoteFile)
> >                          pushed_libs_count += 1
> > +                        self.device.chmod(remoteFile, root=True)
> 
> There's an alternate, rarely-used, path for pushing the same files below --
> probably want to chmod in that case too.

Thanks. I'll do that.

Comment 86

[Bob Clary [:bc] (inactive)]

There were a number of places where we did a push but I hadn't included the corresponding chmod. I'm testing a patch using emulators and hardware where I've added them everywhere to be consistent and to see if that would fix my xpcshell errors on devices. It makes me wonder though whether I should just change adb*.py to automatically do a chmod after a push rather than the error prone trying to remember to chmod after each push. What do you think?

Comment 87

[Bob Clary [:bc] (inactive)]

(In reply to Bob Clary [:bc:] from comment #85)

> > >  
> > > -    def removeDir(self, dirname):
> > 
> > removeDir is called from a few places in the base class -- is it really safe
> > to remove?
> 

It is called but to delete the xpc-other temporary log directory which also happens to be created on the host and not the device. There are several others like this. Thanks! This was a great catch. I'll revisit this patch and try again.

Comment 88

[Bob Clary [:bc] (inactive)]

Attachment: remotexpcshelltests.patch

2nd attempt.

https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&fromchange=de92f6a66cf2a90191549618cf2233315a47bfa9&tochange=941a74fd15777fa9900cda8ec3ebedb398043377

Comment 89

[Geoff Brown [:gbrown]]

Comment on attachment 8966649 [details] [diff] [review]
remotexpcshelltests.patch

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

Looks good. You may need some minor rebasing on top of bug 1389805.

::: testing/xpcshell/remotexpcshelltests.py
@@ +151,5 @@
>          cmd.insert(1, self.remoteHere)
>          cmd = ADBDevice._escape_command_line(cmd)
>          try:
> +            # env is ignored here since the environment has already been
> +            # set for the command via the pushWrapper method.

Perfect comment - thanks!

@@ +211,5 @@
>          else:
>              return -1
>  
>      def removeDir(self, dirname):
> +        self.log.info('removeDir("%s") called.' % dirname)

Did you intend to leave this logging?

Comment 90

[Bob Clary [:bc] (inactive)]

I actually did but I think I'll remove it.

Comment 91

[Bob Clary [:bc] (inactive)]

Attachment: remotexpcshelltests.patch updated

Comment 92

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cabfd7143978
make sure to use root=True with chmod /sdcard/tests/, r=gbrown.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bdb7ce278c1
make sure to use root=True when determining/creating test_root, r=gbrown
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e0d41ace77d
update remotexpcshelltests.py to support hardware, r=gbrown

Comment 93

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/cabfd7143978
https://hg.mozilla.org/mozilla-central/rev/7bdb7ce278c1
https://hg.mozilla.org/mozilla-central/rev/3e0d41ace77d

Comment 95

[Bob Clary [:bc] (inactive)]

Attachment: jittests.patch

Explicitly set root=True for device rm,mkdir,chmod making sure to chmod after pushing new content.

First run is without the patch which shows failures mkdir: '/data': File exists for the su-rooted Pixels at Bitbar. The second run is with this patch. Unfortunately I don't see any change in the failure counts.

https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&fromchange=89610fc7a83649d7d0212721099817879ef60d6f&tochange=ebed2683e1533023a813ac50e6b332e809089ae5

jonco: Do you have a clue about the failures in the second run?

Comment 96

[Geoff Brown [:gbrown]]

Comment on attachment 8968614 [details] [diff] [review]
jittests.patch

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

::: js/src/tests/lib/jittests.py
@@ +706,5 @@
>      for local_file in progs:
>          remote_file = posixpath.join(options.remote_test_root,
>                                       os.path.basename(local_file))
>          device.push(local_file, remote_file)
> +        device.chmod(remote_file, root=True)

nit: Instead of chmod'ing every file, you could chmod(options.remote_test_root, recursive=True, root=True) at the end...probably more efficient when chmod -r is available.

@@ +737,4 @@
>  
> +    jtd_tests = posixpath.join(jit_tests_dir, 'tests')
> +    init_remote_dir(device, jtd_tests)
> +    device.push(JS_TESTS_DIR, jtd_tests, timeout=600)

nit: How long does the push take typically? I know it took at least several minutes years ago, but modern adb performance is much better. If you are seeing << push times, consider reducing the timeout.

Comment 97

[Bob Clary [:bc] (inactive)]

(In reply to Geoff Brown [:gbrown] from comment #96)

> 
> nit: Instead of chmod'ing every file, you could
> chmod(options.remote_test_root, recursive=True, root=True) at the
> end...probably more efficient when chmod -r is available.

Yeah. I couldn't decide. One way I could keep chmodding already existing files, the other would require the chmod for each file which might take a long time. I forgot about us detecting -r support (doh!). I ended up doing the individual chmod if the files were pushed individually.

> 
> @@ +737,4 @@
> >  
> > +    jtd_tests = posixpath.join(jit_tests_dir, 'tests')
> > +    init_remote_dir(device, jtd_tests)
> > +    device.push(JS_TESTS_DIR, jtd_tests, timeout=600)
> 
> nit: How long does the push take typically? I know it took at least several
> minutes years ago, but modern adb performance is much better. If you are
> seeing << push times, consider reducing the timeout.

I don't know. This was an existing limit I carried forward. If I recall correctly, there was a bug comment there were cases where there were a couple of thousand files to be pushed. Fortunately I did use chmod on the directory in this case. ;-)

Comment 98

[Jon Coppeard (:jonco)]

Comment on attachment 8968614 [details] [diff] [review]
jittests.patch

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

This is concerning, but unrelated to this patch AFAICS.

From looking at treederder it seems we don't currently run jittests on ARM hardware, only on the simulator.  Do you know if that's true?  If so then these failures look like bugs we need to investigate.

Comment 99

[Bob Clary [:bc] (inactive)]

Apart from some limited performance testing in Autophone, we do not test on any hardware at the moment. The Jit tests do not currently run even on Android emulators probably due to the existing failures which will also prevent them from being run in the future on either emulators or hardware.

Comment 100

[Jon Coppeard (:jonco)]

(In reply to Bob Clary [:bc:] from comment #99)
Cheers, I filed bug 1454918.

Comment 101

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/94dbb9a11fb3
handle su-rooted devices, r=gbrown, jonco

Comment 102

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/94dbb9a11fb3

Comment 103

[Geoff Brown [:gbrown]]

Attachment: convert marionette tests to adb.py

There are some places where marionette/mozrunner constructs its own commands, rather than using the launch_ functions; I would like to improve that separately in another bug.

I still need to handle bug 1456520, but otherwise, tests are running well:

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

Comment 104

[Bob Clary [:bc] (inactive)]

Comment on attachment 8970594 [details] [diff] [review]
convert marionette tests to adb.py

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

Overall looks good. I'd like to see the requested changes and answers.

The level of indirection in this stuff is confounding. :-( I guess I'm just too stupid to comprehend it all.

Note for another bug. We'll have to support devices at some point and will probably have to start renaming all of the "Fennec" classes as "FennecEmulator" and add the corresponding "FennecHardware" classes but that is a bug for another day.

::: testing/marionette/harness/marionette_harness/runner/base.py
@@ +872,4 @@
>          device_info = None
>          if self.marionette.instance and self.emulator:
>              try:
> +                device_info = self.marionette.instance.runner.device.device.get_info()

adb.py's get_info doesn't support memtotal. Should we add it?

::: testing/mozbase/mozrunner/mozrunner/application.py
@@ +39,4 @@
>  
>  class RemoteContext(object):
>      __metaclass__ = ABCMeta
> +    _device = None

[1]

@@ +73,4 @@
>          return self._adb
>  
>      @property
> +    def device(self):

[1]

@@ +105,5 @@
>          self._adb = adb_path
>          self.avd_home = avd_home
> +        self.remote_process = app
> +        self.device_serial = device_serial
> +        self._device = ADBAndroid(adb=self.adb, device=device_serial)

1. Since we aren't doing a deferred initialization any more, can we just drop the _device attribute and device @property and just assign to self.device here?

::: testing/mozbase/mozrunner/mozrunner/devices/base.py
@@ +14,4 @@
>  import tempfile
>  import time
>  
> +from mozdevice import ADBHost, ADBProcessError

[1] s/ADBProcessError/ADBError/

@@ +64,5 @@
>          remote_dump_dir = posixpath.join(self.app_ctx.remote_profile, 'minidumps')
>          local_dump_dir = tempfile.mkdtemp()
> +        try:
> +            self.device.pull(remote_dump_dir, local_dump_dir)
> +        except ADBProcessError as e:

1. should be ADBError.

@@ +72,2 @@
>          if os.listdir(local_dump_dir):
> +            self.device.rm(remote_dump_dir, recursive=True)

2. This is a change where the remote_dump_dir is completely removed instead of just emptied. Should we recreate it after removing it?

@@ +92,3 @@
>                  break
>              time.sleep(1)
> +        local_profiles_ini = tempfile.NamedTemporaryFile()

I'm not a fan of abusing open temporary file this way but I see the pattern elsewhere in the file so... leave it.

@@ +192,4 @@
>          if not self.restore:
>              return
>  
> +        if self.device.is_file(remote_path) or self.device.is_dir(remote_path):

3. can use self.device.exists(remote_path) instead of checking if it is a file or dir.

@@ +213,4 @@
>  
> +            for backup_file in self.backup_files:
> +                if self.device.is_file('%s.orig' % backup_file) or \
> +                   self.device.is_dir('%s.orig' % backup_file):

3a. ditto re exists.

::: testing/mozbase/mozrunner/mozrunner/devices/emulator.py
@@ +159,1 @@
>          self.port = int(serial[serial.rindex('-') + 1:])

Since we aren't computing serial any more, we can just eliminate it use self.serial in the self.port line:

self.port = int(self.serial[self.serial.rindex('-') + 1:])

Comment 105

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #104)
> The level of indirection in this stuff is confounding. :-( I guess I'm just
> too stupid to comprehend it all.

I know that feeling.

> ::: testing/marionette/harness/marionette_harness/runner/base.py
> @@ +872,4 @@
> >          device_info = None
> >          if self.marionette.instance and self.emulator:
> >              try:
> > +                device_info = self.marionette.instance.runner.device.device.get_info()
> 
> adb.py's get_info doesn't support memtotal. Should we add it?

I am ambivalent, leaning toward "don't bother". I hadn't noticed the difference.

> @@ +72,2 @@
> >          if os.listdir(local_dump_dir):
> > +            self.device.rm(remote_dump_dir, recursive=True)
> 
> 2. This is a change where the remote_dump_dir is completely removed instead
> of just emptied. Should we recreate it after removing it?

I think it is better not to recreate it. Gecko creates the directory on initialization and we usually consider a missing minidumps directory as a sign of a startup crash. Creating the minidumps directory in the harness could mask that.

Have made the other changes...

Comment 106

[Geoff Brown [:gbrown]]

Attachment: convert marionette tests to adb.py

Comment 107

[Bob Clary [:bc] (inactive)]

Comment on attachment 8970750 [details] [diff] [review]
convert marionette tests to adb.py

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

reviewed diff with previous patch and it looks good.

re memtotal: I agree. It is used in only one place in a log and does so in a safe manner.

re dump directory: good catch. Thanks.

Comment 108

[Geoff Brown [:gbrown]]

Attachment: delete devicemanager, Droid, and associated tests

Everything is converted to adb.py: time to cleanup the old stuff!

Comment 109

[Geoff Brown [:gbrown]]

Attachment: bump mozdevice version to 1.0.0

Increase mozdevice version number. Bump to 1.0.0 to align with ahal's SemVer initiative (already used by mozprofile).

Comment 110

[Bob Clary [:bc] (inactive)]

Comment on attachment 8971049 [details] [diff] [review]
delete devicemanager, Droid, and associated tests

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

looks like you got all the weeds. congratulations!

Comment 111

[Bob Clary [:bc] (inactive)]

Comment on attachment 8971208 [details] [diff] [review]
bump mozdevice version to 1.0.0

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

w00t!

Comment 113

[Geoff Brown [:gbrown]]

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

Comment 114

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a79cd8dc4c1c
Convert Android marionette tests to use adb.py; r=bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad8b8a0eb0d2
Remove DeviceManagerADB and Droid classes from mozdevice; r=bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea92acbc6e59
Bump mozdevice version to 1.0.0; r=bc

Comment 115

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1472f261db23
Follow-up: Update mozrunner's mozdevice requirements; r=me on a CLOSED TREE

Comment 116

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/a79cd8dc4c1c
https://hg.mozilla.org/mozilla-central/rev/ad8b8a0eb0d2
https://hg.mozilla.org/mozilla-central/rev/ea92acbc6e59
https://hg.mozilla.org/mozilla-central/rev/1472f261db23

Comment 117

[Geoff Brown [:gbrown]]

mozdevice 1.0.0 uploaded to pypi.org

Comment 118

[Thom Chiovoloni [:tcsc] (ex-moco)]

Releasing mozdevice 1.0.0 breaks consumers of mozrunner 6.15 which was released with `mozdevice >= 0.37` on dep on pypi.

IDK if there's really a way around that, but since mozrunner 7.0.0 is not yet released on pypi, the only fix is to specify the mozdevice version as < 1 in apps that depend on mozrunner 6.15.

Which isn't a huge deal, but it's probably better if you release 7.0.0. So, please release mozrunner 7.0.0. Or whoever has the ability to do that should.

Comment 119

[Geoff Brown [:gbrown]]

(In reply to Thom Chiovoloni [:tcsc] from comment #118)
> Which isn't a huge deal, but it's probably better if you release 7.0.0. So,
> please release mozrunner 7.0.0. Or whoever has the ability to do that should.

Actually, mozrunner 7.0.0 *is* on pypi.

Unfortunately, I updated the mozrunner deps on mozrunner 7.0.0 without bumping the mozrunner version and re-releasing. I think that is the issue: I'll bump to 7.1.0 (or 7.0.1? I'm not sure how to interpret this in semver terms) and release to pypi.

Sorry for the disruption.

Comment 126

[William Lachance (:wlach)]

It seems like these changes broke mozregression. :(

Not asking for any action here, I think the solution is probably for mozregression not to depend on mozdevice (see https://bugzilla.mozilla.org/show_bug.cgi?id=1425102#c0).

Comment 127

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) (use needinfo!) from comment #126)

> Not asking for any action here, I think the solution is probably for
> mozregression not to depend on mozdevice (see
> https://bugzilla.mozilla.org/show_bug.cgi?id=1425102#c0).

Turns out that fixing that is harder than I thought, so back to plan (b) of patching mozdevice.