: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.

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 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.

(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.

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.

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 on attachment 8956843 [details] [diff] [review] support adb install -r r+ lgtm.

Comment on attachment 8956845 [details] [diff] [review] add ADBAndroid.get_top_activity() also lgtm. r+

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 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.

(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.

Pushed by gbrown@mozilla.com: 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

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

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

Mochitest and reftest looking good. robocop failing on testSessionOOMSave, but otherwise okay: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d543f9d3510e269e5fd545ed71b9ca149af6e46

(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!

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

Tested locally with mach against 4.3 and 7.0 emulators, and https://treeherder.mozilla.org/#/jobs?repo=try&revision=dba53c71b2166478b33d268dda4d2dc24af5a115

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

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?

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

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

(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 ?

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

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 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 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?

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 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)

(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 on attachment 8959159 [details] [diff] [review] remove "remote b2g" capability from mozversion Review of attachment 8959159 [details] [diff] [review]: ----------------------------------------------------------------- lgtm. r+

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+.

Thanks for all your suggestions - this is definitely improving!

(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 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)

(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.

Pushed by gbrown@mozilla.com: 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

(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.

(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

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.

(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.

(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?

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.

(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 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.

(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

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

Pushed by gbrown@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6ec4bc1d3720 Convert Android xpcshell test harness to adb.py; r=bc

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

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 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.

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 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=bclary@mozilla.com&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=bclary@mozilla.com&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+

(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!

(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!

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 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

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.

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

Pushed by gbrown@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/aad77f9bd093 Convert Android browser test harnesses to adb.py; r=bc

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)

Pushed by gbrown@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7db3ccb6382a Convert jittest to adb.py; r=bc

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

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=bclary@mozilla.com&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 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...

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 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.

(In reply to Bob Clary [:bc:] from comment #76) > Seems ok but wouldn't you rather use launch_fennec? Yes - thanks! Will change...

Pushed by gbrown@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/cb8ded0a5654 Convert Android mach commands to adb.py; r=bc

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

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 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.

Yes, at least for now.

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.

(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.

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?

(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.

2nd attempt. https://treeherder.mozilla.org/#/jobs?repo=try&author=bclary@mozilla.com&fromchange=de92f6a66cf2a90191549618cf2233315a47bfa9&tochange=941a74fd15777fa9900cda8ec3ebedb398043377

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?

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

Pushed by bclary@mozilla.com: 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

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

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=bclary@mozilla.com&fromchange=89610fc7a83649d7d0212721099817879ef60d6f&tochange=ebed2683e1533023a813ac50e6b332e809089ae5 jonco: Do you have a clue about the failures in the second run?

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.

(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 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.

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.

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

Pushed by bclary@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/94dbb9a11fb3 handle su-rooted devices, r=gbrown, jonco

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 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:])

(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 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.

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

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

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 on attachment 8971208 [details] [diff] [review] bump mozdevice version to 1.0.0 Review of attachment 8971208 [details] [diff] [review]: ----------------------------------------------------------------- w00t!

Pushed by gbrown@mozilla.com: 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

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

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

mozdevice 1.0.0 uploaded to pypi.org

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.

(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.

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).

(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.