rebaseline.py: fix --add-new when there are no expectations at all

tools/rebaseline.py

 #!/usr/bin/python
 
 '''
 Copyright 2012 Google Inc.
 
 Use of this source code is governed by a BSD-style license that can be
 found in the LICENSE file.
 '''
 
 '''
@@ skipping 87 matching lines @@
 # them to display later on.
 class ExceptionHandler(object):
 
   # params:
   #  keep_going_on_failure: if False, report failures and quit right away;
   #                         if True, collect failures until
   #                         ReportAllFailures() is called
   def __init__(self, keep_going_on_failure=False):
     self._keep_going_on_failure = keep_going_on_failure
     self._failures_encountered = []
-    self._exiting = False
-
-  # Exit the program with the given status value.
-  def _Exit(self, status=1):
-    self._exiting = True
-    sys.exit(status)
 
   # We have encountered an exception; either collect the info and keep going,
   # or exit the program right away.

[rmistry, 2013/09/03 17:09:35]

Make this a method level comment.

[epoger, 2013/09/03 18:01:33]

Done throughout ExceptionHandler object.

-  def RaiseExceptionOrContinue(self, e):
-    # If we are already quitting the program, propagate any exceptions
-    # so that the proper exit status will be communicated to the shell.
-    if self._exiting:
-      raise e
+  def RaiseExceptionOrContinue(self):
+    # Get traceback information about the most recently raised exception.
+    exc_info = sys.exc_info()
 
     if self._keep_going_on_failure:
-      print >> sys.stderr, 'WARNING: swallowing exception %s' % e
-      self._failures_encountered.append(e)
+      # I don't understand why we need to log [exc_info[1]] instead of just
+      # exc_info[1] , but for some reason the former preserves more information.

[rmistry, 2013/09/03 17:09:35]

Thats because exc_info[1] is like calling str(exc_info[1]) which is a user
friendly string.
For [exc_info[1]] it calls repr(exc_info[1]).
You should get the output you want by changing [exc_info[1]] to
repr(exc_info[1])

[epoger, 2013/09/03 18:01:33]

Works as you describe... thanks!  Done.

+      print >> sys.stderr, 'WARNING: swallowing exception %s' % [exc_info[1]]
+      self._failures_encountered.append(exc_info)
     else:
-      print >> sys.stderr, e
       print >> sys.stderr, (
           'Halting at first exception; to keep going, re-run ' +
           'with the --keep-going-on-failure option set.')
-      self._Exit()
+      raise exc_info[1], None, exc_info[2]

[epoger, 2013/09/03 16:54:38]

Patchset 5: While I was at it, improved error reporting so that it was a lot
easier to see what was going wrong.

 
   def ReportAllFailures(self):
     if self._failures_encountered:
       print >> sys.stderr, ('Encountered %d failures (see above).' %
                             len(self._failures_encountered))
-      self._Exit()
+      sys.exit(1)
 
 
 # Object that rebaselines a JSON expectations file (not individual image files).
 class JsonRebaseliner(object):
 
   # params:
   #  expectations_root: root directory of all expectations JSON files
   #  expectations_input_filename: filename (under expectations_root) of JSON
   #                               expectations file to read; typically
   #                               "expected-results.json"
@@ skipping 105 matching lines @@
                 gm_json.JSONKEY_ACTUALRESULTS_SUCCEEDED]
     if self._add_new:
       sections.append(gm_json.JSONKEY_ACTUALRESULTS_NOCOMPARISON)
     results_to_update = self._GetActualResults(json_url=actuals_url,
                                                sections=sections)
 
     # Read in current expectations.
     expectations_input_filepath = os.path.join(
         self._expectations_root, builder, self._expectations_input_filename)
     expectations_dict = gm_json.LoadFromFile(expectations_input_filepath)
-    expected_results = expectations_dict[gm_json.JSONKEY_EXPECTEDRESULTS]
+    expected_results = expectations_dict.get(gm_json.JSONKEY_EXPECTEDRESULTS)
+    if not expected_results:

[epoger, 2013/09/03 16:54:38]

Patchset 5: properly handle the case where there are no expectations at all for
a given builder.

+      expected_results = {}

[rmistry, 2013/09/03 17:09:35]

I think you should be able to get rid of this line by changing line:256 to

expected_results = expectations_dict.get(gm_json.JSONKEY_EXPECTEDRESULTS, {})

[epoger, 2013/09/03 18:01:33]

Done, but we still need to set
expectations_dict[gm_json.JSONKEY_EXPECTEDRESULTS] = expected_results if there
weren't any expectations yet.

Do you find the new code preferable?  I actually prefer the old code... one more
line of code, but (IMHO) easier to follow.

if 

[rmistry, 2013/09/03 18:17:18]

Lets go with what you prefer, not a big deal either way to me.

[epoger, 2013/09/03 18:23:07]

Went back to my old way, then. :-)

+      expectations_dict[gm_json.JSONKEY_EXPECTEDRESULTS] = expected_results
 
     # Update the expectations in memory, skipping any tests/configs that
     # the caller asked to exclude.
     skipped_images = []
     if results_to_update:
       for (image_name, image_results) in results_to_update.iteritems():
         (test, config) = self._image_filename_re.match(image_name).groups()
         if self._tests:
           if test not in self._tests:
             skipped_images.append(image_name)
@@ skipping 23 matching lines @@
 parser = argparse.ArgumentParser()
 parser.add_argument('--actuals-base-url',
                     help='base URL from which to read files containing JSON ' +
                     'summaries of actual GM results; defaults to %(default)s',
                     default='http://skia-autogen.googlecode.com/svn/gm-actual')
 parser.add_argument('--actuals-filename',
                     help='filename (within builder-specific subdirectories ' +
                     'of ACTUALS_BASE_URL) to read a summary of results from; ' +
                     'defaults to %(default)s',
                     default='actual-results.json')
-# TODO(epoger): Add test that exercises --add-new argument.
 parser.add_argument('--add-new', action='store_true',
                     help='in addition to the standard behavior of ' +
                     'updating expectations for failing tests, add ' +
                     'expectations for tests which don\'t have expectations ' +
                     'yet.')
 parser.add_argument('--builders', metavar='BUILDER', nargs='+',
                     help='which platforms to rebaseline; ' +
                     'if unspecified, rebaseline all platforms, same as ' +
                     '"--builders %s"' % ' '.join(sorted(TEST_BUILDERS)))
 # TODO(epoger): Add test that exercises --configs argument.
@@ skipping 52 matching lines @@
         expectations_input_filename=args.expectations_filename,
         expectations_output_filename=(args.expectations_filename_output or
                                       args.expectations_filename),
         tests=args.tests, configs=args.configs,
         actuals_base_url=args.actuals_base_url,
         actuals_filename=args.actuals_filename,
         exception_handler=exception_handler,
         add_new=args.add_new)
     try:
       rebaseliner.RebaselineSubdir(builder=builder)
-    except BaseException as e:
-      exception_handler.RaiseExceptionOrContinue(e)
+    except BaseException:
+      exception_handler.RaiseExceptionOrContinue()
   else:
-    exception_handler.RaiseExceptionOrContinue(_InternalException(
-        'expectations_json_file %s not found' % expectations_json_file))
+    try:
+      raise _InternalException('expectations_json_file %s not found' %
+                               expectations_json_file)
+    except BaseException:

[rmistry, 2013/09/03 17:09:35]

Why is this catching BaseException instead of Exception which is what
_InternalException inherits from?

[epoger, 2013/09/03 18:01:33]

I don't think it really matters.  Got rid of the explicit exception type.

[rmistry, 2013/09/03 18:17:18]

I think it does slightly matter because 'except BaseException' catches more than
the subclasses of Exception.

+      exception_handler.RaiseExceptionOrContinue()
 
 exception_handler.ReportAllFailures()