rebaseline.py: if expectations dir contains JSON format results, update those instead of image files

tools/rebaseline.py

Index: tools/rebaseline.py
===================================================================
--- tools/rebaseline.py	(revision 9897)
+++ tools/rebaseline.py	(working copy)
@@ -17,7 +17,6 @@
 import argparse
 import os
 import re
-import subprocess
 import sys
 import urllib2
 
@@ -41,8 +40,6 @@
     sys.path.append(GM_DIRECTORY)
 import gm_json
 
-JSON_EXPECTATIONS_FILENAME='expected-results.json'
-
 # Mapping of gm-expectations subdir (under
 # https://skia.googlecode.com/svn/gm-expected/ )
 # to builder name (see list at http://108.170.217.252:10117/builders )
@@ -78,130 +75,45 @@
     pass
 
 # Object that rebaselines a JSON expectations file (not individual image files).
-#
-# TODO(epoger): Most of this is just the code from the old ImageRebaseliner...
-# some of it will need to be updated in order to properly rebaseline JSON files.
-# There is a lot of code duplicated between here and ImageRebaseliner, but
-# that's fine because we will delete ImageRebaseliner soon.
 class JsonRebaseliner(object):
 
     # params:
-    #  expectations_root: root directory of all expectations
-    #  json_base_url: base URL from which to read json_filename
-    #  json_filename: filename (under json_base_url) from which to read a
-    #                 summary of results; typically "actual-results.json"
+    #  expectations_root: root directory of all expectations JSON files
+    #  expectations_filename: filename (under expectations_root) of JSON
+    #                         expectations file; typically
+    #                         "expected-results.json"
+    #  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"
     #  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; this should only be
     #           specified if the list of tests was also specified (otherwise,
     #           the JSON file will give us test names and configs)
-    #  dry_run: if True, instead of actually downloading files or adding
-    #           files to checkout, display a list of operations that
-    #           we would normally perform
     #  add_new: if True, add expectations for tests which don't have any yet
-    #  missing_json_is_fatal: whether to halt execution if we cannot read a
-    #                         JSON actual result summary file
-    def __init__(self, expectations_root, json_base_url, json_filename,
-                 tests=None, configs=None, dry_run=False,
-                 add_new=False, missing_json_is_fatal=False):
-        raise ValueError('JsonRebaseliner not yet implemented') # TODO(epoger)
+    def __init__(self, expectations_root, expectations_filename,
+                 actuals_base_url, actuals_filename,
+                 tests=None, configs=None, add_new=False):
         if configs and not tests:
             raise ValueError('configs should only be specified if tests ' +
                              'were specified also')
         self._expectations_root = expectations_root
+        self._expectations_filename = expectations_filename
         self._tests = tests
         self._configs = configs
-        self._json_base_url = json_base_url
-        self._json_filename = json_filename
-        self._dry_run = dry_run
+        self._actuals_base_url = actuals_base_url
+        self._actuals_filename = actuals_filename
         self._add_new = add_new
-        self._missing_json_is_fatal = missing_json_is_fatal
-        self._googlestorage_gm_actuals_root = (
-            'http://chromium-skia-gm.commondatastorage.googleapis.com/gm')
         self._testname_pattern = re.compile('(\S+)_(\S+).png')
-        self._is_svn_checkout = (
-            os.path.exists('.svn') or
-            os.path.exists(os.path.join(os.pardir, '.svn')))
-        self._is_git_checkout = (
-            os.path.exists('.git') or
-            os.path.exists(os.path.join(os.pardir, '.git')))
 
-    # If dry_run is False, execute subprocess.call(cmd).
-    # If dry_run is True, print the command we would have otherwise run.
-    # Raises a CommandFailedException if the command fails.
-    def _Call(self, cmd):
-        if self._dry_run:
-            print '%s' % ' '.join(cmd)
-            return
-        if subprocess.call(cmd) != 0:
-            raise CommandFailedException('error running command: ' +
-                                         ' '.join(cmd))
-
-    # Download a single actual result from GoogleStorage, returning True if it
-    # succeeded.
-    def _DownloadFromGoogleStorage(self, infilename, outfilename, all_results):

