make render_pictures properly handle empty expectations file

note that this changes the parameters taken by ImageResultsAndExpectations::Parse()

BUG=skia:1942
R=scroggo@google.com

Author: epoger@google.com

Review URL: https://codereview.chromium.org/289063010

git-svn-id: http://skia.googlecode.com/svn/trunk@14779 2bbb7eff-a529-9590-31e7-b0007b416f81

diff --git a/tools/image_expectations.cpp b/tools/image_expectations.cpp
index 24c9175..ac232e9 100644 (file)
--- a/tools/image_expectations.cpp
+++ b/tools/image_expectations.cpp
@@ -95,12 +95,43 @@ namespace sk_tools {
     // ImageResultsAndExpectations class...
 
     bool ImageResultsAndExpectations::readExpectationsFile(const char *jsonPath) {
-        if (Parse(jsonPath, &fExpectedJsonRoot)) {
-            fExpectedResults = fExpectedJsonRoot[kJsonKey_ExpectedResults];
+        if (NULL == jsonPath) {
+            SkDebugf("JSON expectations filename not specified\n");
+            return false;
+        }
+        SkFILE* filePtr = sk_fopen(jsonPath, kRead_SkFILE_Flag);
+        if (NULL == filePtr) {
+            SkDebugf("JSON expectations file '%s' does not exist\n", jsonPath);
+            return false;
+        }
+        size_t size = sk_fgetsize(filePtr);
+        if (0 == size) {
+            SkDebugf("JSON expectations file '%s' is empty, so no expectations\n", jsonPath);
+            sk_fclose(filePtr);
+            fExpectedResults.clear();
             return true;
-        } else {
+        }
+        bool parsedJson = Parse(filePtr, &fExpectedJsonRoot);
+        sk_fclose(filePtr);
+        if (!parsedJson) {
+            SkDebugf("Failed to parse JSON expectations file '%s'\n", jsonPath);
             return false;
         }
+        Json::Value header = fExpectedJsonRoot[kJsonKey_Header];
+        Json::Value headerType = header[kJsonKey_Header_Type];
+        Json::Value headerRevision = header[kJsonKey_Header_Revision];
+        if (strcmp(headerType.asCString(), kJsonValue_Header_Type)) {
+            SkDebugf("JSON expectations file '%s': expected headerType '%s', found '%s'\n",
+                     jsonPath, kJsonValue_Header_Type, headerType.asCString());
+            return false;
+        }
+        if (headerRevision.asInt() != kJsonValue_Header_Revision) {
+            SkDebugf("JSON expectations file '%s': expected headerRevision %d, found %d\n",
+                     jsonPath, kJsonValue_Header_Revision, headerRevision.asInt());
+            return false;
+        }
+        fExpectedResults = fExpectedJsonRoot[kJsonKey_ExpectedResults];
+        return true;
     }
 
     void ImageResultsAndExpectations::add(const char *sourceName, const char *fileName,
@@ -181,11 +212,10 @@ namespace sk_tools {
         stream.write(jsonStdString.c_str(), jsonStdString.length());
     }
 
-    /*static*/ bool ImageResultsAndExpectations::Parse(const char *jsonPath,
+    /*static*/ bool ImageResultsAndExpectations::Parse(SkFILE *filePtr,
                                                        Json::Value *jsonRoot) {
-        SkAutoDataUnref dataRef(SkData::NewFromFileName(jsonPath));
+        SkAutoDataUnref dataRef(SkData::NewFromFILE(filePtr));
         if (NULL == dataRef.get()) {
-            SkDebugf("error reading JSON file %s\n", jsonPath);
             return false;
         }
 
@@ -193,7 +223,6 @@ namespace sk_tools {
         size_t size = dataRef.get()->size();
         Json::Reader reader;
         if (!reader.parse(bytes, bytes+size, *jsonRoot)) {
-            SkDebugf("error parsing JSON file %s\n", jsonPath);
             return false;
         }
 

diff --git a/tools/image_expectations.h b/tools/image_expectations.h
index b7b135d..a24334e 100644 (file)
--- a/tools/image_expectations.h
+++ b/tools/image_expectations.h
@@ -10,6 +10,7 @@
 
 #include "SkBitmap.h"
 #include "SkJSONCPP.h"
+#include "SkOSFile.h"
 #include "SkRefCnt.h"
 
 namespace sk_tools {
@@ -81,6 +82,20 @@ namespace sk_tools {
     public:
         /**
          * Adds expectations from a JSON file, returning true if successful.
+         *
+         * If the file exists but is empty, it succeeds, and there will be no expectations.
+         * If the file does not exist, this will fail.
+         *
+         * Reasoning:
+         * Generating expectations the first time can be a tricky chicken-and-egg
+         * proposition.  "I need actual results to turn into expectations... but the only
+         * way to get actual results is to run the tool, and the tool won't run without
+         * expectations!"
+         * We could make the tool run even if there is no expectations file at all, but it's
+         * better for the tool to fail if the expectations file is not found--that will tell us
+         * quickly if files are not being copied around as they should be.
+         * Creating an empty file is an easy way to break the chicken-and-egg cycle and generate
+         * the first real expectations.
          */
         bool readExpectationsFile(const char *jsonPath);
 
@@ -116,11 +131,13 @@ namespace sk_tools {
     private:
 
         /**
-         * Read the file contents from jsonPath and parse them into jsonRoot.
+         * Read the file contents from filePtr and parse them into jsonRoot.
+         *
+         * It is up to the caller to close filePtr after this is done.
          *
          * Returns true if successful.
          */
-        static bool Parse(const char *jsonPath, Json::Value *jsonRoot);
+        static bool Parse(SkFILE* filePtr, Json::Value *jsonRoot);
 
         Json::Value fActualResults;
         Json::Value fExpectedJsonRoot;

diff --git a/tools/tests/render_pictures_test.py b/tools/tests/render_pictures_test.py
index 5ab9d67..59722e8 100755 (executable)
--- a/tools/tests/render_pictures_test.py
+++ b/tools/tests/render_pictures_test.py
@@ -242,32 +242,73 @@ class RenderPicturesTest(base_unittest.TestCase):
     }
     self._assert_json_contents(output_json_path, expected_summary_dict)
 
-  def test_untiled(self):
-    """Run without tiles."""
+  def _test_untiled(self, expectations_path=None, expected_summary_dict=None,
+                    additional_args=None):
+    """Base for multiple tests without tiles.
+
+    Args:
+      expectations_path: path we should pass using --readJsonSummaryPath, or
+          None if we should create the default expectations file
+      expected_summary_dict: dict we should compare against the output actual
+          results summary, or None if we should use a default comparison dict
+      additional_args: array of command-line args to add when we run
+          render_pictures
+    """
     output_json_path = os.path.join(self._output_dir, 'actuals.json')
     write_path_dir = self.create_empty_dir(
         path=os.path.join(self._output_dir, 'writePath'))
     self._generate_skps()
-    expectations_path = self._create_expectations()
-    self._run_render_pictures([
+    if expectations_path == None:
+      expectations_path = self._create_expectations()
+    args = [
         '-r', self._input_skp_dir,
         '--readJsonSummaryPath', expectations_path,
         '--writePath', write_path_dir,
-        '--writeJsonSummaryPath', output_json_path])
+        '--writeJsonSummaryPath', output_json_path,
+    ]
+    if additional_args:
+      args.extend(additional_args)
+    self._run_render_pictures(args)
+    if expected_summary_dict == None:
+      expected_summary_dict = {
+          "header" : EXPECTED_HEADER_CONTENTS,
+          "actual-results" : {
+              "red.skp": {
+                  "whole-image": RED_WHOLEIMAGE,
+              },
+              "green.skp": {
+                  "whole-image": GREEN_WHOLEIMAGE,
+              }
+          }
+      }
+    self._assert_json_contents(output_json_path, expected_summary_dict)
+    self._assert_directory_contents(
+        write_path_dir, ['red_skp.png', 'green_skp.png'])
+
+  def test_untiled(self):
+    """Basic test without tiles."""
+    self._test_untiled()
+
+  def test_untiled_empty_expectations_file(self):
+    """Same as test_untiled, but with an empty expectations file."""
+    expectations_path = os.path.join(self._expectations_dir, 'empty')
+    with open(expectations_path, 'w') as fh:
+      pass
     expected_summary_dict = {
         "header" : EXPECTED_HEADER_CONTENTS,
         "actual-results" : {
             "red.skp": {
-                "whole-image": RED_WHOLEIMAGE,
+                "whole-image": modified_dict(
+                    RED_WHOLEIMAGE, {"comparisonResult" : "no-comparison"}),
             },
             "green.skp": {
-                "whole-image": GREEN_WHOLEIMAGE,
+                "whole-image": modified_dict(
+                    GREEN_WHOLEIMAGE, {"comparisonResult" : "no-comparison"}),
             }
         }
     }
-    self._assert_json_contents(output_json_path, expected_summary_dict)
-    self._assert_directory_contents(
-        write_path_dir, ['red_skp.png', 'green_skp.png'])
+    self._test_untiled(expectations_path=expectations_path,
+                       expected_summary_dict=expected_summary_dict)
 
   def test_untiled_writeChecksumBasedFilenames(self):
     """Same as test_untiled, but with --writeChecksumBasedFilenames."""
@@ -313,31 +354,7 @@ class RenderPicturesTest(base_unittest.TestCase):
 
   def test_untiled_validate(self):
     """Same as test_untiled, but with --validate."""
-    output_json_path = os.path.join(self._output_dir, 'actuals.json')
-    write_path_dir = self.create_empty_dir(
-        path=os.path.join(self._output_dir, 'writePath'))
-    self._generate_skps()
-    expectations_path = self._create_expectations()
-    self._run_render_pictures([
-        '-r', self._input_skp_dir,
-        '--readJsonSummaryPath', expectations_path,
-        '--validate',
-        '--writePath', write_path_dir,
-        '--writeJsonSummaryPath', output_json_path])
-    expected_summary_dict = {
-        "header" : EXPECTED_HEADER_CONTENTS,
-        "actual-results" : {
-            "red.skp": {
-                "whole-image": RED_WHOLEIMAGE,
-            },
-            "green.skp": {
-                "whole-image": GREEN_WHOLEIMAGE,
-            }
-        }
-    }
-    self._assert_json_contents(output_json_path, expected_summary_dict)
-    self._assert_directory_contents(
-        write_path_dir, ['red_skp.png', 'green_skp.png'])
+    self._test_untiled(additional_args=['--validate'])
 
   def test_untiled_without_writePath(self):
     """Same as test_untiled, but without --writePath."""