rebaseline.py: add --bugs and --unreviewed flags

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 142 matching lines @@
   #  actuals_base_url: base URL from which to read actual-result JSON files
   #  actuals_filename: filename (under actuals_base_url) from which to read a
   #                    summary of results; typically "actual-results.json"
   #  exception_handler: reference to rebaseline.ExceptionHandler object
   #  tests: list of tests to rebaseline, or None if we should rebaseline
   #         whatever files the JSON results summary file tells us to
   #  configs: which configs to run for each test, or None if we should
   #           rebaseline whatever configs the JSON results summary file tells
   #           us to
   #  add_new: if True, add expectations for tests which don't have any yet
+  #  bugs: optional list of bug numbers which pertain to these expectations
+  #  notes: free-form text notes to add to all updated expectations
+  #  mark_unreviewed: if True, mark these expectations as NOT having been
+  #                   reviewed by a human; otherwise, leave that field blank.
+  #                   Currently, there is no way to make this script mark
+  #                   expectations as reviewed-by-human=True.
+  #                   TODO(epoger): Add that capability to a review tool.
   def __init__(self, expectations_root, expectations_input_filename,
                expectations_output_filename, actuals_base_url,
                actuals_filename, exception_handler,
-               tests=None, configs=None, add_new=False):
+               tests=None, configs=None, add_new=False, bugs=None, notes=None,
+               mark_unreviewed=None):
     self._expectations_root = expectations_root
     self._expectations_input_filename = expectations_input_filename
     self._expectations_output_filename = expectations_output_filename
     self._tests = tests
     self._configs = configs
     self._actuals_base_url = actuals_base_url
     self._actuals_filename = actuals_filename
     self._exception_handler = exception_handler
     self._add_new = add_new
+    self._bugs = bugs
+    self._notes = notes
+    self._mark_unreviewed = mark_unreviewed
     self._image_filename_re = re.compile(gm_json.IMAGE_FILENAME_PATTERN)
     self._using_svn = os.path.isdir(os.path.join(expectations_root, '.svn'))
 
   # Executes subprocess.call(cmd).
   # Raises an Exception if the command fails.
   def _Call(self, cmd):
     if subprocess.call(cmd) != 0:
       raise _InternalException('error running command: ' + ' '.join(cmd))
 
   # Returns the full contents of filepath, as a single string.
@@ skipping 53 matching lines @@
   # Rebaseline all tests/types we specified in the constructor,
   # within this builder's subdirectory in expectations/gm .
   #
   # params:
   #  builder : e.g. 'Test-Win7-ShuttleA-HD2000-x86-Release'
   def RebaselineSubdir(self, builder):
     # Read in the actual result summary, and extract all the tests whose
     # results we need to update.
     actuals_url = '/'.join([self._actuals_base_url,
                             builder, self._actuals_filename])
-    # In most cases, we won't need to re-record results that are already
-    # succeeding, but including the SUCCEEDED results will allow us to
-    # re-record expectations if they somehow get out of sync.
-    sections = [gm_json.JSONKEY_ACTUALRESULTS_FAILED,
-                gm_json.JSONKEY_ACTUALRESULTS_SUCCEEDED]
+    # Only update results for tests that are currently failing.
+    # We don't want to rewrite results for tests that are already succeeding,
+    # because we don't want to add annotation fields (such as
+    # JSONKEY_EXPECTEDRESULTS_BUGS) except for tests whose expectations we
+    # are actually modifying.
+    sections = [gm_json.JSONKEY_ACTUALRESULTS_FAILED]
     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]
 
     # 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)
             continue
         if self._configs:
           if config not in self._configs:
             skipped_images.append(image_name)
             continue
         if not expected_results.get(image_name):
           expected_results[image_name] = {}
         expected_results[image_name][gm_json.JSONKEY_EXPECTEDRESULTS_ALLOWEDDIGESTS] = \
                         [image_results]
+        if self._mark_unreviewed:
+          expected_results[image_name][gm_json.JSONKEY_EXPECTEDRESULTS_REVIEWED] = \

[borenet, 2013/08/30 13:26:55]

Nit: 80 chars

[epoger, 2013/08/30 14:50:15]

Yes, but for these lines (as well as line 293 above) I couldn't think of another
way of breaking the lines without making it harder to understand.  If you have a
suggestion, I'll take it...

[borenet, 2013/09/03 15:42:53]

Maybe something like this?

if self._mark_unreviewed:
  expected_results[image_name]
      [gm_json.JSONKEY_EXPECTEDRESULTS_REVIEWED] = False

