[Android] Redesigns the sharder to allow replicated vs distributed tests

build/android/pylib/gtest/setup.py

Index: build/android/pylib/gtest/setup.py
diff --git a/build/android/pylib/gtest/dispatch.py b/build/android/pylib/gtest/setup.py
similarity index 59%
rename from build/android/pylib/gtest/dispatch.py
rename to build/android/pylib/gtest/setup.py
index 3dfb2620c3f74c4af6bfd7813194933031ac0af5..14519f5d481f0ee3a169e5067002f72438d84cdf 100644
--- a/build/android/pylib/gtest/dispatch.py
+++ b/build/android/pylib/gtest/setup.py
@@ -2,9 +2,8 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-"""Dispatches GTests."""
+"""Generates test runner factory and tests for GTests."""
 
-import copy
 import fnmatch
 import glob
 import logging
@@ -14,12 +13,7 @@ import shutil
 from pylib import android_commands
 from pylib import cmd_helper
 from pylib import constants
-from pylib import ports
 from pylib.base import base_test_result
-from pylib.base import shard
-from pylib.utils import emulator
-from pylib.utils import report_results
-from pylib.utils import xvfb
 
 import gtest_config
 import test_runner
@@ -139,16 +133,16 @@ def _GenerateDepsDirUsingIsolate(test_suite, build_type):
   return deps_dir
 
 
-def _FullyQualifiedTestSuites(exe, option_test_suite, build_type):
+def _FullyQualifiedTestSuite(use_exe_test_runner, suite_name, build_type):
   """Get a list of absolute paths to test suite targets.
 
   Args:
-    exe: if True, use the executable-based test runner.
-    option_test_suite: the test_suite specified as an option.
+    use_exe_test_runner: if True, use the executable-based test runner.
+    suite_name: the suite name specified on the command line.

[frankf, 2013/07/17 04:07:20]

Capital first letter. Also in other places.

[frankf, 2013/07/17 04:07:20]

Sometimes this is called test_suite, other places suite_name. Make sure we use
consistant name and docstring

[gkanwar, 2013/07/17 20:31:26]

Done.

[gkanwar, 2013/07/17 20:31:26]

Done.

     build_type: 'Release' or 'Debug'.
 
   Returns:
-    A list of tuples containing the suite and absolute path.
+    A tuple containing the suite and absolute path.
     Ex. ('content_unittests',
           '/tmp/chrome/src/out/Debug/content_unittests_apk/'
           'content_unittests-debug.apk')
@@ -165,21 +159,17 @@ def _FullyQualifiedTestSuites(exe, option_test_suite, build_type):
     return suite.name, os.path.join(test_suite_dir, relpath)
 
   test_suite_dir = os.path.join(cmd_helper.OutDirectory.get(), build_type)
-  if option_test_suite:
-    all_test_suites = [gtest_config.Suite(exe, option_test_suite)]
-  else:
-    all_test_suites = gtest_config.STABLE_TEST_SUITES
+  test_suite = gtest_config.Suite(use_exe_test_runner, suite_name)
+  name, path = GetQualifiedSuite(test_suite)

[frankf, 2013/07/17 04:07:20]

is name same as suite_name?

[gkanwar, 2013/07/17 20:31:26]

Reworked to remove unnecessary code. This line doesn't exist any more.

 
-  # List of tuples (suite_name, suite_path)
-  qualified_test_suites = map(GetQualifiedSuite, all_test_suites)
+  if not os.path.exists(path):
+    raise Exception('Test suite %s not found in %s.\n'
+                    'Supported test suites:\n %s\n'
+                    'Ensure it has been built.\n' %
+                    (name, path,
+                     [s.name for s in gtest_config.STABLE_TEST_SUITES]))
 
-  for t, q in qualified_test_suites:
-    if not os.path.exists(q):
-      raise Exception('Test suite %s not found in %s.\n'
-                      'Supported test suites:\n %s\n'
-                      'Ensure it has been built.\n' %
-                      (t, q, [s.name for s in gtest_config.STABLE_TEST_SUITES]))
-  return qualified_test_suites
+  return (name, path)
 
 
 def GetTestsFromDevice(runner):
@@ -207,8 +197,9 @@ def GetAllEnabledTests(runner_factory, devices):
   then filters it again using the disabled list on the host.
 
   Args:
-    runner_factory: callable that takes a devices and returns a TestRunner.
-    devices: list of devices.
+    runner_factory: callable that takes device and shard_index and returns
+        a TestRunner.
+    devices: a list of device ids.
 
   Returns:
     List of all enabled tests.
@@ -227,133 +218,58 @@ def GetAllEnabledTests(runner_factory, devices):
   raise Exception('No device available to get the list of tests.')
 
 
-def _RunATestSuite(options, suite_name):
-  """Run a single test suite.
-
-  Helper for Dispatch() to allow stop/restart of the emulator across
-  test bundles.  If using the emulator, we start it on entry and stop
-  it on exit.
+def Setup(use_exe_test_runner, suite_name, test_arguments, timeout,
+          cleanup_test_files, tool, build_type, webkit, push_deps,
+          gtest_filter):
+  """Create the test runner factory and tests.
 
   Args:
-    options: options for running the tests.
-    suite_name: name of the test suite being run.
+    use_exe_test_runner: if True, use the executable-based test runner.
+    suite_name: the suite name specified on the command line.
+    test_arguments: Additional arguments to pass to the test binary.
+    timeout: Timeout for each test.
+    cleanup_test_files: Whether or not to cleanup test files on device.
+    tool: Name of the Valgrind tool.
+    build_type: 'Release' or 'Debug'.
+    webkit: Whether the suite is being run from a WebKit checkout.
+    push_deps: If True, push all dependencies to the device.
+    gtest_filter: filter for tests.
 
   Returns:
-    A tuple of (base_test_result.TestRunResult object, exit code).
-
-  Raises:
-    Exception: For various reasons including device failure or failing to reset
-        the test server port.
+    A tuple of (TestRunnerFactory, tests).
   """
