sandwich: Uplift for production use.

tools/android/loading/sandwich_runner.py

Index: tools/android/loading/sandwich_runner.py
diff --git a/tools/android/loading/sandwich_runner.py b/tools/android/loading/sandwich_runner.py
index 9ed8258f63ecbc827a404831d7ccfafe14b7ad3b..d2ab1b395c381cf283699ee1a15bef7be6497815 100644
--- a/tools/android/loading/sandwich_runner.py
+++ b/tools/android/loading/sandwich_runner.py
@@ -22,6 +22,7 @@ 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'
@@ -140,7 +141,7 @@ class SandwichRunner(object):
         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)
@@ -191,13 +192,15 @@ class SandwichRunner(object):
         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))
@@ -214,10 +217,10 @@ class SandwichRunner(object):
 
   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)
@@ -227,11 +230,6 @@ class SandwichRunner(object):
     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)
@@ -243,7 +241,7 @@ class SandwichRunner(object):
           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: