Closed Bug 845506 Opened 9 years ago Closed 9 years ago

mozdevice needs documentation

Categories

(Testing :: Mozbase, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wlach, Assigned: wlach)

Details

Attachments

(1 file)

This bug covers adding sphinx style documentation for mozdevice for our doc site:

http://mozbase.readthedocs.org

(CCing everyone I can think of who uses/contributes to mozdevice regularly as I hope you will find this work useful!)
This adds documentation for mozdevice

Implementation notes:

* I removed duplicate docstrings from dmSUT and dmADB. As I found when working on this, if we maintain the same information in multiple places, it quickly goes out of date.
* I moved the method order to correspond with that of the documentation
* I did some minor updates to the mozb2g documentation at the same time
* I did some minor changes to the unpackFile method to fix its variable naming to correspond with the style elsewhere in the file. As far as I know sut_tools is the only thing that uses this (https://hg.mozilla.org/build/tools/file/c290db3dd537/sut_tools/installTests.py) and it shouldn't be affected by this change.
Attachment #718606 - Flags: review?(mcote)
Attachment #718606 - Flags: feedback?(jmaher)
Comment on attachment 718606 [details] [diff] [review]
Patch to add documentation for mozdevice

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

there is a lot of code added to devicemanager.py, I don't see it removed from dmSUT or dmADB.  Otherwise this is looking good.

::: docs/mozdevice.rst
@@ +34,5 @@
> +.. automethod:: DeviceManager.getCurrentTime(self)
> +.. automethod:: DeviceManager.getIP
> +.. automethod:: DeviceManager.saveScreenshot
> +.. automethod:: DeviceManager.recordLogcat
> +.. automethod:: DeviceManager.getLogcat

we will have to remember to keep this file up to date as we add/edit devicemanager.
Attachment #718606 - Flags: feedback?(jmaher) → feedback+
Comment on attachment 718606 [details] [diff] [review]
Patch to add documentation for mozdevice

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

Good stuff!

A few specific comments below.  One overall comment is that PEP 257 ("Docstring Conventions") specifies that docstrings should end in periods.  I know we've been trying to standardize our docstrings and comments, so might as well fix 'em up here.  There is also a mix of capitalization at the beginning of parameter descriptions.

::: docs/devicemanagement.rst
@@ +1,5 @@
> +Device management
> +-----------------
> +
> +We provide several modules for communicating with a remote device
> +(e.g. an Android or FirefoxOS-based phone) for the purposes of running

Strictly speaking I believe this should be "an Android- or FirefoxOS-based phone" if you mean to apply the "based" to both.

::: docs/mozdevice.rst
@@ +1,4 @@
> +:mod:`mozdevice` --- Interact with remote devices
> +=================================================
> +
> +mozdevice provides an interface to interact with a remote device such

I believe our suggested usage states that "mozdevice" should be capitalized here since it begins a sentence.

@@ +1,5 @@
> +:mod:`mozdevice` --- Interact with remote devices
> +=================================================
> +
> +mozdevice provides an interface to interact with a remote device such
> +as an Android or FirefoxOS-based phone connected to a

Again, "Android-".

@@ +2,5 @@
> +=================================================
> +
> +mozdevice provides an interface to interact with a remote device such
> +as an Android or FirefoxOS-based phone connected to a
> +workstation. Currently there are two implementations of the interface: one

"workstation" strikes me as odd here... "host machine" maybe?

@@ +3,5 @@
> +
> +mozdevice provides an interface to interact with a remote device such
> +as an Android or FirefoxOS-based phone connected to a
> +workstation. Currently there are two implementations of the interface: one
> +uses a TCP-based protocol to communicate with a server running on the device,

Maybe "custom TCP-based protocol" just to differentiate from the fact that it can also use adb over TCP.

@@ +25,5 @@
> +
> +  dm = mozdevice.DeviceManagerADB()
> +  print dm.listFiles('/mnt/sdcard')
> +  if dm.processExist('org.mozilla.fennec'):
> +      print "Fennec is running"

Mix of single and double quotes.

@@ +34,5 @@
> +.. automethod:: DeviceManager.getCurrentTime(self)
> +.. automethod:: DeviceManager.getIP
> +.. automethod:: DeviceManager.saveScreenshot
> +.. automethod:: DeviceManager.recordLogcat
> +.. automethod:: DeviceManager.getLogcat

Yeah, a bit out of scope here, but autogenerating this list, somehow, would be cool... I can practically guarantee that it will be out of sync at times.  Also, why do some mention the method args and others don't?

@@ +88,5 @@
> +.. autoclass:: mozdevice.DeviceManagerSUT
> +
> +SUT-specific methods
> +````````````````````
> +For the purpose of use within Mozilla's internal automated testing,

Phrasing is a bit awkward here.  Maybe something like "DeviceManagerSUT has several methods that are only used in specific tests and are not present in all DeviceManager implementations."  Something like that.  But I like that you called out that they aren't implemented everywhere.

@@ +105,5 @@
> +
> +.. autoclass:: mozdevice.DroidADB
> +   :members: launchApplication, launchFennec
> +
> +The DroidSUT class provides the same methods described above.

I think we should mention what differentiates DroidSUT from DeviceManagerSUT and DroidADB from DeviceManagerADB.  And does DroidADB not provide the DeviceManagerADB methods described above as well?

::: mozdevice/mozdevice/devicemanager.py
@@ +26,4 @@
>  def abstractmethod(method):
>      line = method.func_code.co_firstlineno
>      filename = method.func_code.co_filename
> +    @wraps(method)

Nice, I learned something new. :)  I guess we can replace this all with the adc module if we ever stop supporting Python versions < 2.6...

