sandwich: Uplift for production use.

tools/android/loading/sandwich_runner.py

 # Copyright 2016 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 logging
 import os
 import shutil
 import sys
 import tempfile
 
 _SRC_DIR = os.path.abspath(os.path.join(
     os.path.dirname(__file__), '..', '..', '..'))
 
 sys.path.append(os.path.join(_SRC_DIR, 'third_party', 'catapult', 'devil'))
 from devil.android import device_utils
 
 import chrome_cache
 import controller
 import devtools_monitor
 import device_setup
 import loading_trace
 
 
 # Standard filenames in the sandwich runner's output directory.
+ERROR_FILENAME = 'error'
 TRACE_FILENAME = 'trace.json'
 VIDEO_FILENAME = 'video.mp4'
 WPR_LOG_FILENAME = 'wpr.log'
 
 # Memory dump category used to get memory metrics.
 MEMORY_DUMP_CATEGORY = 'disabled-by-default-memory-infra'
 
 # Devtools timeout of 1 minute to avoid websocket timeout on slow
 # network condition.
 _DEVTOOLS_TIMEOUT = 60
@@ skipping 98 matching lines @@
 
   def _RunNavigation(self, clear_cache, run_id=None):
     """Run a page navigation to the given URL.
 
     Args:
       clear_cache: Whether if the cache should be cleared before navigation.
       run_id: Id of the run in the output directory. If it is None, then no
         trace or video will be saved.
     """
     run_path = None
-    if self.output_dir is not None and run_id is not None:

[pasko, 2016/06/06 13:58:52]

yeah, I always wondered why we needed SandwichRunner.output_dir to be None in
the first place. Was it for some sort of integration test?

[gabadie, 2016/06/06 14:03:55]

No that was just to not record the trace for WPR recording. But it is actually
more important to have error in WPR recording fails rather than not saving the
trace.

+    if run_id is not None:
       run_path = os.path.join(self.output_dir, str(run_id))
       if not os.path.isdir(run_path):
         os.makedirs(run_path)
     self._chrome_ctl.SetNetworkEmulation(
         self._GetEmulatorNetworkCondition('browser'))
     additional_categories = []
     if self.record_memory_dumps:
       additional_categories = [MEMORY_DUMP_CATEGORY]
     # TODO(gabadie): add a way to avoid recording a trace.
     with self._chrome_ctl.Open() as connection:
@@ skipping 30 matching lines @@
         clear_cache = False
         if self.cache_operation == CacheOperation.CLEAR:
           clear_cache = True
         elif self.cache_operation == CacheOperation.PUSH:
           self._chrome_ctl.PushBrowserCache(self._local_cache_directory_path)
         elif self.cache_operation == CacheOperation.SAVE:
           clear_cache = run_id == 0
         self._RunNavigation(clear_cache=clear_cache, run_id=run_id)
         break
       except controller.ChromeControllerError as error:
+        if error.IsIntermittent() and attempt_id + 1 != self._ATTEMPT_COUNT:
+          dump_filename = 'intermittent_failure{}'.format(attempt_id)
+          dump_path = os.path.join(self.output_dir, str(run_id), dump_filename)
+        else:
+          dump_path = os.path.join(self.output_dir, ERROR_FILENAME)
+        with open(dump_path, 'w') as dump_output:
+          error.Dump(dump_output)
         if not error.IsIntermittent():
-          raise
-        if self.output_dir is not None:
-          dump_path = os.path.join(self.output_dir, str(run_id),
-                                   'error{}'.format(attempt_id))
-          with open(dump_path, 'w') as dump_output:
-            error.Dump(dump_output)
+          error.RaiseOriginal()

[pasko, 2016/06/06 13:58:52]

what does not work with raising ChromeControllerError again via "raise" with no
args?

[gabadie, 2016/06/06 14:03:55]

'raise' would raise the previously catched exception. So here it would raise the
ChromeControllerError. The point of RaiseOriginal() is to raise back the
exception that as failed within the with chrome_ctl.Open(). These way we have
the full stack trace outputed in the stderr.

