736246 - DeviceManagerADB should throw an exception if we know it's not going to work

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[William Lachance (:wlach)]

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.

Comment 1

[William Lachance (:wlach)]

Attachment: Patch to make DeviceManagerADB throw an exception if it's not going to 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.

Comment 2

[William Lachance (:wlach)]

Attachment: Patch to make DeviceManagerADB throw an exception if it's not going to work (take 2)

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

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 4

[William Lachance (:wlach)]

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.

Comment 5

[William Lachance (:wlach)]

(doing a try run to verify this theory)

Comment 6

[Mozilla RelEng Bot]

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/[e-mail]-67f9234f935b

Comment 7

[William Lachance (:wlach)]

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

Comment 8

[William Lachance (:wlach)]

Attachment: Patch to make DeviceManagerADB throw an exception if it's not going to work (take 3)

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1ad80d7f0d2

Comment 10

[Phil Ringnalda (:philor)]

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

Comment 11

[Geoff Brown [:gbrown]]

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

Comment 12

[Geoff Brown [:gbrown]]

Attachment: simple patch to avoid exception in verifyRunAs

Easy fix!

Comment 13

[William Lachance (:wlach)]

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.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/531fc2570841