[epoger, 2013/07/03 20:18:12]

A lot of the old code (which lives on in rebaseline_imagefiles.py) is no longer
needed here.

-        test_name = self._testname_pattern.match(infilename).group(1)
-        if not test_name:
-            print '# unable to find test_name for infilename %s' % infilename
-            return False
-        try:
-            hash_type, hash_value = all_results[infilename]
-        except KeyError:
-            print ('# unable to find filename %s in all_results dict' %
-                   infilename)
-            return False
-        except ValueError as e:
-            print '# ValueError reading filename %s from all_results dict: %s'%(
-                infilename, e)
-            return False
-        url = '%s/%s/%s/%s.png' % (self._googlestorage_gm_actuals_root,
-                                   hash_type, test_name, hash_value)
-        try:
-            self._DownloadFile(source_url=url, dest_filename=outfilename)
-            return True
-        except CommandFailedException:
-            print '# Couldn\'t fetch gs_url %s' % url
-            return False
-
-    # Download a single actual result from skia-autogen, returning True if it
-    # succeeded.
-    def _DownloadFromAutogen(self, infilename, outfilename,
-                             expectations_subdir, builder_name):
-        url = ('http://skia-autogen.googlecode.com/svn/gm-actual/' +
-               expectations_subdir + '/' + builder_name + '/' +
-               expectations_subdir + '/' + infilename)
-        try:
-            self._DownloadFile(source_url=url, dest_filename=outfilename)
-            return True
-        except CommandFailedException:
-            print '# Couldn\'t fetch autogen_url %s' % url
-            return False
-
-    # Download a single file, raising a CommandFailedException if it fails.
-    def _DownloadFile(self, source_url, dest_filename):
-        # Download into a temporary file and then rename it afterwards,
-        # so that we don't corrupt the existing file if it fails midway thru.
-        temp_filename = os.path.join(os.path.dirname(dest_filename),
-                                     '.temp-' + os.path.basename(dest_filename))
-
-        # TODO(epoger): Replace calls to "curl"/"mv" (which will only work on
-        # Unix) with a Python HTTP library (which should work cross-platform)
-        self._Call([ 'curl', '--fail', '--silent', source_url,
-                     '--output', temp_filename ])
-        self._Call([ 'mv', temp_filename, dest_filename ])
-
-    # Returns the full contents of a URL, as a single string.
-    #
-    # Unlike standard URL handling, we allow relative "file:" URLs;
-    # for example, "file:one/two" resolves to the file ./one/two
-    # (relative to current working dir)
-    def _GetContentsOfUrl(self, url):
-        file_prefix = 'file:'
-        if url.startswith(file_prefix):
-            filename = url[len(file_prefix):]
-            return open(filename, 'r').read()
+    # Returns the full contents of filepath, as a single string.
+    # If filepath looks like a URL, try to read it that way instead of as
+    # a path on local storage.
+    def _GetFileContents(self, filepath):
+        if filepath.startswith('http:') or filepath.startswith('https:'):
+            return urllib2.urlopen(filepath).read()
         else:
-            return urllib2.urlopen(url).read()
+            return open(filepath, 'r').read()
 
     # Returns a dictionary of actual results from actual-results.json file.
     #
@@ -212,10 +124,8 @@
     #  u'shadertext3_8888.png': [u'bitmap-64bitMD5', 3713708307125704716]
     # }
     #
-    # If the JSON actual result summary file cannot be loaded, the behavior
-    # depends on self._missing_json_is_fatal:
-    # - if true: execution will halt with an exception
-    # - if false: we will log an error message but return an empty dictionary
+    # If the JSON actual result summary file cannot be loaded, raise an
+    # exception.
     #
     # params:
     #  json_url: URL pointing to a JSON actual result summary file
@@ -224,16 +134,7 @@
     #             gm_json.JSONKEY_ACTUALRESULTS_NOCOMPARISON] ;
     #            if None, then include ALL sections.
     def _GetActualResults(self, json_url, sections=None):
