Stop build on failed device status check, apk install and add CheckInstall step.

build/android/buildbot/bb_device_steps.py

 #!/usr/bin/env python
 # Copyright (c) 2013 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.
 
 import collections
 import glob
 import json
 import multiprocessing
 import optparse
@@ skipping 43 matching lines @@
     I_TEST('AndroidWebView',
            'AndroidWebView.apk',
            'org.chromium.android_webview.shell',
            'AndroidWebViewTest',
            'webview:android_webview/test/data/device_files',
            None),
     ])
 
 VALID_TESTS = set(['chromedriver', 'ui', 'unit', 'webkit', 'webkit_layout'])
 
+NUM_CHECK_INSTALL = 3
 
 
 def SpawnCmd(command):
   """Spawn a process without waiting for termination."""
   print '>', ' '.join(map(pipes.quote, command))
   sys.stdout.flush()
   if TESTING:
     class MockPopen(object):
       @staticmethod
       def wait():
         return 0
     return MockPopen()
 
   return subprocess.Popen(command, cwd=CHROME_SRC)
 
-def RunCmd(command, flunk_on_failure=True):
+def RunCmd(command, flunk_on_failure=True, halt_on_failure=False):
   """Run a command relative to the chrome source root."""
   code = SpawnCmd(command).wait()
   print '<', ' '.join(map(pipes.quote, command))
   if code != 0:
     print 'ERROR: process exited with code %d' % code
     if flunk_on_failure:
       buildbot_report.PrintError()
     else:
       buildbot_report.PrintWarning()
+    # Allow steps to have both halting (i.e. 1) and non-halting exit codes.
+    if code == 1 and halt_on_failure:

[Isaac (away), 2013/04/30 20:01:16]

How about if "code != 88"

[navabi, 2013/04/30 21:41:46]

Done (assuming you mean != 88 and != 0).

+      exit(1)

[Isaac (away), 2013/04/30 20:01:16]

How about an exception instead of sys.exit:

raise subprocess.CalledProcessError()

[Isaac (away), 2013/04/30 20:03:38]

actually looks like that exception takes arguments.

raise OSError() would work.

[navabi, 2013/04/30 21:41:46]

Done.

   return code
 
 
 # multiprocessing map_async requires a top-level function for pickle library.
 def RebootDeviceSafe(device):
   """Reboot a device, wait for it to start, and squelch timeout exceptions."""
   try:
     android_commands.AndroidCommands(device).Reboot(True)
   except errors.DeviceUnresponsiveError as e:
     return e
@@ skipping 55 matching lines @@
   buildbot_report.PrintNamedStep(constants.BROWSERTEST_SUITE_NAME)
   RunCmd(['build/android/run_browser_tests.py'] + args)
 
 def RunChromeDriverTests():
   """Run all the steps for running chromedriver tests."""
   buildbot_report.PrintNamedStep('chromedriver_annotation')
   RunCmd(['chrome/test/chromedriver/run_buildbot_steps.py',
           '--android-package=%s' % constants.CHROMIUM_TEST_SHELL_PACKAGE])
 
 
+def CheckInstall():
+  """Build bot step to see if adb install works on attached devices. """
+  buildbot_report.PrintNamedStep('Check device install')
+  # This step checks if apks can be installed on the devices. The apk installed
+  # is not important. SDK Controller is checked out as part of the Android SDK.
+  args = ['--apk',
+          'third_party/android_tools/sdk/tools/apps/SdkController/bin/'
+          'SdkControllerApp.apk']
+  for _ in range(NUM_CHECK_INSTALL):
+    RunCmd(['build/android/adb_install_apk.py'] + args, halt_on_failure=True)
+
+
 def InstallApk(options, test, print_step=False):
   """Install an apk to all phones.
 
   Args:
     options: options object
     test: An I_TEST namedtuple
     print_step: Print a buildbot step
   """
   if print_step:
     buildbot_report.PrintNamedStep('install_%s' % test.name.lower())
   args = ['--apk', test.apk, '--apk_package', test.apk_package]
   if options.target == 'Release':
     args.append('--release')
 