@@ +79,4 @@
>          """
> +        for interface in interfaces:
> +            match = re.match(r"%s: ip (\S+)" % interface,
> +                             self.shellCheckOutput(['ifconfig', interface]))

Are you sure ifconfig is always present and conforms to this usage?  I seem to recall having to use netcfg on some phones.  I *think* netcfg is always present on android (though I don't know if the output is always identical).

@@ +347,2 @@
>  
> +        Format of tuples is: (processId, processName, userId)

No colon.

::: mozdevice/mozdevice/devicemanagerADB.py
@@ +25,5 @@
> +
> +      dm = mozdevice.DeviceManagerADB()
> +      print dm.listFiles('/mnt/sdcard')
> +      if dm.processExist('org.mozilla.fennec'):
> +          print "Fennec is running"

Mix of single and double quotes again.  Also, is this necessary here, given that the same example is in devicemanagement.rst?

::: mozdevice/mozdevice/devicemanagerSUT.py
@@ +17,5 @@
>  class DeviceManagerSUT(DeviceManager):
> +    """
> +    Implementation of DeviceManager interface that speaks to a device over
> +    TCP/IP using the "system under test" protocol. A software agent such as
> +    Negatus (http://github.com/mozilla/Negatus) must be present and listening

I think we should specifically mention the Java agent as well, since it is still the canonical agent for Android testing.

@@ +29,5 @@
> +
> +      dm = mozdevice.DeviceManagerSUT('192.168.1.5')
> +      print dm.listFiles('/mnt/sdcard')
> +      if dm.processExist('org.mozilla.fennec'):
> +          print "Fennec is running"

Three examples is really starting to feel like overkill. :)