-  attached_devices = []
-  buildbot_emulators = []
-
-  if options.use_emulator:
-    buildbot_emulators = emulator.LaunchEmulators(options.emulator_count,
-                                                  options.abi,
-                                                  wait_for_boot=True)
-    attached_devices = [e.device for e in buildbot_emulators]
-  elif options.test_device:
-    attached_devices = [options.test_device]
-  else:
-    attached_devices = android_commands.GetAttachedDevices()
-
-  if not attached_devices:
-    raise Exception('A device must be attached and online.')
-
-  # Reset the test port allocation. It's important to do it before starting
-  # to dispatch any tests.
-  if not ports.ResetTestServerPortAllocation():

[frankf, 2013/07/17 06:02:17]

What happened to the calls to these?

[gkanwar, 2013/07/17 20:31:26]

I think I removed these because I thought we were going to make the -s switch
required, in which case one call in test_runner.py would have been sufficient.
On second thought, I think this makes sense to stay in Setup(), so I'll add this
back.

-    raise Exception('Failed to reset test server port.')
-
-  deps_dir = _GenerateDepsDirUsingIsolate(suite_name, options.build_type)
+  suite_name, suite_path = _FullyQualifiedTestSuite(use_exe_test_runner,
+                                                    suite_name, build_type)
+  # TODO(gkanwar): This breaks the abstraction of having test_dispatcher.py deal
+  # entirely with the devices. Can we do this another way?
+  attached_devices = android_commands.GetAttachedDevices()
 
+  deps_dir = _GenerateDepsDirUsingIsolate(suite_name, build_type)
   # Constructs a new TestRunner with the current options.
-  def RunnerFactory(device, shard_index):
+  def TestRunnerFactory(device, shard_index):
     return test_runner.TestRunner(
         device,
-        options.test_suite,
-        options.test_arguments,
-        options.timeout,
-        options.cleanup_test_files,
-        options.tool,
-        options.build_type,
-        options.webkit,
-        options.push_deps,
+        suite_path,
+        test_arguments,
+        timeout,
+        cleanup_test_files,
+        tool,
+        build_type,
+        webkit,
+        push_deps,
         constants.GTEST_TEST_PACKAGE_NAME,
         constants.GTEST_TEST_ACTIVITY_NAME,
         constants.GTEST_COMMAND_LINE_FILE,
         deps_dir=deps_dir)
 
   # Get tests and split them up based on the number of devices.