-        try:
-            json_contents = self._GetContentsOfUrl(json_url)
-        except (urllib2.HTTPError, IOError):
-            message = 'unable to load JSON summary URL %s' % json_url
-            if self._missing_json_is_fatal:
-                raise ValueError(message)
-            else:
-                print '# %s' % message
-                return {}
-
+        json_contents = self._GetFileContents(json_url)
         json_dict = gm_json.LoadFromString(json_contents)
         results_to_return = {}
         actual_results = json_dict[gm_json.JSONKEY_ACTUALRESULTS]
@@ -245,172 +146,107 @@
                 results_to_return.update(section_results)
         return results_to_return
 
-    # Returns a list of files that require rebaselining.
+    # Rebaseline all tests/types we specified in the constructor,
+    # within this gm-expectations subdir.
     #
-    # Note that this returns a list of FILES, like this:
-    #  ['imageblur_565.png', 'xfermodes_pdf.png']
-    # rather than a list of TESTS, like this:
-    #  ['imageblur', 'xfermodes']
-    #
     # params:
-    #  json_url: URL pointing to a JSON actual result summary file
-    #  add_new: if True, then return files listed in any of these sections:
-    #            - JSONKEY_ACTUALRESULTS_FAILED
-    #            - JSONKEY_ACTUALRESULTS_NOCOMPARISON
-    #           if False, then return files listed in these sections:
-    #            - JSONKEY_ACTUALRESULTS_FAILED
-    #
-    def _GetFilesToRebaseline(self, json_url, add_new):
-        if self._dry_run:
-            print ''
-            print '#'
-        print ('# Getting files to rebaseline from JSON summary URL %s ...'
-               % json_url)
+    #  subdir : e.g. 'base-shuttle-win7-intel-float'
+    #  builder : e.g. 'Test-Win7-ShuttleA-HD2000-x86-Release'
+    def RebaselineSubdir(self, subdir, 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,
+                                subdir, builder, subdir,
+                                self._actuals_filename])
         sections = [gm_json.JSONKEY_ACTUALRESULTS_FAILED]

[Stephen White, 2013/07/03 20:46:19]

Not new to this patch, IWBN to have some comments describing what these section
keys are (or if there are comments elsewhere, perhaps a reference to them). 
Also, JSONKEY_ACTUALRESULTS_NEW would make more sense to me than
JSON_KEY_ACTUALRESULTS_NOCOMPARISON, if these are new results.

[epoger, 2013/07/08 14:40:54]

Good idea.  I have added comments within gm_json.py.

For now at least, I'm leaving the name as JSON_KEY_ACTUALRESULTS_NOCOMPARISON
(because, for one thing, changing the string value from "no-comparison" to
something else would invalidate any existing GM JSON files).  The comment I
added in gm_json.py should help explain the name.

[Stephen White, 2013/07/08 17:15:19]

OK. Just FYI, I think the Blink equivalent of this state is MISSING.

-        if add_new:
+        if self._add_new:
             sections.append(gm_json.JSONKEY_ACTUALRESULTS_NOCOMPARISON)
-        results_to_rebaseline = self._GetActualResults(json_url=json_url,
-                                                       sections=sections)
-        files_to_rebaseline = results_to_rebaseline.keys()
-        files_to_rebaseline.sort()
-        print '# ... found files_to_rebaseline %s' % files_to_rebaseline
-        if self._dry_run:
-            print '#'
-        return files_to_rebaseline
+        results_to_update = self._GetActualResults(json_url=actuals_url,
+                                                   sections=sections)
 
-    # Rebaseline a single file.
-    def _RebaselineOneFile(self, expectations_subdir, builder_name,
-                           infilename, outfilename, all_results):
-        if self._dry_run:
-            print ''
-        print '# ' + infilename
+        # Read in current expectations.
+        expectations_json_filepath = os.path.join(
+            self._expectations_root, subdir, self._expectations_filename)
+        expectations_dict = gm_json.LoadFromFile(expectations_json_filepath)
 
