Device/test root functionality in mozdevice can cause problems

RESOLVED FIXED in mozilla33

Status

Testing
Mozbase
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: wlach, Assigned: wlach)

Tracking

Trunk
mozilla33
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

In working on Eideticker for Android, I encountered a sort of catch 22 where I couldn't instantiate a device manager object because the "device root" (basically a scratch area for testing) was "busy" because Firefox was still using it for testing, but I couldn't kill Firefox because I couldn't construct a device manager object.

We should only instantiate / initialize the device root when actually needed. Moreover, all this code is quite confusing and in need of an overhaul.
Created attachment 8453205 [details] [diff] [review]
Rework device root stuff

This patch does the following:

* Adds a deviceRoot property, which initializes the device root when accessed
* Turns getDeviceRoot() into a compatibility shim over deviceRoot
* Removes the getTestRoot method, it was pretty useless and confusing (only used by the xpcshell test, and easy enough to fix it to not do so)
* Converts getAppRoot into an Android specific method and put it in the droid subclass

The test root / app root stuff is strictly not backwards compatible but I'm pretty sure they weren't used by anything except the in-tree xpcshell tests. Talos doesn't use them for sure, nor do the gaia ui tests or eideticker.
Attachment #8453205 - Flags: review?(bclary)
Attachment #8453205 - Flags: feedback?(mcote)

Comment 3

4 years ago
Comment on attachment 8453205 [details] [diff] [review]
Rework device root stuff

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

r+ overall with some resolution of the unreachable try block in the deviceRoot property. I made some suggestions but leave it to your discretion on what to do with them. I'd be happy to review another patch if you would like.

::: testing/mozbase/docs/mozdevice.rst
@@ -61,4 @@
>  .. automethod:: DeviceManager.removeFile(self, filename)
>  .. automethod:: DeviceManager.removeDir(self, remoteDirname)
>  .. automethod:: DeviceManager.chmodDir(self, remoteDirname, mask="777")
> -.. automethod:: DeviceManager.getDeviceRoot(self)

Should we leave getDeviceRoot in the docs and mark it deprecated?
http://sphinx-doc.org/markup/para.html?highlight=deprecated#directive-deprecated

::: testing/mozbase/mozdevice/mozdevice/devicemanager.py
@@ +320,5 @@
> +        files
> +        """
> +        # derive deviceroot value if not set
> +        if not self._deviceRoot:
> +            self._deviceRoot = self._getDeviceRootLocation()

_getDeviceRootLocation is only implemented in devicemanagerADB.py and devicemanagerSUT.py. Should we include an abstractmethod in devicemanager.py?

I've always been confused by the 'device root' and 'test root' and their differences between the adb and sut implementations.

In devicemanagerADB.py, _getDeviceRootLocation returns <path/tests> where |path| is the first external storage path (out of a fixed list) where a tests subdirectory already exists or where we can create a tests subdirectory.

In devicemanagerSUT.py, _getDeviceRootLocation returns the value returned by the SUTAgent 'testroot' command which is either the external storage path or the path specified by the SUTAgent.ini file on the device. In general, this path is not to a tests subdirectory.

I think the sut _getDeviceRootLocation should return a path to the tests subdirectory <testroot/tests> to keep the same behavior between the adb and sut versions.

@@ +324,5 @@
> +            self._deviceRoot = self._getDeviceRootLocation()
> +
> +        # attempt to set up device root when first needed
> +        if not self._deviceRootSetup:
> +            if not self.dirExists(self._deviceRoot):

I think it is guaranteed that dirExists will always return True since the adb version of _getDeviceRootLocation will create _deviceRoot if it doesn't exist or raise an exception if it can't and the sut version of _getDeviceRootLocation will only return a testroot that exists and is writable.

@@ +329,5 @@
> +                try:
> +                    self.mkDir(self._deviceRoot)
> +                except:
> +                    self._logger.error("Unable to create device root %s" % self._deviceRoot)
> +                    raise

I don't think this try block will be executed.

I believe the intent here is to use mkDir _deviceRoot as a proxy for checking if the device is in a state where a test can be set up. mkDir by itself will miss the case where the directory is in use by something else such as fennec. This will also miss the case for adb where the directory already exists but is on a read-only file system. In that case, the error will be "mkdir failed for ..., File exists" which won't be caught by the devicemanagerADB versions of mkDir.

adb.py attempts to remove the directory if it already exists and then attempts to create it. That would catch the in-use and read-only cases I believe.

We already create the _deviceRoot directory in the adb version of _getDeviceRootLocation.

If we changed the sut version of _getDeviceRootLocation to return <testroot/tests>, we could:

* remove the mkDir check from deviceRoot property,
* create  a private helper method that would remove the _deviceRoot if it existed and create it again,
* call the helper in the in the adb and sut versions of _getDeviceRootLocation.

We would return consistent paths in the adb and sut cases and would be guaranteed that we had full use of the directory.

@@ +342,2 @@
>  
> +        DEPRECATED: Use the deviceRoot property instead

use http://sphinx-doc.org/markup/para.html?highlight=deprecated#directive-deprecated ?

This might help people using the docs and looking at code consumers that still use getDeviceRoot.

::: testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py
@@ +458,2 @@
>                  try:
> +                    self.mkDir(root)

call the helper method here instead of mkDir?

::: testing/mozbase/mozdevice/mozdevice/devicemanagerSUT.py
@@ +293,5 @@
>                  self._sock = None
>                  raise DMError("Automation Error: Error closing socket")
>  
> +    def _getDeviceRootLocation(self):
> +        return "%s/tests" % self._runCmds([{ 'cmd': 'testroot' }]).strip()

Change this to return <testroot/tests> and call the helper method here?
Attachment #8453205 - Flags: review?(bclary) → review+

Comment 4

4 years ago
Comment on attachment 8453205 [details] [diff] [review]
Rework device root stuff

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

I think bc's points are valid.  Unlike him I'm going to f- this review since I think the points should be addressed. :)

::: testing/mozbase/mozdevice/mozdevice/devicemanager.py
@@ +320,5 @@
> +        files
> +        """
> +        # derive deviceroot value if not set
> +        if not self._deviceRoot:
> +            self._deviceRoot = self._getDeviceRootLocation()

