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

tools/rebaseline.py

Index: tools/rebaseline.py
===================================================================
--- tools/rebaseline.py	(revision 11054)
+++ tools/rebaseline.py	(working copy)
@@ -105,36 +105,29 @@
   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).
@@ -260,7 +253,10 @@
     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.
@@ -304,7 +300,6 @@
                     '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 ' +
@@ -377,10 +372,13 @@
         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()