[mattcary, 2016/06/06 14:37:09]

A disadvantage of this approach is that we lose the fact that we handled &
reraised the exception as a ChromeControllerError. An alternative might be to
extend the ChromeControllerError so that it prints out the whole stack trace.

However, doing that reliably I think is difficult. I think re-raising the
original can be a little surprising, but will be more useful than just
propagating the ChromeControllerError.

[gabadie, 2016/06/06 14:55:14]

I don't see where the disadvantage is. The point of the chrome controller was to
provide an exception bundling with additional information about chrome such as
the chrome log. But the quantity of information its contains is slightly too
big, especially with the verbose Android's logcat.

At this call time, the chrome controller error has already dumped himself out
into a file. Therefore his life goal of providing additional debug data is
completed. Then reraising the original exception in case of a non intermittent
failure then become completely fine because it also make the upper layers not
having to handle the ChromeControllerError border case, and giving a clean stack
trace where the original excepetion as been thrown into the stderr, which makes
debugging far more friendly.

     else:
       logging.error('Failed to navigate to %s after %d attemps' % \
                     (self.url, self._ATTEMPT_COUNT))
       raise
 
   def _PullCacheFromDevice(self):
     assert self.cache_operation == CacheOperation.SAVE
     assert self.cache_archive_path, 'Need to specify where to save the cache'
 
     cache_directory_path = self._chrome_ctl.PullBrowserCache()
     chrome_cache.ZipDirectoryContent(
         cache_directory_path, self.cache_archive_path)
     shutil.rmtree(cache_directory_path)
 
   def Run(self):
     """SandwichRunner main entry point meant to be called once configured."""
+    assert self.output_dir is not None
     assert self._chrome_ctl == None
     assert self._local_cache_directory_path == None
-    if self.output_dir:
-      self._CleanTraceOutputDirectory()
+    self._CleanTraceOutputDirectory()
 
     if self.android_device:
       self._chrome_ctl = controller.RemoteChromeController(self.android_device)
     else:
       self._chrome_ctl = controller.LocalChromeController()
     self._chrome_ctl.AddChromeArguments(['--disable-infobars'])
     self._chrome_ctl.AddChromeArguments(self.chrome_args)
     if self.cache_operation == CacheOperation.SAVE:
       self._chrome_ctl.SetSlowDeath()
-
-    wpr_log_path = None
-    if self.output_dir:
-      wpr_log_path = os.path.join(self.output_dir, WPR_LOG_FILENAME)
-
     try:
       if self.cache_operation == CacheOperation.PUSH:
         assert os.path.isfile(self.cache_archive_path)
         self._local_cache_directory_path = tempfile.mkdtemp(suffix='.cache')
         chrome_cache.UnzipDirectoryContent(
             self.cache_archive_path, self._local_cache_directory_path)
 
       with self._chrome_ctl.OpenWprHost(self.wpr_archive_path,
           record=self.wpr_record,
           network_condition_name=self._GetEmulatorNetworkCondition('wpr'),
           disable_script_injection=self.disable_wpr_script_injection,
-          out_log_path=wpr_log_path):
+          out_log_path=os.path.join(self.output_dir, WPR_LOG_FILENAME)):
         for repeat_id in xrange(self.repeat):
           self._RunUrl(run_id=repeat_id)
     finally:
       if self._local_cache_directory_path:
         shutil.rmtree(self._local_cache_directory_path)
         self._local_cache_directory_path = None
     if self.cache_operation == CacheOperation.SAVE:
       self._PullCacheFromDevice()
 
     self._chrome_ctl = None
@@ skipping 11 matching lines @@
     if not os.path.isdir(repeat_dir):
       continue
     try:
       repeat_id = int(node_name)
     except ValueError:
       continue
     yield repeat_id, repeat_dir
     repeated_run_count += 1
   assert repeated_run_count > 0, ('Error: not a sandwich runner output '
                                   'directory: {}').format(runner_output_dir)