Related to bug 736112 and bug 735451, we should do similar things for DeviceManagerADB: we should throw an exception if we know the resultant class won't work.

The key change in behaviour is that if we don't have an adb binary OR if we can't get a connection to the device, we'll throw a DMError in the constructor. I also did some minor refactoring while I was in there: 1. Removed Init() method, consolidated all that stuff in the constructor. 2. isUnzipAvailable throws an exception (to be consistent with other verify methods). It is considered non-fatal if we don't have it (we catch the exception in the constructor) 3. Removed a bunch of code which sets/resets self.packageName. We now always set self.packageName, but we only use it if self.useRunAs is true. This simplifies quite a bit of logic while resulting in the same external behaviour. 4. verifyRoot is now an external method in line with everything else in devicemanagerADB, and it throws an exception if it doesn't succeed (again, this exception is considered non-fatal and we handle catching it in the constructor) I've tested this patch with both talos and eideticker and it seems to work ok.

This is exactly the same as the previous patch with some accidentally left-over debugging statements removed

Comment on attachment 606344 [details] [diff] [review] Patch to make DeviceManagerADB throw an exception if it's not going to work (take 2) Review of attachment 606344 [details] [diff] [review]: ----------------------------------------------------------------- the changes look good except the requirement of verifyADB. I am not sure how to proceed on this, other than make it a warning like before. ::: build/mobile/devicemanagerADB.py @@ +29,3 @@ > > + # verify that we can run the adb command. can't continue otherwise > + self.verifyADB() I don't like this. On our foopies we do not have adb available to us and our harnesses require an instance of ADB to be instantiated before we instantiate SUT. I guess we could add in our harnesses a: try: dm = dmADB() except: pass but that won't work if you really want to use adb.

As discussed, I think we can get around this by just passing None instead of a devicemanager to the automation class's constructor. We only use the automation class for getting command line options, so we should be ok just setting it to something later. (In reply to Joel Maher (:jmaher) from comment #3) > Comment on attachment 606344 [details] [diff] [review] > Patch to make DeviceManagerADB throw an exception if it's not going to work > (take 2) > > Review of attachment 606344 [details] [diff] [review]: > ----------------------------------------------------------------- > > the changes look good except the requirement of verifyADB. I am not sure > how to proceed on this, other than make it a warning like before. > > ::: build/mobile/devicemanagerADB.py > @@ +29,3 @@ > > > > + # verify that we can run the adb command. can't continue otherwise > > + self.verifyADB() > > I don't like this. On our foopies we do not have adb available to us and > our harnesses require an instance of ADB to be instantiated before we > instantiate SUT. I guess we could add in our harnesses a: > try: > dm = dmADB() > except: > pass > > but that won't work if you really want to use adb.

(doing a try run to verify this theory)

Try run for 67f9234f935b is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=67f9234f935b Results (out of 41 total builds): exception: 7 success: 25 warnings: 2 failure: 3 other: 4 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wlachance@mozilla.com-67f9234f935b

(In reply to Mozilla RelEng Bot from comment #6) > Try run for 67f9234f935b is complete. > Detailed breakdown of the results available here: > https://tbpl.mozilla.org/?tree=Try&rev=67f9234f935b > Results (out of 41 total builds): > exception: 7 > success: 25 > warnings: 2 > failure: 3 > other: 4 > Builds (or logs if builds failed) available at: > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wlachance@mozilla. > com-67f9234f935b Seems to work. The warnings/failures/exceptions were due to upgrade issues unrelated to this.

New patch which changes tests to not create a DeviceManagerADB() automatically. Should fix issue that jmaher mentioned.

Oops, this causes failures when running robocop tests via adb, locally: mozdev@mozdev-virtual-machine:~/src/objdir-native-droid$ make mochitest-robotium /usr/bin/python2.7 ./build/mobile/robocop/parse_ids.py -i ./mobile/android/base/R.java -o ./build/mobile/robocop/fennec_ids.txt Android Debug Bridge version 1.0.29 unable to connect to 192.168.0.70:20701:20701 434 KB/s (20654 bytes in 0.046s) Traceback (most recent call last): File "_tests/testing/mochitest/runtestsremote.py", line 519, in <module> main() File "_tests/testing/mochitest/runtestsremote.py", line 417, in main dm = devicemanagerADB.DeviceManagerADB(options.deviceIP, options.devicePort) File "/home/mozdev/src/objdir-native-droid/_tests/testing/mochitest/devicemanagerADB.py", line 42, in __init__ self.verifyRunAs() File "/home/mozdev/src/objdir-native-droid/_tests/testing/mochitest/devicemanagerADB.py", line 771, in verifyRunAs if self.useDDCopy: AttributeError: DeviceManagerADB instance has no attribute 'useDDCopy' No such device 192.168.0.70:20701 make: *** [mochitest-robotium] Error 1

Easy fix!

Comment on attachment 607042 [details] [diff] [review] simple patch to avoid exception in verifyRunAs Yup, I noticed this myself a bit later... this is exactly the right fix.

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/531fc2570841