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

RESOLVED FIXED in mozilla14

Status

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: wlach, Assigned: wlach)

Tracking

Trunk
mozilla14
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
Component: GoFaster → General
QA Contact: gofaster → general
Created attachment 606342 [details] [diff] [review]
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.
Assignee: nobody → wlachance
Attachment #606342 - Flags: review?(jmaher)
OS: Linux → Android
Hardware: x86_64 → All
Created attachment 606344 [details] [diff] [review]
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
Attachment #606342 - Attachment is obsolete: true
Attachment #606342 - Flags: review?(jmaher)
Attachment #606344 - Flags: review?(jmaher)
Depends on: 736112
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.
Attachment #606344 - Flags: review?(jmaher) → review+
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)

Comment 6

7 years ago
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.
Created attachment 606693 [details] [diff] [review]
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.
Attachment #606344 - Attachment is obsolete: true
Attachment #606693 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1ad80d7f0d2
Keywords: checkin-needed
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/e1ad80d7f0d2
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 607042 [details] [diff] [review]
simple patch to avoid exception in verifyRunAs

Easy fix!
Attachment #607042 - Flags: review?(wlachance)
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.
Attachment #607042 - Flags: review?(wlachance) → review+
Keywords: checkin-needed
Whiteboard: check in needed for "simple patch to avoid exception..."
Keywords: checkin-needed
Whiteboard: check in needed for "simple patch to avoid exception..."
https://hg.mozilla.org/mozilla-central/rev/531fc2570841
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.