-  if options.test_filter:
-    all_tests = [t for t in options.test_filter.split(':') if t]
+  # TODO(gkanwar): Sharding shouldn't happen here.

[frankf, 2013/07/17 04:07:20]

Why not? Test types are free to define what a 'test' is. In this case, it's a
collection of gtests.

[gkanwar, 2013/07/17 20:31:26]

I think it's fine for test types to determine what their 'test' is, but in this
case, we're specifically breaking up tests based on the number of devices, which
makes me feel like this is treading on the toes of test_dispatcher. You
mentioned that you were refactoring gtests, however? Would that refactor affect
this?

[frankf, 2013/07/17 21:08:41]

test_dispatcher doesn't know what a 'test' is. This is an optimization that
gtests do since it's very inefficient for each single test to start/kill a
process (there are thousands of tests in some suites).

On 2013/07/17 20:31:26, gkanwar wrote:

+  if gtest_filter:
+    all_tests = [t for t in gtest_filter.split(':') if t]
   else:
-    all_tests = GetAllEnabledTests(RunnerFactory, attached_devices)
+    all_tests = GetAllEnabledTests(TestRunnerFactory, attached_devices)
   num_devices = len(attached_devices)
   tests = [':'.join(all_tests[i::num_devices]) for i in xrange(num_devices)]
   tests = [t for t in tests if t]
 
-  # Run tests.
-  test_results, exit_code = shard.ShardAndRunTests(
-      RunnerFactory, attached_devices, tests, options.build_type,
-      test_timeout=None, num_retries=options.num_retries)

[frankf, 2013/07/17 04:07:20]

What's the difference between test_timeout and options.timeout above?

[gkanwar, 2013/07/17 20:31:26]

Looks like test_timeout sets the watchdog timeout. This is the timeout for
thread inactivity in the _RunAllTests function. If no thread has started a new
test for that period of time, the JoinAll function will raise a TimeoutError. It
looks like this setting was intended to be set per test_type, not as a
user-facing setting. I can set this back to None for all test types, but in that
case I'm not sure what purpose it's serving, so maybe it should just be removed?

[frankf, 2013/07/17 21:08:41]

Let's not touch this in this CL and revisit it.

On 2013/07/17 20:31:26, gkanwar wrote:

-
-  report_results.LogFull(
-      results=test_results,
-      test_type='Unit test',
-      test_package=suite_name,
-      build_type=options.build_type,
-      flakiness_server=options.flakiness_dashboard_server)
-
-  for buildbot_emulator in buildbot_emulators:
-    buildbot_emulator.Shutdown()
-
-  return (test_results, exit_code)
-
-
-def _ListTestSuites():
-  """Display a list of available test suites."""
-  print 'Available test suites are:'
-  for test_suite in gtest_config.STABLE_TEST_SUITES:
-    print test_suite
-
-
-def Dispatch(options):
-  """Dispatches the tests, sharding if possible.
-
-  If options.use_emulator is True, all tests will be run in new emulator
-  instance.
-
-  Args:
-    options: options for running the tests.
-
-  Returns:
-    base_test_result.TestRunResults object with the results of running the tests
-  """
-  results = base_test_result.TestRunResults()
-
-  if options.test_suite == 'help':
-    _ListTestSuites()
-    return (results, 0)
-
-  if options.use_xvfb:
-    framebuffer = xvfb.Xvfb()
-    framebuffer.Start()
-
-  all_test_suites = _FullyQualifiedTestSuites(options.exe, options.test_suite,
-                                              options.build_type)
-  exit_code = 0
-  for suite_name, suite_path in all_test_suites:
-    # Give each test suite its own copy of options.
-    test_options = copy.deepcopy(options)
-    test_options.test_suite = suite_path
-    test_results, test_exit_code = _RunATestSuite(test_options, suite_name)
-    results.AddTestRunResults(test_results)
-    if test_exit_code and exit_code != constants.ERROR_EXIT_CODE:
-      exit_code = test_exit_code
-
-  if options.use_xvfb:
-    framebuffer.Stop()
-
-  return (results, exit_code)
+  return (TestRunnerFactory, tests)