@@ +554,5 @@
>      def catFile(self, remoteFile):
>          """
>          Returns the contents of remoteFile
> +
> +        DEPRECATED: Use pullFile in new code

I might even put a warning here that this method is inherently unsafe.

::: mozdevice/mozdevice/droid.py
@@ +72,5 @@
> +        :param appName: Name of fennec application (e.g. `org.mozilla.fennec`)
> +        :param intent: Intent to launch application with
> +        :param mozEnv: Mozilla specific environment to pass into application
> +        :param extraArgs: Extra arguments to be parsed by fennec
> +        :param url: URL to open

I guess this isn't going away? :)
Attachment #718606 - Flags: review?(mcote) → review+
Ok, just to get things moving I pushed a commit with most of your suggested fixes:

https://github.com/mozilla/mozbase/commit/3fc87f83016aa55e878e9c611490c7d29ceaff31

Comments on stuff below. I'll leave this bug open so we can fix any lingering issues in a followup.

(In reply to Mark Côté ( :mcote ) from comment #3)

> @@ +34,5 @@
> > +.. automethod:: DeviceManager.getCurrentTime(self)
> > +.. automethod:: DeviceManager.getIP
> > +.. automethod:: DeviceManager.saveScreenshot
> > +.. automethod:: DeviceManager.recordLogcat
> > +.. automethod:: DeviceManager.getLogcat
> 
> Yeah, a bit out of scope here, but autogenerating this list, somehow, would
> be cool... I can practically guarantee that it will be out of sync at times.
> Also, why do some mention the method args and others don't?

Unfortunately the use of the abstractmethod decorators makes it impossible for python to introspect the function signature properly for those methods. We have to declare them explicitly, hence singling out methods one by one. The saveScreenshot, etc. methods do not need the method signatures spelled out because they are not decorated.

> @@ +88,5 @@
> > +.. autoclass:: mozdevice.DeviceManagerSUT
> > +
> > +SUT-specific methods
> > +````````````````````
> > +For the purpose of use within Mozilla's internal automated testing,
> 
> Phrasing is a bit awkward here.  Maybe something like "DeviceManagerSUT has
> several methods that are only used in specific tests and are not present in
> all DeviceManager implementations."  Something like that.  But I like that
> you called out that they aren't implemented everywhere.

Let me know if you like the wording here:

https://github.com/mozilla/mozbase/commit/3fc87f83016aa55e878e9c611490c7d29ceaff31#L3R90

> @@ +105,5 @@
> > +
> > +.. autoclass:: mozdevice.DroidADB
> > +   :members: launchApplication, launchFennec
> > +
> > +The DroidSUT class provides the same methods described above.
> 
> I think we should mention what differentiates DroidSUT from DeviceManagerSUT
> and DroidADB from DeviceManagerADB.  And does DroidADB not provide the
> DeviceManagerADB methods described above as well?

Agreed. Let me know if you like the wording here:

https://github.com/mozilla/mozbase/commit/3fc87f83016aa55e878e9c611490c7d29ceaff31#L3R99

> @@ +79,4 @@
> >          """
> > +        for interface in interfaces:
> > +            match = re.match(r"%s: ip (\S+)" % interface,
> > +                             self.shellCheckOutput(['ifconfig', interface]))
> 
> Are you sure ifconfig is always present and conforms to this usage?  I seem
> to recall having to use netcfg on some phones.  I *think* netcfg is always
> present on android (though I don't know if the output is always identical).

No idea on this. This has nothing to do with documentation though. :) We can file a followup to investigate if you like...

> ::: mozdevice/mozdevice/devicemanagerADB.py
> @@ +25,5 @@
> > +
> > +      dm = mozdevice.DeviceManagerADB()
> > +      print dm.listFiles('/mnt/sdcard')
> > +      if dm.processExist('org.mozilla.fennec'):
> > +          print "Fennec is running"
> 
> Mix of single and double quotes again.  Also, is this necessary here, given
> that the same example is in devicemanagement.rst?

I killed the duplicate examples.