-        # First try to download this result image from Google Storage.
-        # If that fails, try skia-autogen.
-        # If that fails too, just go on to the next file.
-        #
-        # This not treated as a fatal failure because not all
-        # platforms generate all configs (e.g., Android does not
-        # generate PDF).
-        #
-        # TODO(epoger): Once we are downloading only files that the
-        # actual-results.json file told us to, this should become a
-        # fatal error.  (If the actual-results.json file told us that
-        # the test failed with XXX results, we should be able to download
-        # those results every time.)
-        if not self._DownloadFromGoogleStorage(infilename=infilename,
-                                               outfilename=outfilename,
-                                               all_results=all_results):
-            if not self._DownloadFromAutogen(infilename=infilename,
-                                             outfilename=outfilename,
-                                             expectations_subdir=expectations_subdir,
-                                             builder_name=builder_name):
-                print '# Couldn\'t fetch infilename ' + infilename
-                return
+        # 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._testname_pattern.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
+                expectations_dict[gm_json.JSONKEY_EXPECTEDRESULTS] \
+                                 [image_name] \
+                                 [gm_json.JSONKEY_EXPECTEDRESULTS_ALLOWEDDIGESTS] = \

[Stephen White, 2013/07/03 20:46:19]

Similarly, I have no idea what ALLOWEDDIGESTS means. A comment or reference to
docs would be great.

[epoger, 2013/07/08 14:40:54]

See new comments in gm_json.py

+                                     [image_results]
 
-        # Add this file to version control (if appropriate).
-        if self._add_new:
-            if self._is_svn_checkout:
-                cmd = [ 'svn', 'add', '--quiet', outfilename ]
-                self._Call(cmd)
-                cmd = [ 'svn', 'propset', '--quiet', 'svn:mime-type',
-                        'image/png', outfilename ];
-                self._Call(cmd)
-            elif self._is_git_checkout:
-                cmd = [ 'git', 'add', outfilename ]
-                self._Call(cmd)
+        # Write out updated expectations.
+        gm_json.WriteToFile(expectations_dict, expectations_json_filepath)
 
-    # Rebaseline the given configs for a single test.
-    #
-    # params:
-    #  expectations_subdir
-    #  builder_name
-    #  test: a single test to rebaseline
-    #  all_results: a dictionary of all actual results
-    def _RebaselineOneTest(self, expectations_subdir, builder_name, test,
-                           all_results):
-        if self._configs:
-            configs = self._configs
-        else:
-            if (expectations_subdir == 'base-shuttle-win7-intel-angle'):
-                configs = [ 'angle', 'anglemsaa16' ]
-            else:
-                configs = [ '565', '8888', 'gpu', 'pdf', 'mesa', 'msaa16',
-                            'msaa4' ]
-        if self._dry_run:
-            print ''
-        print '# ' + expectations_subdir + ':'
-        for config in configs:
-            infilename = test + '_' + config + '.png'
-            outfilename = os.path.join(expectations_subdir, infilename);
-            self._RebaselineOneFile(expectations_subdir=expectations_subdir,
-                                    builder_name=builder_name,
-                                    infilename=infilename,
-                                    outfilename=outfilename,
-                                    all_results=all_results)
+        if skipped_images:
+            print ('Skipped these tests due to test/config filters: %s' %
+                   skipped_images)
 
-    # Rebaseline all tests/types we specified in the constructor,
-    # within this gm-expectations subdir.
-    #
-    # params:
-    #  subdir : e.g. 'base-shuttle-win7-intel-float'
-    #  builder : e.g. 'Test-Win7-ShuttleA-HD2000-x86-Release'
-    def RebaselineSubdir(self, subdir, builder):
-        json_url = '/'.join([self._json_base_url,
-                             subdir, builder, subdir,
-                             self._json_filename])
-        all_results = self._GetActualResults(json_url=json_url)
 
