rebaseline.py : add self-tests

tools/rebaseline.py

Index: tools/rebaseline.py
===================================================================
--- tools/rebaseline.py	(revision 9354)
+++ tools/rebaseline.py	(working copy)
@@ -17,7 +17,6 @@
 import os
 import subprocess
 import sys
-import tempfile
 
 # Mapping of gm-expectations subdir (under
 # https://skia.googlecode.com/svn/gm-expected/ )
@@ -48,6 +47,9 @@
 }
 
 
+class CommandFailedException(Exception):
+    pass
+
 class Rebaseliner(object):
 
     # params:
@@ -75,30 +77,50 @@
             os.path.exists('.git') or
             os.path.exists(os.path.join(os.pardir, '.git')))
 
-    # Execute subprocess.call(), unless dry_run is True
-    def _Call(self, cmd, stdout=None):
+    # 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 0
-        if stdout:
-            return subprocess.call(cmd, stdout=stdout)
-        else:
-            return subprocess.call(cmd)
+            return
+        if subprocess.call(cmd) != 0:
+            raise CommandFailedException('error running command: ' +

[epoger, 2013/05/31 03:54:06]

It seems to me that the common case ought to be: if a shell command fails, error
out.

If certain cases want to catch the shell command failure and move on, that's
fine... hence the custom CommandFailedException class, which makes sure we're
not catching terrible low-level Exceptions (AssertionError, SyntaxError, etc.)

+                                         ' '.join(cmd))
 
+    # Download a single file, raising a CommandFailedException if it fails.
+    def _DownloadFile(self, source_url, dest_filename):

[epoger, 2013/05/31 03:54:06]

Extracted the "curl" call into a new _DownloadFile() method, to make it easier
to use standard Python HTTP library calls instead of Unix shell commands later
on.  (See TODO below)

+        # 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 ])
+
     # Rebaseline a single file.
     def _RebaselineOneFile(self, expectations_subdir, builder_name,
                            infilename, outfilename):
         url = ('http://skia-autogen.googlecode.com/svn/gm-actual/' +
                expectations_subdir + '/' + builder_name + '/' +
                expectations_subdir + '/' + infilename)
-        cmd = [ 'curl', '--fail', '--silent', url ]
-        temp = tempfile.NamedTemporaryFile()
-        ret = self._Call(cmd, stdout=temp)
-        if ret != 0:
+
+        # Try to download this file, but if that fails, keep going...
+        #
+        # TODO(senorblanco): Why is this not treated as a serious failure?

[epoger, 2013/05/31 03:54:06]

Stephen, any thoughts on this question?

[Stephen White, 2013/06/03 14:09:30]

I think it's because not all platforms generate all configs (e.g., Android
doesn't generate pdf). We could (and should) probably tweak the list of configs
in this tool to reflect what the bots actually generate.

I don't think we should abort, though, since otherwise the tool will become
useless every time someone tweaks the configs on the bots without updating this
script.

+        # Even though we may not have *expectations* checked in for
+        # some tests, we should always have *actual* results for every
+        # test, right?
+        try:
+            self._DownloadFile(source_url=url, dest_filename=outfilename)
+        except CommandFailedException:
             print '# Couldn\'t fetch ' + url
             return
-        cmd = [ 'cp', temp.name, outfilename ]
-        self._Call(cmd);
+
+        # Add this file to version control (if it isn't already).
         if self._is_svn_checkout:
             cmd = [ 'svn', 'add', '--quiet', outfilename ]
             self._Call(cmd)