797897 - DeviceManagerSUT's pushDir does pointless work, slowing things down

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[William Lachance (:wlach)]

Due to the interaction between pushDir and pushFile, we check for the existence of the directories we're trying to push several times. This is annoying. You can see this in the current sut unit tests:

https://github.com/mozilla/mozbase/blob/master/mozdevice/tests/sut_push.py#L47

We should modify pushFile to take an option to NOT attempt to check/create the dir it's writing to, and make pushDir use that. To preserve backwards compatibility, we should make this True by default.

e.g.

def pushFile(self, localname, destname, createDir=True):

It should be possible to fix this bug and verify just by editing the mozdevice sources (devicemanagerSUT.py, and then modifying the unit test I linked to above). You can test your changes by running the following with an activated mozbase virtualenv from the mozbase root:

python test.py mozdevices/tests/manifest.ini

Comment 1

[Stanley Chan]

Hey wlach, I believe this bug should be within my reach. From what I understand from your description, all that's needed to be done is to add the extra parameter in the pushFile method (all of the ones? even located at devicemanager.py, devicemanagerADB.py, and devicemanagerSUT.py?).

I don't know how to edit the unit test, nor test/verify my changes. Can I get some guidance/resources on this?

Thanks.

Comment 2

[Stanley Chan]

Sorry, I was a bit hasty; looking at this helped a bit: https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Getting_Help_with_Mozbase

I still don't fully understand the unit test part though, can I get some help with that?

Comment 3

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

This bug appears to be fixed.  The only optimization I can see is reducing our checking/creating of parent directories.

Comment 4

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #3)
> This bug appears to be fixed.  The only optimization I can see is reducing
> our checking/creating of parent directories.

Nope, this bug very much still exists. It's not a very pressing issue though.

Comment 5

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

please explain, I looked a the code this morning and could not find it.

Comment 6

[William Lachance (:wlach)]

(In reply to Stanley Chan from comment #2)
> Sorry, I was a bit hasty; looking at this helped a bit:
> https://wiki.mozilla.org/Auto-tools/Projects/
> MozBase#Getting_Help_with_Mozbase
> 
> I still don't fully understand the unit test part though, can I get some
> help with that?

Sorry about the terribly late response on this, it must have gotten lost in my bugmail. For posterity, instructions on running the tests are here:

https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Running_the_tests

You'll need to set up a mozbase environment for development first before doing that:

https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Initial_Setup

I think someone else is interested in this bug. If it gets fixed before you have a chance, feel free to pop by #ateam and I'm sure we can find some other mozdevice bug for you to sink your teeth into. :)

Comment 7

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

ok, I was confused by the original intent of this bug.  The problem is that we check for directories in pushDir and pushFile.  In many cases we know the directory exists, so lets make a way to ignore the checking of it.  The main goal here is to reduce the calls we make to the device, the secondary goal is to improve runtime.

Comment 8

[akruglov]

Hi there!
Can I take that one?

I even think that I got patch for it already :)

Comment 9

[William Lachance (:wlach)]

(In reply to akruglov from comment #8)
> Hi there!
> Can I take that one?
> 
> I even think that I got patch for it already :)

Yes, please attach a patch to this bug and assign me (:wlach) to review. :)

Comment 10

[akruglov]

Attachment: fix for bugzilla 797897

I've generated it with git diff. Is it OK? Should I use some other tool?

Comment 11

[William Lachance (:wlach)]

Comment on attachment 775937 [details] [diff] [review]
fix for bugzilla 797897

This is a really great start. Yes, the output of "git diff" is fine. In the future though, please mark your attachment as a patch, as it makes reviewing easier.

The main problem here is that corresponding changes need to happen in devicemanagerADB's implementation of this command (devicemanagerADB.py) and the method prototypes and documentation in devicemanager.py need to be updated.

I also have a minor request for imporvement:

>diff --git a/mozdevice/mozdevice/devicemanagerSUT.py b/mozdevice/mozdevice/devicemanagerSUT.py
>index 0e03115..fc25357 100644
>--- a/mozdevice/mozdevice/devicemanagerSUT.py
>+++ b/mozdevice/mozdevice/devicemanagerSUT.py
>@@ -338,9 +338,10 @@ class DeviceManagerSUT(DeviceManager):
>         # woops, we couldn't find an end of line/return value
>         raise DMError("Automation Error: Error finding end of line/return value when running '%s'" % cmdline)
> 
>-    def pushFile(self, localname, destname, retryLimit = None):
>+    def pushFile(self, localname, destname, retryLimit = None, createDir = True):

So I know this is existing code, but pep8 says that there should be no whitespace around "=" signs. So while we're here, let's change this line to:

  def pushFile(self, localname, destname, retryLimit=None, createDir=True):

See: http://www.python.org/dev/peps/pep-0008/#other-recommendations

>@@ -362,7 +363,7 @@ class DeviceManagerSUT(DeviceManager):
>         if not self.dirExists(name):
>             self._runCmds([{ 'cmd': 'mkdr ' + name }])
> 
>-    def pushDir(self, localDir, remoteDir, retryLimit = None):
>+    def pushDir(self, localDir, remoteDir, retryLimit = None, createDir = True):

Same comment as above on whitespace.

Comment 12

[akruglov]

Attachment: bugzilla797897.v2

Comment 13

[William Lachance (:wlach)]

Comment on attachment 776592 [details] [diff] [review]
bugzilla797897.v2

Sorry for the late response. This looks great! All we need is a corresponding set of modifications to devicemanagerADB.py and we're good to go.

Comment 14

[akruglov]

And where pushFile in devicemanagerADB.py tries to create directory?
I found only one line where it's possible:
https://github.com/mozilla/mozbase/blob/master/mozdevice/mozdevice/devicemanagerADB.py#L207

Am I right?

Comment 15

[William Lachance (:wlach)]

(In reply to akruglov from comment #14)
> And where pushFile in devicemanagerADB.py tries to create directory?
> I found only one line where it's possible:
> https://github.com/mozilla/mozbase/blob/master/mozdevice/mozdevice/
> devicemanagerADB.py#L207
> 
> Am I right?

I think you meant: https://github.com/mozilla/mozbase/blob/master/mozdevice/mozdevice/devicemanagerADB.py#L200

If we eliminated this check, we could still save a bit of time with adb, though not nearly as much as with the sut case.

It looks like we don't do the same sorts of checking for adb, primarily because the implementation is rather different. However, it's still important that we keep the function prototypes the same between the two, so we can use the different implementations interchangeably. So please do modify the implementation in devicemanagerADB.py as well as the "prototype" in devicemanager.py.

Comment 16

[akruglov]

Attachment: bugzilla797897.v3

I made corresponded changes in devicemanager.py (in signature of abstract method pushFile) and in devicemanagerADB.py (hopefully I got the idea right).

Comment 17

[William Lachance (:wlach)]

Comment on attachment 781123 [details] [diff] [review]
bugzilla797897.v3

Review of attachment 781123 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good except one minor problem which can be easily fixed. I'll just take out the line and push your patch! Thanks for your patience.

::: mozdevice/mozdevice/devicemanagerADB.py
@@ +196,5 @@
>          # but that would be different behaviour from devicemanagerSUT. Throw
>          # an exception so we have the same behaviour between the two
>          # implementations
>          retryLimit = retryLimit or self.retryLimit
> +        if createDir:

On closer inspection, we actually don't want to do this -- we're checking an error condition here, and we should leave this in place. I'll take this out before applying the patch.

Comment 18

[William Lachance (:wlach)]

Pushed: https://github.com/mozilla/mozbase/commit/b52e447ffce6bf0600e305f90c2982c7cf545725