You might be able to line up the ['s as well.

It's admittedly ugly.  Otherwise, if you can't break it up nicely and you're
going over the 80 character limit anyway, I suggest putting the "= False" (etc)
on the same line for easier reading.

[epoger, 2013/09/03 17:28:14]

I took that idea and ran with it.  What do you think of the results?

[borenet, 2013/09/03 17:30:57]

LGTM.  Are the backslashes required?

+                          False
+        if self._bugs:
+          expected_results[image_name][gm_json.JSONKEY_EXPECTEDRESULTS_BUGS] = \
+                          self._bugs
+        if self._notes:

[epoger, 2013/08/30 09:18:25]

Patchset 4 adds --notes flag, and records those notes in the appropriate place
within the JSON.

+          expected_results[image_name][gm_json.JSONKEY_EXPECTEDRESULTS_NOTES] = \

[borenet, 2013/08/30 13:26:55]

80 chars

+                          self._notes
 
     # Write out updated expectations.
     expectations_output_filepath = os.path.join(
         self._expectations_root, builder, self._expectations_output_filename)
     gm_json.WriteToFile(expectations_dict, expectations_output_filepath)
 
     # Mark the JSON file as plaintext, so text-style diffs can be applied.
     # Fixes https://code.google.com/p/skia/issues/detail?id=1442
     if self._using_svn:
       self._Call(['svn', 'propset', '--quiet', 'svn:mime-type',
@@ skipping 10 matching lines @@
                     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('--bugs', metavar='BUG', type=int, nargs='+',
+                    help='Skia bug numbers (under ' +
+                    'https://code.google.com/p/skia/issues/list ) which '+
+                    'pertain to this set of rebaselines.')

[borenet, 2013/09/03 15:42:53]

I'd prefer this syntax for multi-line strings:

parser.add_argument('--bugs', metavar='BUG', type=int, nargs='+',
                    help=('Skia bug numbers (under '
                          'https://code.google.com/p/skia/issues/list ) which '
                          'pertain to this set of rebaselines.'))

[epoger, 2013/09/03 17:28:14]

Done.

 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.
 parser.add_argument('--configs', metavar='CONFIG', nargs='+',
                     help='which configurations to rebaseline, e.g. ' +
                     '"--configs 565 8888", as a filter over the full set of ' +
                     'results in ACTUALS_FILENAME; if unspecified, rebaseline ' +
                     '*all* configs that are available.')
@@ skipping 11 matching lines @@
                     default='')
 parser.add_argument('--expectations-root',
                     help='root of expectations directory to update-- should ' +
                     'contain one or more builder subdirectories. Defaults to ' +
                     '%(default)s',
                     default=os.path.join('expectations', 'gm'))
 parser.add_argument('--keep-going-on-failure', action='store_true',
                     help='instead of halting at the first error encountered, ' +
                     'keep going and rebaseline as many tests as possible, ' +
                     'and then report the full set of errors at the end')
+parser.add_argument('--notes',
+                    help='free-form text notes to add to all updated ' +
+                    'expectations')
 # TODO(epoger): Add test that exercises --tests argument.
 parser.add_argument('--tests', metavar='TEST', nargs='+',
                     help='which tests to rebaseline, e.g. ' +
                     '"--tests aaclip bigmatrix", as a filter over the full ' +
                     'set of results in ACTUALS_FILENAME; if unspecified, ' +
                     'rebaseline *all* tests that are available.')
+parser.add_argument('--unreviewed', action='store_true',
+                    help='mark all expectations modified by this run as ' +
+                    '"%s": False' % gm_json.JSONKEY_EXPECTEDRESULTS_REVIEWED)
 args = parser.parse_args()
 exception_handler = ExceptionHandler(
     keep_going_on_failure=args.keep_going_on_failure)
 if args.builders:
   builders = args.builders
   missing_json_is_fatal = True
 else:
   builders = sorted(TEST_BUILDERS)
   missing_json_is_fatal = False
 for builder in builders:
   if not builder in TEST_BUILDERS:
     raise Exception(('unrecognized builder "%s"; ' +
                      'should be one of %s') % (
                          builder, TEST_BUILDERS))
 
   expectations_json_file = os.path.join(args.expectations_root, builder,
                                         args.expectations_filename)
   if os.path.isfile(expectations_json_file):
     rebaseliner = JsonRebaseliner(
         expectations_root=args.expectations_root,
         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)
+        add_new=args.add_new, bugs=args.bugs, notes=args.notes,
+        mark_unreviewed=args.unreviewed)
     try:
       rebaseliner.RebaselineSubdir(builder=builder)
     except BaseException as e:
       exception_handler.RaiseExceptionOrContinue(e)
   else:
     exception_handler.RaiseExceptionOrContinue(_InternalException(
         'expectations_json_file %s not found' % expectations_json_file))
 
 exception_handler.ReportAllFailures()