Android: improves test sharding reliability.

build/android/run_tests.py

 #!/usr/bin/env python
 #
 # Copyright (c) 2012 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.
 
 """Runs all the native unit tests.
 
 1. Copy over test binary to /data/local on device.
 2. Resources: chrome/unit_tests requires resources (chrome.pak and en-US.pak)
@@ skipping 190 matching lines @@
     self.gtest_filter = gtest_filter or ''
     self.test_arguments = test_arguments
     self.timeout = timeout
     self.rebaseline = rebaseline
     self.performance_test = performance_test
     self.cleanup_test_files = cleanup_test_files
     self.tool = tool
     self.log_dump_name = log_dump_name
     self.fast_and_loose = fast_and_loose
     self.build_type = build_type
-    test = SingleTestRunner(self.attached_devices[0], test_suite, gtest_filter,
-                            test_arguments, timeout, rebaseline,
-                            performance_test, cleanup_test_files, tool, 0,
-                            not not self.log_dump_name, fast_and_loose,
-                            build_type)
     self.tests = []
     if not self.gtest_filter:
       # No filter has been specified, let's add all tests then.
-      # The executable/apk needs to be copied before we can call GetAllTests.
-      test.test_package.StripAndCopyExecutable()
-      all_tests = test.test_package.GetAllTests()
-      if not rebaseline:
-        disabled_list = test.GetDisabledTests()
-        # Only includes tests that do not have any match in the disabled list.
-        all_tests = filter(lambda t:
-                           not any([fnmatch.fnmatch(t, disabled_pattern)
-                                    for disabled_pattern in disabled_list]),
-                           all_tests)
-      self.tests = all_tests
+      self.tests, self.attached_devices = self._GetTests()
+
+  def _GetTests(self):

[Isaac (away), 2012/10/31 19:18:14]

I wonder if this would be more stable if we did the all-device apk push before
this part, asked all devices to generate the list of tests and then just used
the first list that we got back.  Would that reduce retry logic?

[bulach, 2012/10/31 19:27:31]

we have no mechanism to do that, and it doesn't look like this patch should
introduce this. I think the first step is to capture exception points and retry.
we can later optimize..

+    """Returns a tuple of (all_tests, available_devices).
+
+    Tries to obtain the list of available tests.
+    Raises Exception if all devices failed.
+    """
+    available_devices = list(self.attached_devices)
+    while available_devices:
+      try:

[Isaac (away), 2012/10/31 19:18:14]

nit: Might be cleaner with a local variable for the current device.

On line 224: device = available_devices.pop()

Then replace occurrences of available_devices[-1] with device and remove line
231.

[bulach, 2012/10/31 19:27:31]

then line 227 would be more complicated, as it'd the typical path where we don't
pop() anything..
ok to keep pop() as an exception? I think it's clearer.

+        logging.info('Obtaining tests from %s', available_devices[-1])
+        all_tests = self._GetTestsFromDevice(available_devices[-1])
+        return all_tests, available_devices
+      except Exception as e:
+        logging.info('Failed obtaining tests from %s %s',
+                     available_devices[-1], e)
+        available_devices.pop()
+    raise Exception('No device available to get the list of tests.')
+
+  def _GetTestsFromDevice(self, device):
+    test = SingleTestRunner(device, self.test_suite, self.gtest_filter,
+                            self.test_arguments, self.timeout, self.rebaseline,
+                            self.performance_test, self.cleanup_test_files,
+                            self.tool, 0,
+                            not not self.log_dump_name, self.fast_and_loose,
+                            self.build_type)
+    # The executable/apk needs to be copied before we can call GetAllTests.
+    test.test_package.StripAndCopyExecutable()
+    all_tests = test.test_package.GetAllTests()
+    if not self.rebaseline:
+      disabled_list = test.GetDisabledTests()
+      # Only includes tests that do not have any match in the disabled list.
+      all_tests = filter(lambda t:
+                         not any([fnmatch.fnmatch(t, disabled_pattern)
+                                  for disabled_pattern in disabled_list]),
+                         all_tests)
+    return all_tests
 
   def CreateShardedTestRunner(self, device, index):
     """Creates a suite-specific test runner.
 
     Args:
       device: Device serial where this shard will run.
       index: Index of this device in the pool.
 
     Returns:
       A SingleTestRunner object.
@@ skipping 203 matching lines @@
   # the batch (this happens because the exit status is a sum of all failures
   # from all suites, but the buildbot associates the exit status only with the
   # most recent step).
   if options.exit_code:
     return failed_tests_count
   return 0
 
 
 if __name__ == '__main__':
   sys.exit(main(sys.argv))