-        if self._tests:
-            for test in self._tests:
-                self._RebaselineOneTest(expectations_subdir=subdir,
-                                        builder_name=builder,
-                                        test=test, all_results=all_results)
-        else:  # get the raw list of files that need rebaselining from JSON
-            filenames = self._GetFilesToRebaseline(json_url=json_url,
-                                                   add_new=self._add_new)
-            for filename in filenames:
-                outfilename = os.path.join(subdir, filename);
-                self._RebaselineOneFile(expectations_subdir=subdir,
-                                        builder_name=builder,
-                                        infilename=filename,
-                                        outfilename=outfilename,
-                                        all_results=all_results)
-
 # main...
 
 parser = argparse.ArgumentParser()
+parser.add_argument('--actuals-base-url',
+                    help='base URL from which to read ACTUALS_FILENAME ' +

[Stephen White, 2013/07/03 20:46:19]

I'm not sure "ACTUALS_FILENAME" is meaningful to someone using this tool. Is
there something simpler/clearer we could display to the user?

[epoger, 2013/07/08 14:40:54]

ACTUALS_FILENAME is part of the --help output generated by argparse (see below).
 But I have modified this a bit.... hopefully it's clearer.  If not, I welcome
any wording suggestions you might have. :-)


$ tools/rebaseline.py --help
usage: rebaseline.py [-h] [--actuals-base-url ACTUALS_BASE_URL]
                     [--actuals-filename ACTUALS_FILENAME] [--add-new]
                     [--configs CONFIG [CONFIG ...]] [--dry-run]
                     [--expectations-filename EXPECTATIONS_FILENAME]
                     [--expectations-root EXPECTATIONS_ROOT]
                     [--subdirs SUBDIR [SUBDIR ...]] [--tests TEST [TEST ...]]

optional arguments:
  -h, --help            show this help message and exit
  --actuals-base-url ACTUALS_BASE_URL
                        base URL from which to read files containing JSON
                        summaries of actual GM results; defaults to http
                        ://skia-autogen.googlecode.com/svn/gm-actual
  --actuals-filename ACTUALS_FILENAME
                        filename (within platform-specific subdirectories of
                        ACTUALS_BASE_URL) to read a summary of results from;
                        defaults to actual-results.json
  --add-new             in addition to the standard behavior of updating
                        expectations for failing tests, add expectations for
                        tests which don't have expectations yet.
  --configs CONFIG [CONFIG ...]
                        which configurations to rebaseline, e.g. "--configs
                        565 8888"; if unspecified, run a default set of
                        configs. This should ONLY be specified if --tests has
                        also been specified.
  --dry-run             instead of actually downloading files or adding files
                        to checkout, display a list of operations that we
                        would normally perform
  --expectations-filename EXPECTATIONS_FILENAME
                        filename (under EXPECTATIONS_ROOT) to read current
                        expectations from, and to write new expectations into;
                        defaults to expected-results.json
  --expectations-root EXPECTATIONS_ROOT
                        root of expectations directory to update-- should
                        contain one or more base-* subdirectories. Defaults to
                        .
  --subdirs SUBDIR [SUBDIR ...]
                        which platform subdirectories to rebaseline; if
                        unspecified, rebaseline all subdirs, same as "--
                        subdirs base-android-galaxy-nexus base-android-
                        nexus-10 base-android-nexus-4 base-android-nexus-7
                        base-android-nexus-s base-android-xoom base-macmini
                        base-macmini-lion-float base-shuttle-win7-intel-angle
                        base-shuttle-win7-intel-directwrite base-shuttle-win7
                        -intel-float base-shuttle_ubuntu12_ati5770"
  --tests TEST [TEST ...]
                        which tests to rebaseline, e.g. "--tests aaclip
                        bigmatrix"; if unspecified, then all failing tests
                        (according to the actual-results.json file) will be
                        rebaselined.

[Stephen White, 2013/07/08 17:15:19]

Much better, thanks!