Agree with Bob here; yet more confusion around deviceRoot that can be cleaned up at the same time.

@@ +329,5 @@
> +                try:
> +                    self.mkDir(self._deviceRoot)
> +                except:
> +                    self._logger.error("Unable to create device root %s" % self._deviceRoot)
> +                    raise

I guess adb.py already does it, but do we definitely want to overwrite deviceRoot each time?  If so, we should probably be clear somewhere that anything in deviceRoot won't stick around.  If not, we could just create and then remove a temporary file to make sure we can write to the directory.

@@ +338,4 @@
>      def getDeviceRoot(self):
>          """
> +        Get the device root on the device filesystem for putting temporary testing
> +        files

I think we try to end most comments with periods, no?

::: testing/xpcshell/remotexpcshelltests.py
@@ +595,4 @@
>  
>      if (options.dm_trans == "adb"):
>          if (options.deviceIP):
> +            dm = mozdevice.DroidADB(options.deviceIP, options.devicePort, packageName=None, deviceRoot=options.remoteTestRoot)

Hm are we still cleaning up surrounding code when doing changes?  If so consider removing those awful, lingering parentheses. :)
Attachment #8453205 - Flags: feedback?(mcote) → feedback-
Created attachment 8453999 [details] [diff] [review]
Revised patch

Ok, another iteration. I wound up consolidating logic inside a _setupDeviceRoot method, which is instantiated inside both adb and sut implementation of devicemanager. I think this fixes most of the weird logic problems :bc mentioned. I also fixed up the doc nits and the mozdevice unit tests.
Attachment #8453205 - Attachment is obsolete: true
Attachment #8453999 - Flags: review?(bclary)
Attachment #8453999 - Flags: feedback?(mcote)
Created attachment 8454066 [details] [diff] [review]
Also replace uses of getDeviceRoot with deviceRoot

Just realized I should also fix the stuff I planned to deprecate.
Attachment #8453999 - Attachment is obsolete: true
Attachment #8453999 - Flags: review?(bclary)
Attachment #8453999 - Flags: feedback?(mcote)
Attachment #8454066 - Flags: review?(bclary)
Attachment #8454066 - Flags: feedback?(mcote)

Comment 8

4 years ago
Comment on attachment 8454066 [details] [diff] [review]
Also replace uses of getDeviceRoot with deviceRoot

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

Should we also change the remaining uses of getDeviceRoot() to deviceRoot in these files?

testing/mozbase/mozdevice/mozdevice/devicemanager.py
testing/mozbase/mozdevice/mozdevice/dmcli.py
testing/mozbase/mozdevice/sut_tests/test_exec.py
testing/mozbase/mozdevice/sut_tests/test_push1.py
testing/mozbase/mozdevice/sut_tests/test_pushbinary.py
testing/mozbase/mozdevice/sut_tests/test_push2.py
testing/mozbase/mozdevice/sut_tests/test_exec_env.py
testing/mozbase/mozdevice/sut_tests/test_pushsmalltext.py
testing/mozbase/mozdevice/sut_tests/test_fileExists.py
testing/mozbase/mozdevice/sut_tests/test_pull.py
testing/mozbase/mozdevice/sut_tests/test_getdir.py
testing/remotecppunittests.py
testing/xpcshell/runtestsb2g.py

As discussed on irc, we can file a follow up bug to validate the deviceRoot to make sure it is usable.

::: testing/mozbase/mozdevice/mozdevice/devicemanager.py
@@ +50,5 @@
>          self._logger = mozlog.getLogger("DeviceManager")
>          self._logLevel = logLevel
>          self._logger.setLevel(logLevel)
>          self._remoteIsWin = None
> +        self._deviceRootSetup = False

_deviceRootSetup seems kind of like a verb. maybe something like _isDeviceRootSetup ? feel free to ignore this.

::: testing/mozbase/mozdevice/mozdevice/devicemanagerSUT.py
@@ +518,1 @@
>              cmd = cmd[:2] + self._getExtraAmStartArgs() + cmd[2:] 

Maybe kill the already existing trailing space?
Attachment #8454066 - Flags: review?(bclary) → review+
Created attachment 8454618 [details] [diff] [review]
New version of patch with bc's nits addressed, carrying forward r+
Attachment #8454066 - Attachment is obsolete: true
Attachment #8454066 - Flags: feedback?(mcote)
Attachment #8454618 - Flags: review+
Attachment #8454618 - Flags: feedback?(mcote)
Blocks: 1037651
Comment on attachment 8454618 [details] [diff] [review]
New version of patch with bc's nits addressed, carrying forward r+

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

Looks good.
Attachment #8454618 - Flags: feedback?(mcote) → feedback+
See try run results from: https://bugzilla.mozilla.org/show_bug.cgi?id=1036530#c6
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4d30c250d375
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.