-  RunCmd(['build/android/adb_install_apk.py'] + args)
+  RunCmd(['build/android/adb_install_apk.py'] + args, halt_on_failure=True)

[navabi, 2013/04/27 00:25:46]

This will make the build stop if an apk install fails. Before any call to
InstallApk (most come from RunInstrumentationSuite), there is a new
CheckInstall() step to make sure APKs can be installed on attached devices.
Thus, that should catch most issues, but this will fail if something goes wrong
between the CheckInstall() step and the step to run the instrumentation tests.

Note: Unit tests do not install via InstallAPK. It installs in
test_package_apk.StripAndCopyExecutable which calls ManagedInstall. Thus,
CheckInstall is still needed as it is a step that is run regardless of what
tests are run.

[Isaac (away), 2013/04/30 20:01:16]

I'm confused why this isn't sufficient -- why do we need the check install?

[navabi, 2013/04/30 21:41:46]

Because not all test suites install apk's with adb_install_apk.py. This is
sufficient for RunInstrumentationSuite, since it calls InstallApk. The unit
tests, for example, do not install the apk using InstallApk (or even
adb_install_apk.py). They install with ManagedInstall in android_commands.py.
Thus, CheckInstall is needed to cover all cases.

[navabi, 2013/04/30 21:43:55]

For more information see the Note on the comment above. It explains the same
thing.

[Isaac (away), 2013/04/30 21:53:17]

That makes sense. Maybe in those cases we could change the test suite to early
terminate if install falls (it may do so already).

Frank, would be OK w/ us changing run_tests to return warning code 88 on
non-fatal errors (i.e. test failures)?

 
 
 def RunInstrumentationSuite(options, test):
   """Manages an invocation of run_instrumentaiton_tests.py.
 
   Args:
     options: options object
     test: An I_TEST namedtuple
   """
   buildbot_report.PrintNamedStep('%s_instrumentation_tests' % test.name.lower())
@@ skipping 73 matching lines @@
   SpawnCmd(['build/android/adb_logcat_monitor.py', logcat_dir])
 
   # Wait for logcat_monitor to pull existing logcat
   RunCmd(['sleep', '5'])
 
   if options.reboot:
     RebootDevices()
 
   # Device check and alert emails
   buildbot_report.PrintNamedStep('device_status_check')
-  RunCmd(['build/android/device_status_check.py'])
+  RunCmd(['build/android/device_status_check.py'], halt_on_failure=True)

[Isaac (away), 2013/04/30 21:53:17]

devise_status_check returns error codes on things that should not halt the
build.  Script needs to be changed to return warning code.

Warning code (88) should probably be put in pylib/constants.py.

[navabi, 2013/04/30 21:59:56]

I don't think that is the case. The main function of device_status_check will
either exit normally or return with code 1 if there are no devices attached.

What other error codes may be returned by device_status_check with should not
halt the build?

[Isaac (away), 2013/04/30 22:07:08]

Sorry, yes you're right.  I forgot Siva changed this is a recent CL.

 
   # Provision devices
   buildbot_report.PrintNamedStep('provision_devices')
   target = options.factory_properties.get('target', 'Debug')
   RunCmd(['build/android/provision_devices.py', '-t', target])
 
+  # Check to see if devices can install apks.
+  CheckInstall()
+
   if options.install:
     test_obj = INSTRUMENTATION_TESTS[options.install]
     InstallApk(options, test_obj, print_step=True)
 
   if 'chromedriver' in options.test_filter:
     RunChromeDriverTests()
   if 'unit' in options.test_filter:
     RunTestSuites(options, gtest_config.STABLE_TEST_SUITES)
     RunBrowserTestSuite(options)
   if 'ui' in options.test_filter:
@@ skipping 78 matching lines @@
       'slave', 'android'))
   if os.path.exists(build_internal_android):
     android_paths.insert(0, build_internal_android)
   os.environ['PATH'] = os.pathsep.join(android_paths + [os.environ['PATH']])
 
   MainTestWrapper(options)
 
 
 if __name__ == '__main__':
   sys.exit(main(sys.argv))