From abeb9589ec9e918203249652b15e7af6c9b33e18 Mon Sep 17 00:00:00 2001 From: "commit-bot@chromium.org" Date: Mon, 19 May 2014 15:25:10 +0000 Subject: [PATCH] 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 --- tools/image_expectations.cpp | 43 +++++++++++++++--- tools/image_expectations.h | 21 ++++++++- tools/tests/render_pictures_test.py | 87 ++++++++++++++++++++++--------------- 3 files changed, 107 insertions(+), 44 deletions(-) diff --git a/tools/image_expectations.cpp b/tools/image_expectations.cpp index 24c9175..ac232e9 100644 --- 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 --- 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 --- 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.""" -- 2.7.4