Only consider Android devices with more than 30% battery remaining for layout tests

Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py

Index: Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py
diff --git a/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py b/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py
index 173580b68205f0bb469dd72da0bc7ce83df8c44c..f8281797d0b50e163cfcde8c48ddf063c713e0a0 100644
--- a/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py
+++ b/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py
@@ -275,17 +275,6 @@ class AndroidCommands(object):
         AndroidCommands._adb_command_path = command_path
         return command_path
 
-    @staticmethod
-    def get_devices(executive):
-        re_device = re.compile('^([a-zA-Z0-9_:.-]+)\tdevice$', re.MULTILINE)
-        result = executive.run_command([AndroidCommands.adb_command_path(executive), 'devices'],
-                error_handler=executive.ignore_error)
-        devices = re_device.findall(result)
-        if not devices:
-            raise AssertionError('No devices attached. Result of "adb devices": %s' % result)
-
-        return devices
-
     # Local private methods.
 
     def _log_error(self, message):
@@ -308,6 +297,75 @@ class AndroidCommands(object):
 
         return [int(n) for n in result.group(1).split('.')]
 
+
+# A class to encapsulate device status and information, such as the AndroidCommands
+# instances and whether the device has been set up.
+class AndroidDevices(object):
+    # How many percentages of battery power does a device need to have left in order

[DaleCurtis, 2013/06/21 18:06:23]

Rephrase as a statement.  Something like "The minimum battery level required to
run layout tests."

[Peter Beverloo, 2013/06/24 17:12:17]

Done.

+    # for it to participate in running the layout tests?
+    MINIMUM_BATTERY_PERCENTAGE = 30
+
+    # A list to keep track of which devices have been prepared.

[DaleCurtis, 2013/06/21 18:06:23]

Instead of a global variable w/ static accessors you could have
ChromiumAndroidPort pass in AndroidDevices to ChromiumAndroidDriver like you do
with AndroidCommands.  If globals are common style here, feel to ignore me.

[Dirk Pranke, 2013/06/21 23:02:17]

globals are not common. I'm also not clear on what we think we're gaining by
using globals here. It is quite uncommon to create more than one port object per
process.

[Peter Beverloo, 2013/06/24 17:12:17]

Agreed, changed.

+    _prepared_devices = []
+
+    def __init__(self, executive, default_device=None):
+        self._usable_devices = []
+        self._default_device = default_device
+
+    def usable_devices(self, executive):
+        if self._usable_devices:
+            return self._usable_devices
+
+        if self._default_device:
+            self._usable_devices = [AndroidCommands(executive, self._default_device)]

[DaleCurtis, 2013/06/21 18:06:23]

Just set this in __init__() ? You don't need self._default_device then.

[Peter Beverloo, 2013/06/24 17:12:17]