+                    'files; defaults to %(default)s',
+                    default='http://skia-autogen.googlecode.com/svn/gm-actual')
+parser.add_argument('--actuals-filename',
+                    help='filename (under ACTUALS_BASE_URL) to read a ' +

[Stephen White, 2013/07/03 20:46:19]

Same here for "ACTUALS_BASE_URL".

[epoger, 2013/07/08 14:40:54]

Looking at the --help output, I think it's good as is... any particular
suggestion?

+                    '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.')
+# TODO(epoger): Add test that exercises --configs argument.
+# TODO(epoger): Once we are only rebaselining JSON files, update the helpstring
+# to indicate that this is a *filter* over the config names that
+# actual-results.json tells us need to be rebaselined.
+# You don't need to specify tests also, etc.
 parser.add_argument('--configs', metavar='CONFIG', nargs='+',
                     help='which configurations to rebaseline, e.g. ' +
                     '"--configs 565 8888"; if unspecified, run a default ' +
                     'set of configs. This should ONLY be specified if ' +
                     '--tests has also been specified.')
+# TODO(epoger): The --dry-run argument will no longer be needed once we
+# are only rebaselining JSON files.
 parser.add_argument('--dry-run', action='store_true',
                     help='instead of actually downloading files or adding ' +
                     'files to checkout, display a list of operations that ' +
                     'we would normally perform')
+parser.add_argument('--expectations-filename',
+                    help='filename (under EXPECTATIONS_ROOT) to read ' +
+                    'current expectations from, and to write new ' +
+                    'expectations into; defaults to %(default)s',
+                    default='expected-results.json')
 parser.add_argument('--expectations-root',
                     help='root of expectations directory to update-- should ' +
                     'contain one or more base-* subdirectories. Defaults to ' +
                     '%(default)s',
                     default='.')
-parser.add_argument('--json-base-url',
-                    help='base URL from which to read JSON_FILENAME ' +
-                    'files; defaults to %(default)s',
-                    default='http://skia-autogen.googlecode.com/svn/gm-actual')
-parser.add_argument('--json-filename',
-                    help='filename (under JSON_BASE_URL) to read a summary ' +
-                    'of results from; defaults to %(default)s',
-                    default='actual-results.json')
 parser.add_argument('--subdirs', metavar='SUBDIR', nargs='+',
                     help='which platform subdirectories to rebaseline; ' +
                     'if unspecified, rebaseline all subdirs, same as ' +
                     '"--subdirs %s"' % ' '.join(sorted(SUBDIR_MAPPING.keys())))
+# TODO(epoger): Add test that exercises --tests argument.
+# TODO(epoger): Once we are only rebaselining JSON files, update the helpstring
+# to indicate that this is a *filter* over the test names that
+# actual-results.json tells us need to be rebaselined.
 parser.add_argument('--tests', metavar='TEST', nargs='+',
                     help='which tests to rebaseline, e.g. ' +
                     '"--tests aaclip bigmatrix"; if unspecified, then all ' +
@@ -438,25 +274,22 @@
     #
     # See https://goto.google.com/ChecksumTransitionDetail
     expectations_json_file = os.path.join(args.expectations_root, subdir,
-                                          JSON_EXPECTATIONS_FILENAME)
+                                          args.expectations_filename)
     if os.path.isfile(expectations_json_file):
-        sys.stderr.write('ERROR: JsonRebaseliner is not implemented yet.\n')
-        sys.exit(1)
         rebaseliner = JsonRebaseliner(
             expectations_root=args.expectations_root,
+            expectations_filename=args.expectations_filename,
             tests=args.tests, configs=args.configs,
-            dry_run=args.dry_run,
-            json_base_url=args.json_base_url,
-            json_filename=args.json_filename,
-            add_new=args.add_new,
-            missing_json_is_fatal=missing_json_is_fatal)
+            actuals_base_url=args.actuals_base_url,
+            actuals_filename=args.actuals_filename,
+            add_new=args.add_new)
     else:
         rebaseliner = rebaseline_imagefiles.ImageRebaseliner(
             expectations_root=args.expectations_root,
             tests=args.tests, configs=args.configs,
             dry_run=args.dry_run,
-            json_base_url=args.json_base_url,
-            json_filename=args.json_filename,
+            json_base_url=args.actuals_base_url,
+            json_filename=args.actuals_filename,
             add_new=args.add_new,
             missing_json_is_fatal=missing_json_is_fatal)
     rebaseliner.RebaselineSubdir(subdir=subdir, builder=builder)