rebaseline.py: add --add-new option, and only add new expectations if it is set

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.
 '''
 
 '''
 Rebaselines the given GM tests, on all bots and all configurations.
 Must be run from the gm-expected directory.  If run from a git or SVN
 checkout, the files will be added to the staging area for commit.
 '''
 
 # System-level imports
 import argparse
 import os
 import subprocess
 import sys
 import urllib2
 
 # Imports from within Skia
 #
-# Make sure that they are in the PYTHONPATH, but add them at the *end*
-# so any that are already in the PYTHONPATH will be preferred.
+# We need to add the 'gm' directory, so that we can import gm_json.py within
+# that directory.  That script allows us to parse the actual-results.json file
+# written out by the GM tool.

[Stephen White, 2013/06/11 17:29:39]

Thinking about this some more, maybe gm_json.py should be in tools instead? I
guess that just moves the problem to display_json_results.py. Maybe that should
be in tools too, actually. I think having all the python in one place and all
the C++ in another is kind of nice, but I defer to the greater team on that.

[epoger, 2013/06/11 18:10:20]

I think this is very topical in the context of recent discussions elsewhere
about directory organization, moving to git repository(ies), etc.

My goal was to maintain the JSON file schema definition (embodied by gm_json.py
and gm_expectations.cpp) in one place, not two places that could get out of
sync.

Ultimately, gm and all this other stuff is all tools (stuff that developers and
buildbots might run to exercise the Skia library).

+# Make sure that the 'gm' dir is in the PYTHONPATH, but add it at the *end*
+# so any dirs that are already in the PYTHONPATH will be preferred.
+#
+# This assumes that the 'gm' directory has been checked out as a sibling of
+# the 'tools' directory containing this script, which will be the case if
+# 'trunk' was checked out as a single unit.
 GM_DIRECTORY = os.path.realpath(
     os.path.join(os.path.dirname(os.path.dirname(__file__)), 'gm'))
 if GM_DIRECTORY not in sys.path:
     sys.path.append(GM_DIRECTORY)
 import gm_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 )
@@ skipping 35 matching lines @@
     #  subdirs: which platform subdirectories to rebaseline; if not specified,
     #           rebaseline all platform subdirectories
     #  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
     def __init__(self, json_base_url, json_filename,
-                 subdirs=None, tests=None, configs=None, dry_run=False):
+                 subdirs=None, tests=None, configs=None, dry_run=False,
+                 add_new=False):
         if configs and not tests:
             raise ValueError('configs should only be specified if tests ' +
                              'were specified also')
         self._tests = tests
         self._configs = configs
         if not subdirs:
             self._subdirs = sorted(SUBDIR_MAPPING.keys())
             self._missing_json_is_fatal = False
         else:
             self._subdirs = subdirs
             self._missing_json_is_fatal = True
         self._json_base_url = json_base_url
         self._json_filename = json_filename
         self._dry_run = dry_run
+        self._add_new = add_new
         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.
@@ skipping 39 matching lines @@
     #  ['imageblur', 'xfermodes']
     #
     # 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 list so we
     #   go on to the next platform
     #
     # 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_FAILUREIGNORED

[epoger, 2013/06/12 14:24:11]

Patchset 4 removes FAILUREIGNORED from the list of result types to rebaseline,
based on live discussion with Rob and Brian.

Although we don't actually mark any tests as FAILUREIGNORED yet, our plan is to
do that for tests that are known to fail... which means that we typically DON'T
want to check in their actual results as their expectations.

+    #            - JSONKEY_ACTUALRESULTS_NOCOMPARISON
+    #           if False, then return files listed in these sections:
+    #            - JSONKEY_ACTUALRESULTS_FAILED
+    #            - JSONKEY_ACTUALRESULTS_FAILUREIGNORED
     #
-    # TODO(epoger): add a parameter indicating whether "no-comparison"
-    # results (those for which we don't have any expectations yet)
-    # should be rebaselined.  For now, we only return failed expectations.
-    def _GetFilesToRebaseline(self, json_url):
+    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)
         try:
             json_contents = self._GetContentsOfUrl(json_url)
         except urllib2.HTTPError:
             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_dict = gm_json.LoadFromString(json_contents)
         actual_results = json_dict[gm_json.JSONKEY_ACTUALRESULTS]
+        sections = [gm_json.JSONKEY_ACTUALRESULTS_FAILED,
+                    gm_json.JSONKEY_ACTUALRESULTS_FAILUREIGNORED]
+        if add_new:
+            sections.append(gm_json.JSONKEY_ACTUALRESULTS_NOCOMPARISON)
 
         files_to_rebaseline = []
-        failed_results = actual_results[gm_json.JSONKEY_ACTUALRESULTS_FAILED]
-        if failed_results:
-            files_to_rebaseline.extend(failed_results.keys())
+        for section in sections:
+            section_results = actual_results[section]
+            if section_results:
+                files_to_rebaseline.extend(section_results.keys())
 
         print '# ... found files_to_rebaseline %s' % files_to_rebaseline
         if self._dry_run:
             print '#'
         return files_to_rebaseline
 
     # Rebaseline a single file.
     def _RebaselineOneFile(self, expectations_subdir, builder_name,
                            infilename, outfilename):
         if self._dry_run:
@@ skipping 14 matching lines @@
         # then fail if any of those expected combinations are
         # missing... but then this tool would become useless every
         # time someone tweaked the configs on the bots without
         # updating this script.
         try:
             self._DownloadFile(source_url=url, dest_filename=outfilename)
         except CommandFailedException:
             print '# Couldn\'t fetch ' + url
             return
 
-        # Add this file to version control (if it isn't already).
-        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)
+        # Add this file to version control (if appropriate).
+        if self._add_new:

[Stephen White, 2013/06/11 17:29:39]

Actually, at least for git, I think we want to do this regardless of --add-new.
This will put them in the index/staging area, ready for commit, which is
consistent with the blink rebaselining tools. And although my svn-fu seems to be
atrophying, I don't think there's any harm with adding them in the svn case as
well.

[epoger, 2013/06/11 18:10:20]

Brian and/or Rob, do you have any thoughts on this?

My understanding is that there was some frustration about having to manually
un-add expectations after running rebaseline.py, and that this might make it
better.  But maybe I misunderstood the situation...

[robertphillips, 2013/06/12 14:13:41]

I would greatly prefer if the addition of new baselines was opt-in (as is
proposed in this CL). The current behavior of the rebaseline script (always add
new baselines) makes for more typing on my part (to remove them).

+            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)
 
     # Rebaseline the given configs for a single test.
     #
     # params:
     #  expectations_subdir
     #  builder_name
     #  test: a single test to rebaseline
     def _RebaselineOneTest(self, expectations_subdir, builder_name, test):
         if self._configs:
             configs = self._configs
@@ skipping 24 matching lines @@
             builder_name = SUBDIR_MAPPING[subdir]
             if self._tests:
                 for test in self._tests:
                     self._RebaselineOneTest(expectations_subdir=subdir,
                                             builder_name=builder_name,
                                             test=test)
             else:  # get the raw list of files that need rebaselining from JSON
                 json_url = '/'.join([self._json_base_url,
                                      subdir, builder_name, subdir,
                                      self._json_filename])
-                filenames = self._GetFilesToRebaseline(json_url=json_url)
+                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_name,
                                             infilename=filename,
                                             outfilename=outfilename)
 
 # main...
 
 parser = argparse.ArgumentParser()
+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('--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.')
 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('--json-base-url',
@@ skipping 10 matching lines @@
                     '"--subdirs %s"' % ' '.join(sorted(SUBDIR_MAPPING.keys())))
 parser.add_argument('--tests', metavar='TEST', nargs='+',
                     help='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.')
 args = parser.parse_args()
 rebaseliner = Rebaseliner(tests=args.tests, configs=args.configs,
                           subdirs=args.subdirs, dry_run=args.dry_run,
                           json_base_url=args.json_base_url,
-                          json_filename=args.json_filename)
+                          json_filename=args.json_filename,
+                          add_new=args.add_new)
 rebaseliner.RebaselineAll()