> @@ +554,5 @@
> >      def catFile(self, remoteFile):
> >          """
> >          Returns the contents of remoteFile
> > +
> > +        DEPRECATED: Use pullFile in new code
> 
> I might even put a warning here that this method is inherently unsafe.

We should probably just rewrite the method to call pullFile instead, or remove the method altogether. Filed bug 848843 to handle that.

> ::: mozdevice/mozdevice/droid.py
> @@ +72,5 @@
> > +        :param appName: Name of fennec application (e.g. `org.mozilla.fennec`)
> > +        :param intent: Intent to launch application with
> > +        :param mozEnv: Mozilla specific environment to pass into application
> > +        :param extraArgs: Extra arguments to be parsed by fennec
> > +        :param url: URL to open
> 
> I guess this isn't going away? :)

Well it's been around for a year now. :) I do indeed hope to put that in mozrunner someday, but it's slow in coming.
(In reply to William Lachance (:wlach) from comment #4)
> > @@ +34,5 @@
> > > +.. automethod:: DeviceManager.getCurrentTime(self)
> > > +.. automethod:: DeviceManager.getIP
> > > +.. automethod:: DeviceManager.saveScreenshot
> > > +.. automethod:: DeviceManager.recordLogcat
> > > +.. automethod:: DeviceManager.getLogcat
> > 
> > Yeah, a bit out of scope here, but autogenerating this list, somehow, would
> > be cool... I can practically guarantee that it will be out of sync at times.
> > Also, why do some mention the method args and others don't?
> 
> Unfortunately the use of the abstractmethod decorators makes it impossible
> for python to introspect the function signature properly for those methods.
> We have to declare them explicitly, hence singling out methods one by one.
> The saveScreenshot, etc. methods do not need the method signatures spelled
> out because they are not decorated.

All right, we'll have to be very vigilant here then.

> > @@ +88,5 @@
> > > +.. autoclass:: mozdevice.DeviceManagerSUT
> > > +
> > > +SUT-specific methods
> > > +````````````````````
> > > +For the purpose of use within Mozilla's internal automated testing,
> > 
> > Phrasing is a bit awkward here.  Maybe something like "DeviceManagerSUT has
> > several methods that are only used in specific tests and are not present in
> > all DeviceManager implementations."  Something like that.  But I like that
> > you called out that they aren't implemented everywhere.
> 
> Let me know if you like the wording here:
> 
> https://github.com/mozilla/mozbase/commit/
> 3fc87f83016aa55e878e9c611490c7d29ceaff31#L3R90

Yup yup.

> > @@ +105,5 @@
> > > +
> > > +.. autoclass:: mozdevice.DroidADB
> > > +   :members: launchApplication, launchFennec
> > > +
> > > +The DroidSUT class provides the same methods described above.
> > 
> > I think we should mention what differentiates DroidSUT from DeviceManagerSUT
> > and DroidADB from DeviceManagerADB.  And does DroidADB not provide the
> > DeviceManagerADB methods described above as well?
> 
> Agreed. Let me know if you like the wording here:
> 
> https://github.com/mozilla/mozbase/commit/
> 3fc87f83016aa55e878e9c611490c7d29ceaff31#L3R99

Groovy.

> > @@ +79,4 @@
> > >          """
> > > +        for interface in interfaces:
> > > +            match = re.match(r"%s: ip (\S+)" % interface,
> > > +                             self.shellCheckOutput(['ifconfig', interface]))
> > 
> > Are you sure ifconfig is always present and conforms to this usage?  I seem
> > to recall having to use netcfg on some phones.  I *think* netcfg is always
> > present on android (though I don't know if the output is always identical).
> 
> No idea on this. This has nothing to do with documentation though. :) We can
> file a followup to investigate if you like...

Yeah sorry, I missed that it was just relocated code; thought you were sneaking in some new code. :) I will file a separate bug.

> > @@ +554,5 @@
> > >      def catFile(self, remoteFile):
> > >          """
> > >          Returns the contents of remoteFile
> > > +
> > > +        DEPRECATED: Use pullFile in new code
> > 
> > I might even put a warning here that this method is inherently unsafe.
> 
> We should probably just rewrite the method to call pullFile instead, or
> remove the method altogether. Filed bug 848843 to handle that.

Yup, that's great.
Let's call this done.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.