Refactor test scripts for android to improve performance

build/android/pylib/device/device_utils.py

 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Provides a variety of device interactions based on adb.
 
 Eventually, this will be based on adb_wrapper.
 """
 # pylint: disable=W0613
 
@@ skipping 604 matching lines @@
       timeout: timeout in seconds
       retries: number of retries
 
     Raises:
       CommandTimeoutError on timeout.
       DeviceUnreachableError on missing device.
     """
     self.RunShellCommand(['am', 'force-stop', package], check_return=True)
 
   @decorators.WithTimeoutAndRetriesFromInstance()
-  def ClearApplicationState(self, package, timeout=None, retries=None):
+  def ClearApplicationState(self, package, timeout=None, retries=None,
+                            package_check_needed=True):

[jbudorick, 2015/01/16 14:21:41]

timeout and retries should be the last 2 parameters.

[Jaekyun Seok (inactive), 2015/01/18 23:01:18]

Done.

     """Clear all state for the given package.
 
     Args:
       package: A string containing the name of the package to stop.
       timeout: timeout in seconds
       retries: number of retries
+      package_check_needed: Whether the package existence should be checked or
+                            not
 
     Raises:
       CommandTimeoutError on timeout.
       DeviceUnreachableError on missing device.
     """
     # Check that the package exists before clearing it. Necessary because
     # calling pm clear on a package that doesn't exist may never return.
-    if self.GetApplicationPath(package):
+    if (not package_check_needed) or self.GetApplicationPath(package):

[jbudorick, 2015/01/16 14:21:41]

I'm extremely wary about adding more somewhat superfluous options to
DeviceUtils, _especially_ ones that let the client screw up (as this one does).

[Jaekyun Seok (inactive), 2015/01/18 23:01:18]

I agree with your concern, but each call to self.GetApplicationPath(package)
took almost 1 sec when I tested, so I want to skip this when it isn't needed
obviously.

[jbudorick, 2015/01/20 14:11:23]

That's reasonable; I'm just not sure that the client should be determining when
this isn't needed. This exposes an implementation detail that I don't think we
should be exposing.

I wonder if there's something a bit more clever we can do with caching the
output of pm list packages... the tricky part of that would be keeping the cache
from going stale.

[Jaekyun Seok (inactive), 2015/01/20 22:00:23]

Caching device status seems dangerous because we can't track all the status
changes on a device. How about calling "device.RunShellCommand(['pm', 'clear',
package], check_return=True)" directly from
TestPackageApk.ClearApplicationState?

[jbudorick, 2015/01/20 22:04:04]

What do you mean by "we can't track all the status changes?"
No, we actively discourage calling RunShellCommand when a viable alternative
exists (as it does here).

[Jaekyun Seok (inactive), 2015/01/20 22:11:17]

For example, a package could be uninstalled by device.RunShellCommand(...), and
in that case, we can't update cache. Do you think we can ignore such case?

[jbudorick, 2015/01/20 22:22:43]

I think that case is less risky than the current implementation, but I'll admit
that it isn't risk-free. (Cases like that are one reason why we discourage using
RunShellCommand raw where alternatives exist, as one does here)

That said, we do some amount of caching already that could conceivably be messed
up by RunShellCommand. Perhaps calls to RunShellCommand should clear non-RO
cache items.

[Jaekyun Seok (inactive), 2015/01/20 23:20:35]

I see. I will add cache to store application paths.
For now, RunShellCommand is being used in many sites including
TestPackageApk.ClearApplicationState. So clearing cache of application paths for
every call to RunShellCommand will make caching useless.
I will leave this clearing as TODO.

[jbudorick, 2015/01/20 23:34:24]

This could probably be integrated into the existing cache, although perhaps the
caching mechanism should be entirely moved into AdbWrapper. I'd at least
recommend waiting until Juan chimes in, though. He's in EMEA, so it'll be a
little while.
I agree that that's out of scope of the bug. In face, this entire CL may be
getting out of the scope of the bug, as it's getting heavily into DeviceUtils
performance tuning rather than things specific to content_browsertests.

[perezju, 2015/01/21 10:28:52]

Yeah, I also dislike the idea of adding this option. Because it helps improve
the efficiency but only for this particular one caller. I would rather prefer if
we find an alternative that could also bring benefits to other clients as well.

On 2015/01/20 23:34:24, jbudorick wrote:
I don't think caching should be done by AdbWrapper, which should only know how
to run adb commands, but not what those commands do (e.g. lifetimes of data or
what is read-only).
I like the idea of caching, but I'm not sure how easy will it be to get it
right. We need to check which other DeviceUtils methods deal with pm and should
invalidate the cache. Also we should probably do a quick check to see if other
clients are calling 'pm' via RunShellCommand since we may want to have those
using calling the appropriate DeviceUtils' methods instead.

Alternatively, can we check whether the 'pm path' test is actually still needed?
I've traced back the CL where that change was introduced to [1], when not having
the check would break many bots.

I've tested now, however, on a device on my desk and 'pm clear' seems to work
fine even with non-existent packages (returns 'Failed' but doesn't hang as the
comment suggests). Maybe this was fixed on recent Android versions? If so, maybe
we can just skip the test if the version is recent enough? Would this be enough
to reap the efficiency benefits without having to introduce much more
complexity?

[1]: https://codereview.chromium.org/12210036/#ps20002

[jbudorick, 2015/01/21 14:57:00]

My concern in this case is that AdbWrapper handles Uninstall directly. (I
imagine there are similar cases.)
Agreed, it's tricky.
We'll have to test this out with older versions of android.

[jbudorick, 2015/01/21 15:03:14]

If this last part _does_ work on versions starting with ICS, then that's
definitely the way to go.

If not, we could also consider implementing a safe version of pm clear that we
install to the device (similar to what we do with unzip).

       self.RunShellCommand(['pm', 'clear', package], check_return=True)
 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def SendKeyEvent(self, keycode, timeout=None, retries=None):
     """Sends a keycode to the device.
 
     See: http://developer.android.com/reference/android/view/KeyEvent.html
 
     Args:
       keycode: A integer keycode to send to the device.
@@ skipping 635 matching lines @@
     Returns:
       A Parallelizer operating over |devices|.
     """
     if not devices:
       devices = adb_wrapper.AdbWrapper.GetDevices()
     devices = [d if isinstance(d, cls) else cls(d) for d in devices]
     if async:
       return parallelizer.Parallelizer(devices)
     else:
       return parallelizer.SyncParallelizer(devices)