By using this in the constructor, the AndroidCommands class will attempt to
interact with the filesystem to find the path to the ADB tool (and may error out
when it can't be found).  In general we're trying to keep the constructors free
of that, and initialize paths like that lazily.

The default_device argument is necessary to support the argument to
run_webkit_tests.py allowing a specific device to be used for testing.

+            return self._usable_devices
+
+        re_device = re.compile('^([a-zA-Z0-9_:.-]+)\tdevice$', re.MULTILINE)

[DaleCurtis, 2013/06/21 18:06:23]

Providing an example in comment is good practice when writing a regex.

[Peter Beverloo, 2013/06/24 17:12:17]

Done.

+        result = executive.run_command([AndroidCommands.adb_command_path(executive), 'devices'],
+                error_handler=executive.ignore_error)
+        devices = re_device.findall(result)
+        if not devices:
+            raise AssertionError('Unable to find attached Android devices. ADB output: %s' % result)
+
+        for device_serial in devices:
+            commands = AndroidCommands(executive, device_serial)
+            if self._battery_level_for_device(commands) < AndroidDevices.MINIMUM_BATTERY_PERCENTAGE:
+                continue
+
+            self._usable_devices.append(commands)
+
+        if not self._usable_devices:
+            raise AssertionError('No devices attached with more than %d percent battery.' %
+                AndroidDevices.MINIMUM_BATTERY_PERCENTAGE)
+
+        return self._usable_devices
+
+    def get_device(self, executive, device_index):
+        devices = self.usable_devices(executive)
+        if device_index >= len(devices):
+            raise AssertionError('Device index exceeds number of usable devices.')
+
+        return devices[device_index]
+
+    # Public static methods
+    @staticmethod
+    def is_device_prepared(device_index):
+        return device_index in AndroidDevices._prepared_devices

[DaleCurtis, 2013/06/21 18:06:23]

device_index is an integer above, but you're calling this method with a serial. 
maybe device_serial instead?

[Peter Beverloo, 2013/06/24 17:12:17]

Done.

+
+    @staticmethod
+    def set_device_prepared(device_index):

[DaleCurtis, 2013/06/21 18:06:23]

Ditto.

[Peter Beverloo, 2013/06/24 17:12:17]

Done.

+        AndroidDevices._prepared_devices.append(device_index)
+
+    # Private methods
+    def _battery_level_for_device(self, commands):
+        battery_status = commands.run(['shell', 'dumpsys', 'battery'])
+        if 'Error' in battery_status:
+            _log.warning('Unable to read the battery level from device with serial "%s".' % commands.get_serial())
+            return 100
+
+        return int(re.findall('level: (\d+)', battery_status)[0])
+
+
 class ChromiumAndroidPort(chromium.ChromiumPort):
     port_name = 'chromium-android'
 
@@ -332,22 +390,18 @@ class ChromiumAndroidPort(chromium.ChromiumPort):
         else:
             self._driver_details = DumpRenderTreeDriverDetails()
 
+        # Initialize the AndroidDevices class which tracks available devices.
+        default_device = None
         if hasattr(self._options, 'adb_device') and len(self._options.adb_device):
-            self._devices = [self._options.adb_device]
-        else:
-            self._devices = []
+            default_device = self._options.adb_device
 
+        self._devices = AndroidDevices(self._executive, default_device)
+
+        # Tell AndroidCommands where to search for the "adb" command.
         AndroidCommands.set_adb_command_path_options(['adb',
             self.path_from_chromium_base('third_party', 'android_tools', 'sdk', 'platform-tools', 'adb')])
 
     # Local public methods.
-    def get_device_serial(self, worker_number):
-        if not self._devices:
-            self._devices = AndroidCommands.get_devices(self._executive)
-        if worker_number >= len(self._devices):
-            raise AssertionError('Worker number exceeds available number of devices')
-        return self._devices[worker_number]
-
     def path_to_forwarder(self):
         return self._build_path('forwarder')
 
@@ -387,10 +441,11 @@ class ChromiumAndroidPort(chromium.ChromiumPort):
         return 'DumpRenderTree'
 
     def default_child_processes(self):
-        if self._devices:
-            return len(self._devices)
+        usable_device_count = len(self._devices.usable_devices(self._executive))
+        if not usable_device_count:
+            raise AssertionError('There are no devices available to run the layout tests on.')
 
-        return len(AndroidCommands.get_devices(self._executive))
+        return usable_device_count
 
     def default_baseline_search_path(self):
         return map(self._webkit_baseline_path, self.FALLBACK_PATHS['android'])
@@ -437,6 +492,7 @@ class ChromiumAndroidPort(chromium.ChromiumPort):
 
     def create_driver(self, worker_number, no_timeout=False):
         return ChromiumAndroidDriver(self, worker_number, pixel_tests=self.get_option('pixel_tests'), driver_details=self._driver_details,
+                                     android_commands=self._devices.get_device(self._executive, worker_number),
                                      # Force no timeout to avoid test driver timeouts before NRWT.
                                      no_timeout=True)
 
@@ -621,7 +677,7 @@ http://crbug.com/165250 discusses making these pre-built binaries externally ava
 
 
 class ChromiumAndroidDriver(driver.Driver):
-    def __init__(self, port, worker_number, pixel_tests, driver_details, no_timeout=False):
+    def __init__(self, port, worker_number, pixel_tests, driver_details, android_commands, no_timeout=False):
         super(ChromiumAndroidDriver, self).__init__(port, worker_number, pixel_tests, no_timeout)
         self._in_fifo_path = driver_details.device_fifo_directory() + 'stdin.fifo'
         self._out_fifo_path = driver_details.device_fifo_directory() + 'test.fifo'
@@ -629,11 +685,10 @@ class ChromiumAndroidDriver(driver.Driver):
         self._read_stdout_process = None
         self._read_stderr_process = None
         self._forwarder_process = None
-        self._has_setup = False
         self._original_governors = {}
         self._original_kptr_restrict = None
 
-        self._android_commands = AndroidCommands(port._executive, port.get_device_serial(worker_number))
+        self._android_commands = android_commands
         self._driver_details = driver_details
 
         # FIXME: If we taught ProfileFactory about "target" devices we could
@@ -710,12 +765,11 @@ class ChromiumAndroidDriver(driver.Driver):
         self._push_platform_resources()
 
     def _setup_test(self):
-        if self._has_setup:
+        if AndroidDevices.is_device_prepared(self._android_commands.get_serial()):
             return
 
         self._android_commands.restart_as_root()
         self._setup_md5sum_and_push_data_if_needed()
-        self._has_setup = True
         self._setup_performance()
 
         # Required by webkit_support::GetWebKitRootDirFilePath().
@@ -730,6 +784,9 @@ class ChromiumAndroidDriver(driver.Driver):
         # Make sure that the disk cache on the device resets to a clean state.
         self._android_commands.run(['shell', 'rm', '-r', self._driver_details.device_cache_directory()])
 
+        # Mark this device as having been set up.
+        AndroidDevices.set_device_prepared(self._android_commands.get_serial())
+
     def _log_error(self, message):
         _log.error('[%s] %s' % (self._android_commands.get_serial(), message))
 
@@ -1008,7 +1065,7 @@ class ChromiumAndroidDriver(driver.Driver):
             self._forwarder_process.kill()
             self._forwarder_process = None
 
-        if self._has_setup:
+        if AndroidDevices.is_device_prepared(self._android_commands.get_serial()):
             if not ChromiumAndroidDriver._loop_with_timeout(self._remove_all_pipes, DRIVER_START_STOP_TIMEOUT_SECS):
                 raise AssertionError('Failed to remove fifo files. May be locked.')