Closed Bug 1145680 Opened 10 years ago Closed 9 years ago

[mozdevice] Add reboot, move, copy, and info methods to adb.py

Categories

(Testing :: Mozbase, defect)

Product:
Testing
Component:
Mozbase
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox44 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: davehunt, Assigned: davehunt)

References

Details

Attachments

(1 file, 10 obsolete files)

Add copy, move, and getInfo to adb.py v2.7
62.25 KB, patch
bc
: review+
Details | Diff | Splinter Review
In order to drop adb.py in as a replacement for devicemanagerADB.py we should add the following methods: reboot (there's an Android implementation, we need one for B2G) copy (equivalent of copyTree in devicemanager.py) move (equivalent of moveTree in devicemanager.py) info (equivalent of getInfo in devicemanager.py)
BC: I'm happy to work on this, do you have any suggestions before I start?
Blocks: 1143834
Flags: needinfo?(bob)
We should create an adb_b2g.py where we can put b2g specific stuff. The adb_android.py contains the android reboot but really the only android specific code is contained in is_device_ready(). I think a better approach would be to put a basic reboot/is_device_ready in adb.py. Perhaps making is_device_ready abstract so it can be overridden in adb_android.py and adb_b2g.py. copyTree, moveTree and perhaps even getInfo look like they can be implemented with basic shell commands that may be available in both android and b2g. If so, lets put them in adb.py. If not, we can put the android/b2bg specific code in adb_android.py/adb_b2g.py as appropriate. wlach, what do you think?
Flags: needinfo?(bob) → needinfo?(wlachance)
All this makes sense to me.
Flags: needinfo?(wlachance)
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7263e30f0b94 I couldn't find any tests for adb.py, are there any?
Assignee: nobody → dave.hunt
Status: NEW → ASSIGNED
Attachment #8587435 - Flags: review?(bob)
(In reply to Dave Hunt (:davehunt) from comment #4) > I couldn't find any tests for adb.py, are there any? No. :-(
Comment on attachment 8587435 [details] [diff] [review] Add copy, move, and getInfo to adb.py v1.0 Review of attachment 8587435 [details] [diff] [review]: ----------------------------------------------------------------- Initial comments... missing adb_b2g.py? ::: testing/mozbase/mozdevice/mozdevice/adb_android.py @@ +363,1 @@ > """ADBAndroid provides all of the methods of :class:`mozdevice.ADB` with why the change in order?
Forgot to add the adb_b2g.py change in my previous patch.
Attachment #8587435 - Attachment is obsolete: true
Attachment #8587435 - Flags: review?(bob)
Attachment #8587478 - Flags: review?(bob)
Comment on attachment 8587478 [details] [diff] [review] Add copy, move, and getInfo to adb.py v1.1 Review of attachment 8587478 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mozbase/mozdevice/mozdevice/adb.py @@ +1738,5 @@ > + """ > + source = posixpath.normpath(source) > + destination = posixpath.normpath(destination) > + if self.is_dir(source, timeout=timeout): > + self.shell('cp -R %s %s' % (source, destination), timeout=timeout, shell is a low level method that isn't intended to be used directly unless you handle the returned ADBProcess properly. Reading https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozdevice/mozdevice/adb.py#912 shows how much is going on in shell that is handled by the other users of shell. shell_bool is probably more appropriate here. cp isn't available on Android 2.3 from what I can tell. We need to implement a fallback that recursively copies the directory if cp isn't available. @@ +1741,5 @@ > + if self.is_dir(source, timeout=timeout): > + self.shell('cp -R %s %s' % (source, destination), timeout=timeout, > + root=root) > + else: > + self.shell('dd if=%s of=%s' % (source, destination), ditto shell_bool... @@ +1763,5 @@ > + > + """ > + source = posixpath.normpath(source) > + destination = posixpath.normpath(destination) > + self.shell('mv %s %s' % (source, destination), timeout=timeout, ditto shell_bool... @@ +1800,5 @@ > + :raises: * ADBTimeoutError > + * ADBError > + > + """ > + return wlach: Do we need to implement abstractmethod the way devicemanager.py does and decorate these methods? Or will raising NotImplementedError be sufficient ? I don't want someone to accidentally use adb.py and use these methods which aren't implemented. @@ +1816,5 @@ > + :raises: * ADBTimeoutError > + * ADBError > + > + """ > + return ditto abstract method/exception here. @@ +1827,5 @@ > + throwing an ADBTimeoutError. > + This timeout is per adb call. The total time spent > + may exceed this value. If it is not specified, the value > + set in the ADBDevice constructor is used. > + :returns: battery charge as a percentage. :returns: the total memory available. what units? @@ +1832,5 @@ > + :raises: * ADBTimeoutError > + * ADBError > + > + """ > + return ditto abstract method/exception here. This is only implemented in abd_b2g.py which leaves ADBAndroid with an unimplemented abstract method. Either we should implement this for both android and b2g or not make it an abstract method and move it to adb_b2g.ADBB2G ::: testing/mozbase/mozdevice/mozdevice/adb_android.py @@ +358,5 @@ > self.reboot(timeout=timeout) > return output > > > +class ADBAndroid(ADBAndroidMixin, ADBDevice): I'm sure you are changing the order here for a reason. Can you explain why the original order is wrong? I'm not looking for a docstring change or anything, I just want to understand the rationale. ::: testing/mozbase/mozdevice/mozdevice/adb_b2g.py @@ +38,5 @@ > + throwing an ADBTimeoutError. > + This timeout is per adb call. The total time spent > + may exceed this value. If it is not specified, the value > + set in the ADBDevice constructor is used. > + :returns: battery charge as a percentage. :returns: total memory as a percentage. units? @@ +69,5 @@ > + return self.shell_bool('ls /sbin', timeout=timeout) > + > + > +class ADBB2G(ADBB2GMixin, ADBDevice): > + """ADBB2G provides all of the methods of :class:`mozdevice.ADB` with There is no class ADB. use :class:`mozdevice.ADBDevice`
Attachment #8587478 - Flags: review?(bob) → review-
wlach: ni on the abstractmethod question.
Flags: needinfo?(wlachance)
jgriffin: I remember us discussing the conditions which would indicate that a b2g device is ready to be used. davehunt's patch uses return self.shell_bool('ls /sbin', timeout=timeout) to indicate if the b2g device is ready. But if I recall correctly, we had discussed whether marionette's availability would be a better indicator. Thoughts?
Flags: needinfo?(jgriffin)
It depends on what we mean by 'booted'. Marionette's availability means that the b2g process has started, although the UI won't necessarily be available yet. In most contexts, it probably makes sense to use that instead of 'ls /sbin'; I can't think of any cases where we'd need to do something earlier in the boot process.
Flags: needinfo?(jgriffin)
(In reply to Bob Clary [:bc:] from comment #9) > @@ +1800,5 @@ > > + :raises: * ADBTimeoutError > > + * ADBError > > + > > + """ > > + return > > wlach: Do we need to implement abstractmethod the way devicemanager.py does > and decorate these methods? Or will raising NotImplementedError be > sufficient ? > > I don't want someone to accidentally use adb.py and use these methods which > aren't implemented. I think it might be a smart idea, but I don't think it's strictly required. Now that we require python 2.7, we could also consider using the abstract base class (I haven't tried it so I don't know how well it works): https://docs.python.org/2/library/abc.html I agree with you that we should at least raise a NotImplementedError rather than silent failing.
Flags: needinfo?(wlachance)
(In reply to Bob Clary [:bc:] from comment #9) > Comment on attachment 8587478 [details] [diff] [review] > Add copy, move, and getInfo to adb.py v1.1 > > Review of attachment 8587478 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: testing/mozbase/mozdevice/mozdevice/adb.py > @@ +1738,5 @@ > > + """ > > + source = posixpath.normpath(source) > > + destination = posixpath.normpath(destination) > > + if self.is_dir(source, timeout=timeout): > > + self.shell('cp -R %s %s' % (source, destination), timeout=timeout, > > shell is a low level method that isn't intended to be used directly unless > you handle the returned ADBProcess properly. Reading > https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozdevice/ > mozdevice/adb.py#912 shows how much is going on in shell that is handled by > the other users of shell. > > shell_bool is probably more appropriate here. Noted, thanks! > cp isn't available on Android 2.3 from what I can tell. We need to implement > a fallback that recursively copies the directory if cp isn't available. Did you mean the -R option specifically, or there's no way to copy via a remote shell on 2.3 at all? > @@ +1827,5 @@ > > + throwing an ADBTimeoutError. > > + This timeout is per adb call. The total time spent > > + may exceed this value. If it is not specified, the value > > + set in the ADBDevice constructor is used. > > + :returns: battery charge as a percentage. > > :returns: the total memory available. > > what units? I'll clean this up and add units, however I can't see any option other than assuming that the units will not change. The current value includes a suffix indicating the units - would that suffice? > ::: testing/mozbase/mozdevice/mozdevice/adb_android.py > @@ +358,5 @@ > > self.reboot(timeout=timeout) > > return output > > > > > > +class ADBAndroid(ADBAndroidMixin, ADBDevice): > > I'm sure you are changing the order here for a reason. Can you explain why > the original order is wrong? I'm not looking for a docstring change or > anything, I just want to understand the rationale. This was so that the methods in the Mixin take precedent over the pseudo base class. I would prefer an abstract base class though, which you've indicated would be your preferred option too, so I can look into that. > @@ +69,5 @@ > > + return self.shell_bool('ls /sbin', timeout=timeout) > > + > > + > > +class ADBB2G(ADBB2GMixin, ADBDevice): > > + """ADBB2G provides all of the methods of :class:`mozdevice.ADB` with > > There is no class ADB. > > use :class:`mozdevice.ADBDevice` Oops, sorry. (In reply to Jonathan Griffin (:jgriffin) from comment #12) > It depends on what we mean by 'booted'. Marionette's availability means > that the b2g process has started, although the UI won't necessarily be > available yet. In most contexts, it probably makes sense to use that > instead of 'ls /sbin'; I can't think of any cases where we'd need to do > something earlier in the boot process. I'll update the patch to wait for Marionette to be available, however I suspect this would mean assuming and hard-coding the Marionette port. Is there another way? I could wait for the available message to appear in the logcat? (In reply to William Lachance (:wlach) from comment #13) > (In reply to Bob Clary [:bc:] from comment #9) > > > @@ +1800,5 @@ > > > + :raises: * ADBTimeoutError > > > + * ADBError > > > + > > > + """ > > > + return > > > > wlach: Do we need to implement abstractmethod the way devicemanager.py does > > and decorate these methods? Or will raising NotImplementedError be > > sufficient ? > > > > I don't want someone to accidentally use adb.py and use these methods which > > aren't implemented. > > I think it might be a smart idea, but I don't think it's strictly required. > Now that we require python 2.7, we could also consider using the abstract > base class (I haven't tried it so I don't know how well it works): > > https://docs.python.org/2/library/abc.html > > I agree with you that we should at least raise a NotImplementedError rather > than silent failing. I'd like to use abc, and even used it in an earlier version of this patch, but decided not to refactor too much. It sounds like you're open to such a refactor, so I'll revisit this in an updated patch. Thanks!
I've included some documentation updates here too. I haven't yet addressed your Android 2.3 copy comment as I'd appreciate a little more information. I'm thinking I'd like to move the Marionette availability checking out to a separate bug, if that's okay? Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0eba222413d
Attachment #8587478 - Attachment is obsolete: true
Attachment #8589095 - Flags: review?(bob)
(In reply to Dave Hunt (:davehunt) from comment #14) > > > cp isn't available on Android 2.3 from what I can tell. We need to implement > > a fallback that recursively copies the directory if cp isn't available. > > Did you mean the -R option specifically, or there's no way to copy via a > remote shell on 2.3 at all? On my nexus-s 2.3 devices, there is no cp. I think you can recurse using mkdir/dd and implement a replacement for cp [-R]. I think it would be good to implement a check for cp in ADBDevice in a similar fashion that was done for ls and cache the value perhaps just a a boolean like _has_cp. Then later you can use cp if it exists. I think we can presume that if cp is available it supports -R unless we find out otherwise later. > > > @@ +1827,5 @@ > > > + throwing an ADBTimeoutError. > > > + This timeout is per adb call. The total time spent > > > + may exceed this value. If it is not specified, the value > > > + set in the ADBDevice constructor is used. > > > + :returns: battery charge as a percentage. > > > > :returns: the total memory available. > > > > what units? > > I'll clean this up and add units, however I can't see any option other than > assuming that the units will not change. The current value includes a suffix > indicating the units - would that suffice? > I think so. I was mostly looking for a comment in the docstring about what was returned. > > ::: testing/mozbase/mozdevice/mozdevice/adb_android.py > > @@ +358,5 @@ > > > self.reboot(timeout=timeout) > > > return output > > > > > > > > > +class ADBAndroid(ADBAndroidMixin, ADBDevice): > > > > I'm sure you are changing the order here for a reason. Can you explain why > > the original order is wrong? I'm not looking for a docstring change or > > anything, I just want to understand the rationale. > > This was so that the methods in the Mixin take precedent over the pseudo > base class. I would prefer an abstract base class though, which you've > indicated would be your preferred option too, so I can look into that. > Well, I wasn't so much asking for the abc stuff as at least something like what devicemanager did. If the abc stuff isn't too intrusive we can do that. > > (In reply to Jonathan Griffin (:jgriffin) from comment #12) > > It depends on what we mean by 'booted'. Marionette's availability means > > that the b2g process has started, although the UI won't necessarily be > > available yet. In most contexts, it probably makes sense to use that > > instead of 'ls /sbin'; I can't think of any cases where we'd need to do > > something earlier in the boot process. > > I'll update the patch to wait for Marionette to be available, however I > suspect this would mean assuming and hard-coding the Marionette port. Is > there another way? I could wait for the available message to appear in the > logcat? > Could you just add a keyword argument to ADBB2G with the appropriate default? > (In reply to William Lachance (:wlach) from comment #13) > > (In reply to Bob Clary [:bc:] from comment #9) > > > > > @@ +1800,5 @@ > > > > + :raises: * ADBTimeoutError > > > > + * ADBError > > > > + > > > > + """ > > > > + return > > > > > > wlach: Do we need to implement abstractmethod the way devicemanager.py does > > > and decorate these methods? Or will raising NotImplementedError be > > > sufficient ? > > > > > > I don't want someone to accidentally use adb.py and use these methods which > > > aren't implemented. > > > > I think it might be a smart idea, but I don't think it's strictly required. > > Now that we require python 2.7, we could also consider using the abstract > > base class (I haven't tried it so I don't know how well it works): > > > > https://docs.python.org/2/library/abc.html > > > > I agree with you that we should at least raise a NotImplementedError rather > > than silent failing. > > I'd like to use abc, and even used it in an earlier version of this patch, > but decided not to refactor too much. It sounds like you're open to such a > refactor, so I'll revisit this in an updated patch. Thanks! If we want to change to abc abstract classes, lets do it in another bug. We can reuse devicemanager's approach for now. I'll take a look at your new patch next. (In reply to Dave Hunt (:davehunt) from comment #15) > Created attachment 8589095 [details] [diff] [review] > Add copy, move, and getInfo to adb.py v2.0 > > I've included some documentation updates here too. I haven't yet addressed > your Android 2.3 copy comment as I'd appreciate a little more information. > I'm thinking I'd like to move the Marionette availability checking out to a > separate bug, if that's okay? > Sure. > Try: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0eba222413d
Comment on attachment 8589095 [details] [diff] [review] Add copy, move, and getInfo to adb.py v2.0 Review of attachment 8589095 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for updating the methods in mozdevice.rst. The sphinx output looks ok to me. Autophone seems to like the patch during a local test. I wonder if you need to keep name compatibility with devicemanager on the methods copy and move? In the other methods like rm and mkdir I made the method names reflect the actual shell command they were implementing/emulating. Would you mind changing the names to cp and mv ? Also, while you are in here, would you mind removing the existing blank lines just before the terminating """ in the docstrings? I introduced them since emacs adds them when reformatting the docstrings, but mcote has asked me to remove them elsewhere. This seems like a good opportunity while we are in here. I would really like to see the recursive implementation of copy/cp in this patch. I'm going to clear the r? so I can re-review the final patch but this looks good. Thanks. ::: testing/mozbase/mozdevice/mozdevice/adb.py @@ +503,1 @@ > """ I think I'd like to split the docstring amongst the various classes. Perhaps something like: """ADBDevice is an abstract base class which provides methods which can be used to interact with the associated Android or B2G based device. It must be used via one of the concrete implementations in :class:`ADBAndroid` or :class:`ADBB2G`. """ @@ +1750,5 @@ > + > + """ > + source = posixpath.normpath(source) > + destination = posixpath.normpath(destination) > + if self.is_dir(source, timeout=timeout): We should pass root=root in case it is needed to access the directory. Will you be implementing the recursive dd implementation for devices that don't have cp in this bug? @@ +1755,5 @@ > + self.shell_bool('cp -R %s %s' % (source, destination), > + timeout=timeout, root=root) > + else: > + self.shell_bool('dd if=%s of=%s' % (source, destination), > + timeout=timeout, root=root) If cp is available, why are we using dd instead of cp here? @@ +1800,5 @@ > + self.command_output(["reboot"], timeout=timeout) > + self.command_output(["wait-for-device"], timeout=timeout) > + return self.is_device_ready(timeout=timeout) > + > + def is_device_ready(self, timeout=None): This should be an abstract method. @@ +1801,5 @@ > + self.command_output(["wait-for-device"], timeout=timeout) > + return self.is_device_ready(timeout=timeout) > + > + def is_device_ready(self, timeout=None): > + """Returns True if the device is ready. Should mention it is an abstract method to be over-ridden. @@ +1813,5 @@ > + :raises: * ADBTimeoutError > + * ADBError > + > + """ > + return self.shell_bool('ls /sbin', timeout=timeout) just return here since it will be abstract. ::: testing/mozbase/mozdevice/mozdevice/adb_android.py @@ +15,1 @@ > """ADBAndroid implements :class:`ADBDevice` providing Android-specific functionality. :: from mozdevice import ADBAndroid adbdevice = ADBAndroid() print adbdevice.list_files("/mnt/sdcard") if adbdevice.process_exist("org.mozilla.fennec"): print "Fennec is running" """ ::: testing/mozbase/mozdevice/mozdevice/adb_b2g.py @@ +8,5 @@ > + > + > +class ADBB2G(ADBDevice): > + """ADBDevice implementation with B2G-specific functionality""" > + """ADBB2G implements :class:`ADBDevice` providing B2G-specific functionality. :: from mozdevice import ADBB2G adbdevice = ADBB2G() print adbdevice.list_files("/mnt/sdcard") if adbdevice.process_exist("b2g"): print "B2G is running" """ @@ +20,5 @@ > + may exceed this value. If it is not specified, the value > + set in the ADBDevice constructor is used. > + :returns: battery charge as a percentage. > + :raises: * ADBTimeoutError > + * ADBError I wonder if we should either list the possible exceptions that open/read can raise or if we should catch them and wrap them in ADBErrors. @@ +40,5 @@ > + may exceed this value. If it is not specified, the value > + set in the ADBDevice constructor is used. > + :returns: memory total with units. > + :raises: * ADBTimeoutError > + * ADBError ditto: I wonder if we should either list the possible exceptions that open/read can raise or if we should catch them and wrap them in ADBErrors. @@ +86,5 @@ > + directives = [directive] > + > + if 'memtotal' in directives: > + info['memtotal'] = self.get_memory_total(timeout=timeout) > + return info why not something like: if directive == 'memtotal': return {'memtotal': self.get_memory_total(timeout=timeout)} return super(ADBB2G, self).get_info(directive=directive, timeout=timeout)
Attachment #8589095 - Flags: review?(bob)
wlach, do you have an opinion on whether we should wrap the possible exceptions from open/read in ADBError or if we should just list the possible exceptions in the raises list?
Flags: needinfo?(wlachance)
Had to think about this for a bit and look up resources on the subject. I suspect we should wrap the possible exceptions, as the fact that we use open/read internally are implementation details which we should not expose to the user of the class. Some useful resources I found on the subject: http://stackoverflow.com/questions/839636/best-practices-for-python-exceptions http://eli.thegreenplace.net/2008/08/21/robust-exception-handling/
Flags: needinfo?(wlachance)
wlach, thanks! That makes perfect sense when you frame in terms of hiding the implementation details. dhunt: ok, lets wrap any possible exceptions.
Only requesting feedback as the patch is not yet complete. (In reply to Bob Clary [:bc:] from comment #17) > I wonder if you need to keep name compatibility with devicemanager on the > methods copy and move? In the other methods like rm and mkdir I made the > method names reflect the actual shell command they were > implementing/emulating. Would you mind changing the names to cp and mv ? I've made this change, however 'cp' might be misleading as the shell command is either cp or dd depending on whether the target is a file or directory. > Also, while you are in here, would you mind removing the existing blank > lines just before the terminating """ in the docstrings? I introduced them > since emacs adds them when reformatting the docstrings, but mcote has asked > me to remove them elsewhere. This seems like a good opportunity while we are > in here. Done. > I would really like to see the recursive implementation of copy/cp in this > patch. I can take a stab at this, however I don't have a suitable test device. I'll test the code path on my B2G device though. > @@ +1755,5 @@ > > + self.shell_bool('cp -R %s %s' % (source, destination), > > + timeout=timeout, root=root) > > + else: > > + self.shell_bool('dd if=%s of=%s' % (source, destination), > > + timeout=timeout, root=root) > > If cp is available, why are we using dd instead of cp here? In my testing, cp only works for directories, if the source is a file we need to use dd. > @@ +1800,5 @@ > > + self.command_output(["reboot"], timeout=timeout) > > + self.command_output(["wait-for-device"], timeout=timeout) > > + return self.is_device_ready(timeout=timeout) > > + > > + def is_device_ready(self, timeout=None): > > This should be an abstract method. I can make it an abstract method, but at this time it doesn't need to be. This could be considered a minimal device ready that would work for any implementation. We'll ultimately have a B2G specific implementation that waits for Marionette, but if you want I can move this basic implementation into the ADBB2G implementation. > @@ +20,5 @@ > > + may exceed this value. If it is not specified, the value > > + set in the ADBDevice constructor is used. > > + :returns: battery charge as a percentage. > > + :raises: * ADBTimeoutError > > + * ADBError > > I wonder if we should either list the possible exceptions that open/read can > raise or if we should catch them and wrap them in ADBErrors. I'm now catching IOError and re-raising it as ADBError. Is this what you had in mind? > @@ +86,5 @@ > > + directives = [directive] > > + > > + if 'memtotal' in directives: > > + info['memtotal'] = self.get_memory_total(timeout=timeout) > > + return info > > why not something like: > > if directive == 'memtotal': > return {'memtotal': self.get_memory_total(timeout=timeout)} > > return super(ADBB2G, self).get_info(directive=directive, > timeout=timeout) We need to return a combined dictionary including memtotal if directive is None. Also, this allows for there to be additional implementation specific directives.
Attachment #8589095 - Attachment is obsolete: true
Attachment #8590116 - Flags: feedback?(bob)
(In reply to Dave Hunt (:davehunt) from comment #21) I'll reply to your comment here but will do the review/feedback separately. > Created attachment 8590116 [details] [diff] [review] > Add copy, move, and getInfo to adb.py v2.1 > > Only requesting feedback as the patch is not yet complete. > > > I've made this change, however 'cp' might be misleading as the shell command > is either cp or dd depending on whether the target is a file or directory. Well, we are emulating cp to a degree so I think that is ok. I wonder if the inability of cp to copy files that you have seen is a b2g only issue? Busybox's cp copies files ok as does cp on Android 4.2, 4.3, 4.4 on Nexus 4, 5, 7. > > I can take a stab at this, however I don't have a suitable test device. I'll > test the code path on my B2G device though. I can help test it on android. > > > @@ +1755,5 @@ > > > + self.shell_bool('cp -R %s %s' % (source, destination), > > > + timeout=timeout, root=root) > > > + else: > > > + self.shell_bool('dd if=%s of=%s' % (source, destination), > > > + timeout=timeout, root=root) > > > > If cp is available, why are we using dd instead of cp here? > > In my testing, cp only works for directories, if the source is a file we > need to use dd. > This seems like it is a b2g only thing. I don't think we need to worry about an android cp only and a b2g cp+dd solution though. We can code it to the lowest common denominator using cp+dd. Maybe a comment about that would help others understand why you are doing it this way though. > > @@ +1800,5 @@ > > > + self.command_output(["reboot"], timeout=timeout) > > > + self.command_output(["wait-for-device"], timeout=timeout) > > > + return self.is_device_ready(timeout=timeout) > > > + > > > + def is_device_ready(self, timeout=None): > > > > This should be an abstract method. > > I can make it an abstract method, but at this time it doesn't need to be. > This could be considered a minimal device ready that would work for any > implementation. We'll ultimately have a B2G specific implementation that > waits for Marionette, but if you want I can move this basic implementation > into the ADBB2G implementation. > I think I'd like that. ADBDevice should be abstract and ADBAndroid has its own version. Let's put that in the ADBB2G. > > @@ +20,5 @@ > > > + may exceed this value. If it is not specified, the value > > > + set in the ADBDevice constructor is used. > > > + :returns: battery charge as a percentage. > > > + :raises: * ADBTimeoutError > > > + * ADBError > > > > I wonder if we should either list the possible exceptions that open/read can > > raise or if we should catch them and wrap them in ADBErrors. > > I'm now catching IOError and re-raising it as ADBError. Is this what you had > in mind? > I can't find a comprehensive list of exceptions that open/read can raise, but it does seem we could get IOError or OSError; both of which are instances of EnvironmentError. If tf2.read() returned '', tf2.read().splitlines()[0] would return IndexError: list index out of range. Lets just catch Exception and wrap that. > > why not something like: > > > > if directive == 'memtotal': > > return {'memtotal': self.get_memory_total(timeout=timeout)} > > > > return super(ADBB2G, self).get_info(directive=directive, > > timeout=timeout) > > We need to return a combined dictionary including memtotal if directive is > None. Also, this allows for there to be additional implementation specific > directives. Ok. I missed the None input. Thanks.
Comment on attachment 8590116 [details] [diff] [review] Add copy, move, and getInfo to adb.py v2.1 Review of attachment 8590116 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for all of your work on this. I'm sure I'm a pita, but I appreciate working with you on this. ::: testing/mozbase/mozdevice/mozdevice/adb.py @@ +506,5 @@ > > :raises: * ADBError > * ADBTimeoutError > * ValueError > this blank line too. @@ +1744,5 @@ > + set in the ADB constructor is used. > + :raises: * ADBTimeoutError > + * ADBError > + """ > + return self.shell_bool('ls /sbin', timeout=timeout) We discussed this in the comments, but to be redundant: make this abstract and move the implementation to adb_b2g.py. ::: testing/mozbase/mozdevice/mozdevice/adb_b2g.py @@ +40,5 @@ > + try: > + with open(tf.name) as tf2: > + return tf2.read().splitlines()[0] > + except IOError as e: > + raise ADBError(e) As I mentioned in the comments, I think we should catch any Exception and wrap that. Normally in adb*.py, I've been using ADBError('message') to create the exceptions. If we use ADBError(e) like this, then str(ADBError(e)) will only output the message and lose the underlying error type. If you import traceback, you could do something like: >>> try: ... raise Exception('asldkjfasldfj') ... except Exception as e: ... raise ADBError(traceback.format_exception_only(type(e), e)[0].strip()) ... which would be the equivalent of raise ADBError('Exception: asldkjfasldfj') @@ +64,5 @@ > + for line in tf2.read().splitlines(): > + key, value = line.split(':') > + meminfo[key] = value.strip() > + except IOError as e: > + raise ADBError(e) ditto
Attachment #8590116 - Flags: feedback?(bob) → feedback+
This patch includes the check for cp and alternate solution for when it's not available. Could you test it on Android 2.3? I also discovered that cp works fine for files/directories on B2G so not sure what I was seeing before.
Attachment #8590116 - Attachment is obsolete: true
Attachment #8591658 - Flags: review?(bob)
Comment on attachment 8591658 [details] [diff] [review] Add copy, move, and getInfo to adb.py v2.2 Review of attachment 8591658 [details] [diff] [review]: ----------------------------------------------------------------- I made a mistake in my earlier reviews. shell_bool will only return True or False or raise ADBTimeoutError or ADBRootError. It will essentially hide errors due to missing sources, or read only file systems. There are some issues with cp that I'd like to address. Let me work up a patch and we can fold it in with yours. I'll do that right now. ::: testing/mozbase/mozdevice/mozdevice/adb.py @@ +568,5 @@ > + if self.shell_bool("type cp"): > + self._have_cp = True > + except ADBError: > + self._logger.debug("Check for cp failed") > + Interesting use of type. I checked and all of my devices have it available. @@ +1693,5 @@ > + source = posixpath.normpath(source) > + destination = posixpath.normpath(destination) > + if self._have_cp: > + self.shell_bool('cp -R %s %s' % (source, destination), > + timeout=timeout, root=root) Interesting use of cp -R to copy a file. Certainly simplifies things. We could just early return here. @@ +1697,5 @@ > + timeout=timeout, root=root) > + else: > + is_file = self.is_file(source) > + destination_dir = destination > + if is_file: If the source doesn't exist, is_file will be False and we'll end up creating a directory with the destination name. @@ +1711,5 @@ > + else: > + for i in self.list_files(source): > + self.cp(posixpath.join(source, i), > + posixpath.join(destination, i)) > + return True Let me work up a patch for this and we can fold it into yours.
Attachment #8591658 - Flags: review?(bob) → review-
Attached patch bug1145680-followup.patch (obsolete) — — Splinter Review
Comments in the patch. Please review and if this is ok, fold it into your patch and lets get wlach to review.
Attachment #8593067 - Flags: review?(dave.hunt)
Comment on attachment 8593067 [details] [diff] [review] bug1145680-followup.patch Review of attachment 8593067 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mozbase/mozdevice/mozdevice/adb.py @@ +1706,5 @@ > + source) > + > + if self.is_file(source, timeout=timeout, root=root): > + self.shell_output('dd if=%s of=%s' % (source, destination), > + timeout=timeout, root=root) I didn't handle the case where the destination is a directory instead of a file. If destination is a directory, we need to append the base name of the source.
Comment on attachment 8593067 [details] [diff] [review] bug1145680-followup.patch Review of attachment 8593067 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Combined patch coming up. ::: testing/mozbase/mozdevice/mozdevice/adb.py @@ +564,5 @@ > self._ls += " -a" > > # Do we have cp? > + if self.shell_bool("type cp"): > + self._have_cp = True We could simplify this further to: self._have_cp = self.shell_bool("type cp") I'll leave it as it is for now, but let me know if you'd like me to change it.
Attachment #8593067 - Flags: review?(dave.hunt) → review+
Attachment #8591658 - Attachment is obsolete: true
Attachment #8593067 - Attachment is obsolete: true
Attachment #8594704 - Flags: review?(bob)
(In reply to Dave Hunt (:davehunt) from comment #28) > Comment on attachment 8593067 [details] [diff] [review] > We could simplify this further to: > > self._have_cp = self.shell_bool("type cp") That's a great idea, please go ahead and incorporate that in your final patch. I've looked over the patch and it seems fine. I want to do some more manual testing just to make sure we didn't miss anything between cp and dd. I should be able to do that as soon as a current test I am running with my devices finishes. Thanks a lot!
Attachment #8594704 - Attachment is obsolete: true
Attachment #8594704 - Flags: review?(bob)
Attachment #8594826 - Flags: review?(bob)
Comment on attachment 8594826 [details] [diff] [review] Add copy, move, and getInfo to adb.py v2.4 Review of attachment 8594826 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the minor tweak changing the argument of cp from recurse to recursive. Sorry for missing that before. ::: testing/mozbase/mozdevice/mozdevice/adb.py @@ +1668,5 @@ > if proc_name == app[:75]: > return True > return False > + > + def cp(self, source, destination, recurse=False, timeout=None, root=False): My bad. rm uses recursive as an argument. Can you please replace s/recurse/recursive/ as the argument?
Attachment #8594826 - Flags: review?(bob) → review+
Flags: needinfo?(dave.hunt)
Summary: [mozbase] Add reboot, move, copy, and info methods to adb.py → [mozdevice] Add reboot, move, copy, and info methods to adb.py
Flags: needinfo?(dave.hunt)
Keywords: checkin-needed
Backing this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4b883ffea234 to see if it clears up some near permafailing Gij(8) that started happening on that checkin-needed push Ryan did earlier. I previously backed out bug 1154426 and bug 1155634 to see if that would clear things up, but it doesn't look like they did, so I'm moving on to some of the other patches in that push.
Flags: needinfo?(dave.hunt)
Gij(8) went green with this backed out.
I don't think Gij uses ADBDevice, but to be sure I've triggered a full try run. Note that I will need to update this patch once bug 1156812 lands. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ddb00903852
I've replicated the Gij(8) error on my try run but I really don't understand it. To me it looks like a timeout (or crash?) when running the tests. Kevin: Does Gij use the Python mozbase package somehow? How can I replicate this locally? In this patch we add a few methods and change ADBDevice to be an abstract class. This means instead of creating an ADBDevice object we would need to create an ADBAndriod or ADBB2G, but I can't find any references to ADBDevice for Gij...
Flags: needinfo?(dave.hunt) → needinfo?(kgrandon)
(In reply to Dave Hunt (:davehunt) from comment #38) > Kevin: Does Gij use the Python mozbase package somehow? How can I replicate > this locally? In this patch we add a few methods and change ADBDevice to be > an abstract class. This means instead of creating an ADBDevice object we > would need to create an ADBAndriod or ADBB2G, but I can't find any > references to ADBDevice for Gij... I'm not really sure how or why this would cause gaia tests to fail. I'm forwarding this to James/Gareth/Aus who might have a better idea about the inner workings of Gij.
Flags: needinfo?(kgrandon)
Flags: needinfo?(jlal)
Flags: needinfo?(gaye)
Flags: needinfo?(aus)
Gij uses the python stuff to run gecko. :ahal, :lightsofapollo, and I did the work to integrate the python gecko runner into marionette-js-runner on recommendation from :jgriffin (and :ahal) that the python code had good support for devices and crash reporting among other things. You can reproduce any issues locally that you're seeing with Gij on TC by downstreaming your changes through marionette-js-runner and then to gaia. Check out https://developer.mozilla.org/en-US/Firefox_OS/Automated_testing/Gaia_integration_tests for an overview of the jsmarionette stuff.
Flags: needinfo?(gaye)
It sounds like the last two comments should provide enough info, so clearing flags for James and Aus. Please re-flag if needed, thanks.
Flags: needinfo?(jlal)
Flags: needinfo?(aus)
(In reply to Bob Clary [:bc:] from comment #41) > https://github.com/mozilla-b2g/gaia/blob/ > f34ce82a840ad3c0aed3bfff18517b3f6a0eb37f/tests/python/gaia-ui-tests/gaiatest/ > mixins/treeherder.py#L183 > > adb_device = ADBDevice(self.device_serial) This is part of the Gip tests, which are passing on try. Whilst this should be updated, it will only affect the on-device Python tests after a new release of mozdevice. Gareth pointed me towards https://github.com/mozilla-b2g/marionette-js-runner/tree/master/host/python/runner-service/runner_service which is where mozrunner is used to handle running against the emulator. What's strange is that mozrunner uses DeviceManagerADB and not ADBDevice, which is what my patch is changing. Is there a way to get more information from the failure? It looks like a timeout (based on "timeout of 60000ms exceeded") or crash (based on "Crash detected but error running stackwalk") but the traces in the log fail to mention the line in Gaia or mozrunner that they're executing at the time. It's also interesting to note that one of the reruns in the try actually passed...
Flags: needinfo?(gaye)
Gareth suggested I may be able to get a VM from Taskcluster to use for replicating the failure, and thought James would be able to help me to set this up.
Flags: needinfo?(jlal)
I'm able to run the ringtones tests locally and they're not using mozrunner at all (they pass without having it installed) so I'm still fairly certain my patch should not be causing Gij failures here. Bob suggested removing the abstract base class changes from my path in case they are somehow affecting the tests - I'll submit a try run shortly with this change. In the meantime it would be great if I could get someone familiar with the Gij tests to look at the failure and provide more information - is this a timeout or a crash? What line of the test code (or Python module) is failing?
Flags: needinfo?(kgrandon)
(In reply to Dave Hunt (:davehunt) from comment #45) > I'm able to run the ringtones tests locally and they're not using mozrunner > at all (they pass without having it installed) so I'm still fairly certain > my patch should not be causing Gij failures here. Bob suggested removing the > abstract base class changes from my path in case they are somehow affecting > the tests - I'll submit a try run shortly with this change. In the meantime > it would be great if I could get someone familiar with the Gij tests to look > at the failure and provide more information - is this a timeout or a crash? > What line of the test code (or Python module) is failing? Which version of marionette-js-runner are you using and how do you know that the ringtones tests are not using mozrunner?
Flags: needinfo?(gaye)
Oh sorry I didn't read the full comment. Are you sure that they aren't available to the virtualenv marionette-js-runner is using?
This is where I find my mozrunner ftr gareth@albee:~/Documents/gaia/node_modules/marionette-js-runner/venv/bin$ ls activate activate.fish diff-profiles easy_install gaia-integration mozinfo mozrunner pip2 python python2.7 sutini activate.csh activate_this.py dm easy_install-2.7 manifestparser mozprofile pip pip2.7 python2 structlog view-profile
(In reply to Gareth Aye [:gaye] from comment #46) > Which version of marionette-js-runner are you using and how do you know that > the ringtones tests are not using mozrunner? How do I determine this? I'm using an up to date clone of the Gaia repository. (In reply to Gareth Aye [:gaye] from comment #47) > Oh sorry I didn't read the full comment. Are you sure that they aren't > available to the virtualenv marionette-js-runner is using? The make test-integration script makes reference to searching for a ci_venv path, which is not found. I had assumed that without this present any required packages would be installed or there would be a failure. (In reply to Gareth Aye [:gaye] from comment #48) > This is where I find my mozrunner ftr > > gareth@albee:~/Documents/gaia/node_modules/marionette-js-runner/venv/bin$ ls > activate activate.fish diff-profiles easy_install > gaia-integration mozinfo mozrunner pip2 python python2.7 sutini > activate.csh activate_this.py dm easy_install-2.7 > manifestparser mozprofile pip pip2.7 python2 structlog > view-profile I found the same! Thank you, coming from not much node experience it's hard to find this kind of stuff out without being told. I activated the virtual environment, installed a mozdevice running my patch and can now replicate the failure! Thanks again! :)
Flags: needinfo?(kgrandon)
Flags: needinfo?(jlal)
Okay, I can reliably replicate this now by running the following: $ TEST_FILES=./apps/ringtones/test/marionette/pick_test.js make test-integration It appears that the test 'Cancel setting selected sound is failing' in the suite 'Ringtones picker' during setup due to a timeout, and the application shows the homescreen. This is the second suite in the file, and if I comment out the first suite it passes just fine. If I swap the order of the suites, then the second suite still fails. If I remove my patch then the test passes. I've tried reverting various parts of my patch too, but I've so far been unable to prevent the failure for more than a single instance. It would help considerably if I had the ability to log to stdout or have a full stacktrace. I can't see that the use of abstract base classes is the cause as the harness and mozrunner already use this. The harness is using B2GDeviceRunner, which has nothing to do with ADB, and my patch should only affect ADBDevice and friends.
I ran a try without the abstract base class, which passed initially, but failed on a retrigger: https://treeherder.mozilla.org/#/jobs?repo=try&revision=193ebe9225f3 I am now using mozrunner from mozilla-central locally (to attempt to debug), and the tests are passing again even, with my patch applied(!) This should be no different from what TaskCluster is doing... Worth noting that I have seen an alert that B2G is already running and that only one copy can be running at a time. I've also seen that when the homescreen is displayed in the app, closing the app caused it to restart into the settings app and allowed the tests to recover. This would seem to suggest that we have a race condition starting multiple instances of the application, and sometimes an instance without the appropriate profile might be being launched? I'm struggling here, and could do with some help. I need to land this bug as it blocks switching mozrunner to use ADBDevice, which will allow mozrunner to target remote devices such as those in our remote partner lab. We will need this before we can enhance marionette to use the B2GDeviceRunner, which improves crash detection and handling.
Flags: needinfo?(gaye)
Flags: needinfo?(ahalberstadt)
Sorry, I don't have any additional insights to this. I'll mention that between each test Gij starts and stops a new mozrunner instance. They communicate over an http server. The python side of the glue is here: https://github.com/mozilla-b2g/marionette-js-runner/blob/master/host/python/runner-service/runner_service/listener.py For example, before a test is run, that listener will receive a "do_start_runner" action. I'm not sure where that gets sent on the JS side. My only suggestion would be to try and eliminate some of the components. Is this a problem of mozrunner not shutting down fast enough? Is Gij not waiting before sending the next message? Is it something in the communication layer between them going awry? If you think it might be a timing issue, try adding sleeps on both the python and js sides. Also, getting to a point where you can add print statements in python and see them in stdout or a file would be very helpful, maybe :gaye knows how to do this. Sorry I know that's not very helpful, but I'm just as bewildered as you are as to how your patch could trigger this.
Flags: needinfo?(ahalberstadt)
Also it might be good to see if you can cut down your patch to the bare minimum to what reproduces this, and file a new bug with simple STR. My guess is that this problem is unrelated to your patch and you just won the game of hidden failure roulette.
If you want some python output, you could try piping the python process' output to stdout and stderr by setting stdio in https://github.com/mozilla-b2g/marionette-js-runner/blob/master/host/host.js#L100 to something like [0, 1, 2, 2].
Flags: needinfo?(gaye)
In my version in $GAIA/node_modules/marionette-js-runner/host/host.js on gaia master, everything is being swallowed by the js parent proc.
Have you guys thought about trying to cut down on some of the inheritance in your codebase? http://en.wikipedia.org/wiki/Composition_over_inheritance has a bit of discussion. My experience with python mozilla code is that most of it is documentation about methods that don't do anything because they're not implemented by that part of the crazy inheritance tree...
Additionally you may consider submitting a new try run for this and seeing what it does. We were running into a few failures last week due to infra changes. (I'm not sure if it's fixed or not, but worth a shot if you're confident that your patch does not impact Gij).
Latest version of patch, with new try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=638ceccc2eea
Attachment #8595370 - Attachment is obsolete: true
I've replicated the issue without my patch applied and a very simple test. I'll raise a blocking bug.
Depends on: 1159200
Are you sure the issue that you filed is the same one that your patch caused? Do you see a significant difference with and without your patch applied?
Flags: needinfo?(dave.hunt)
The trace is exactly the same. With my patch applied it seems to happen sooner, but I suspect it's some sort of race condition. It's worth noting that the failure intermittently happened even before my patch landed.
Flags: needinfo?(dave.hunt)
I've restored the patch since the blocker is resolved and triggered a try run: try: -b o -p android-api-9,android-api-11,android-x86,emulator,linux64_gecko -u all -t none https://treeherder.mozilla.org/#/jobs?repo=try&revision=1441f9494a64
That try run is looking mighty green. =)
Okay, let's have one final review before landing this. Nothing has changed since the last patch, I've just restored it due to bitrot.
Attachment #8598521 - Attachment is obsolete: true
Attachment #8679339 - Flags: review?(bob)
Comment on attachment 8679339 [details] [diff] [review] Add copy, move, and getInfo to adb.py v2.7 Looks good. I triggered a couple more tests for Gij(24) to make sure it was intermittent.
Attachment #8679339 - Flags: review?(bob) → review+
https://hg.mozilla.org/mozilla-central/rev/40b23b4f5267
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: