Truly ignore failures in skimage.

If the expectation is set to ignore failures, do not return a
negative 1 (indicating a failure) when a successful decode
does not match the expectation.

If the file could not be decoded at all, report this to the
user depending on the input expectations file:

No expectations:
Report failure. The user wanted to know if the file could be
decoded.

Expectations expected a valid result:
Report failure. The decode should have matched the result.

Empty expectations:
Print a message that the expectation was missing, but still
return success from skimage, since this is a newly added file
(i.e. it has no expectation yet).

Ignore failure:
Return success from skimage, since it is a known failure.

Update the self tests to ensure these behaviors.

R=epoger@google.com

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

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

8 files changed, 197 insertions, 25 deletions

diff --git a/tools/skimage_main.cpp b/tools/skimage_main.cpp
index 6ff5978f12..5b8f095485 100644
--- a/tools/skimage_main.cpp
+++ b/tools/skimage_main.cpp
@@ -93,6 +93,21 @@ static void make_outname(SkString* dst, const char outDir[], const char src[],
 
 // Store the names of the filenames to report later which ones failed, succeeded, and were
 // invalid.
+// FIXME: Add more arrays, for more specific types of errors, and make the output simpler.
+// If each array holds one type of error, the output can change from:
+//
+// Failures:
+//      <image> failed for such and such reason
+//      <image> failed for some different reason
+//
+// to:
+//
+// Such and such failures:
+//      <image>
+//
+// Different reason failures:
+//      <image>
+//
 static SkTArray<SkString, false> gInvalidStreams;
 static SkTArray<SkString, false> gMissingCodecs;
 static SkTArray<SkString, false> gDecodeFailures;
@@ -104,6 +119,9 @@ static SkTArray<SkString, false> gFailedSubsetDecodes;
 // the bots will not turn red with each new image test.
 static SkTArray<SkString, false> gMissingExpectations;
 static SkTArray<SkString, false> gMissingSubsetExpectations;
+// For files that are expected to fail.
+static SkTArray<SkString, false> gKnownFailures;
+static SkTArray<SkString, false> gKnownSubsetFailures;
 
 static SkBitmap::Config gPrefConfig(SkBitmap::kNo_Config);
 
@@ -188,24 +206,15 @@ static void write_expectations(const SkBitmap& bitmap, const char* filename) {
 }
 
 /**
- *  Return true if this filename is a known failure, and therefore a failure
- *  to decode should be ignored.
- */
-static bool expect_to_fail(const char* filename) {
-    if (NULL == gJsonExpectations.get()) {
-        return false;
-    }
-    skiagm::Expectations jsExpectations = gJsonExpectations->get(filename);
-    return jsExpectations.ignoreFailure();
-}
-
-/**
  *  Compare against an expectation for this filename, if there is one.
  *  @param digest GmResultDigest, computed from the decoded bitmap, to compare to the
- *           expectation.
+ *          expectation.
  *  @param filename String used to find the expected value.
  *  @param failureArray Array to add a failure message to on failure.
- *  @param missingArray Array to add missing expectation to on failure.
+ *  @param missingArray Array to add failure message to when missing image
+ *          expectation.
+ *  @param ignoreArray Array to add failure message to when the image does not match
+ *          the expectation, but this is a failure we can ignore.
  *  @return bool True in any of these cases:
  *                  - the bitmap matches the expectation.
  *               False in any of these cases:
@@ -217,7 +226,8 @@ static bool expect_to_fail(const char* filename) {
 static bool compare_to_expectations_if_necessary(const skiagm::GmResultDigest& digest,
                                                  const char* filename,
                                                  SkTArray<SkString, false>* failureArray,
-                                                 SkTArray<SkString, false>* missingArray) {
+                                                 SkTArray<SkString, false>* missingArray,
+                                                 SkTArray<SkString, false>* ignoreArray) {
     if (!digest.isValid()) {
         if (failureArray != NULL) {
             failureArray->push_back().printf("decoded %s, but could not create a GmResultDigest.",
@@ -243,7 +253,10 @@ static bool compare_to_expectations_if_necessary(const skiagm::GmResultDigest& d
         return true;
     }
 
-    if (failureArray != NULL) {
+    if (jsExpectation.ignoreFailure()) {
+        ignoreArray->push_back().printf("%s does not match expectation, but this is known.",
+                                        filename);
+    } else if (failureArray != NULL) {
         failureArray->push_back().printf("decoded %s, but the result does not match "
                                          "expectations.",
                                          filename);
@@ -410,12 +423,26 @@ static void decodeFileAndWrite(const char srcPath[], const SkString* writePath)
 
     if (!codec->decode(&stream, &bitmap, gPrefConfig,
                        SkImageDecoder::kDecodePixels_Mode)) {
-        if (expect_to_fail(filename)) {
-            gSuccessfulDecodes.push_back().appendf(
-                "failed to decode %s, which is a known failure.", srcPath);
-        } else {
-            gDecodeFailures.push_back().set(srcPath);
+        if (NULL != gJsonExpectations.get()) {
+            skiagm::Expectations jsExpectations = gJsonExpectations->get(filename);
+            if (jsExpectations.ignoreFailure()) {
+                // This is a known failure.
+                gKnownFailures.push_back().appendf(
+                    "failed to decode %s, which is a known failure.", srcPath);
+                return;
+            }
+            if (jsExpectations.empty()) {
+                // This is a failure, but it is a new file. Mark it as missing, with
+                // a note that it should be marked failing.
+                gMissingExpectations.push_back().appendf(
+                    "new file %s (with no expectations) FAILED to decode.", srcPath);
+                return;
+            }
         }
+
+        // If there was a failure, and either there was no expectations file, or
+        // the expectations file listed a valid expectation, report the failure.
+        gDecodeFailures.push_back().set(srcPath);
         return;
     }
 
@@ -438,7 +465,8 @@ static void decodeFileAndWrite(const char srcPath[], const SkString* writePath)
     skiagm::GmResultDigest digest(bitmap);
     if (compare_to_expectations_if_necessary(digest, filename,
                                              &gDecodeFailures,
-                                             &gMissingExpectations)) {
+                                             &gMissingExpectations,
+                                             &gKnownFailures)) {
         gSuccessfulDecodes.push_back().printf("%s [%d %d]", srcPath, bitmap.width(),
                                               bitmap.height());
     } else if (!FLAGS_mismatchPath.isEmpty()) {
@@ -491,7 +519,8 @@ static void decodeFileAndWrite(const char srcPath[], const SkString* writePath)
                     if (compare_to_expectations_if_necessary(subsetDigest,
                                                              subsetName.c_str(),
                                                              &gFailedSubsetDecodes,
-                                                             &gMissingSubsetExpectations)) {
+                                                             &gMissingSubsetExpectations,
+                                                             &gKnownSubsetFailures)) {
                         gSuccessfulSubsetDecodes.push_back().printf("Decoded subset %s from %s",
                                                               subsetDim.c_str(), srcPath);
                     } else if (!FLAGS_mismatchPath.isEmpty()) {
@@ -714,8 +743,11 @@ int tool_main(int argc, char** argv) {
         failed |= print_strings("Failed subset decodes", gFailedSubsetDecodes);
         print_strings("Decoded subsets", gSuccessfulSubsetDecodes);
         print_strings("Missing subset expectations", gMissingSubsetExpectations);
+        print_strings("Known subset failures", gKnownSubsetFailures);
     }
 
+    print_strings("Known failures", gKnownFailures);
+
     return failed ? -1 : 0;
 }
 
diff --git a/tools/tests/skimage/input/bad-images/empty-results.json b/tools/tests/skimage/input/bad-images/empty-results.json
new file mode 100644
index 0000000000..8ae56100de
--- /dev/null
+++ b/tools/tests/skimage/input/bad-images/empty-results.json
@@ -0,0 +1,12 @@
+{
+   "actual-results" : {
+      "failed" : null,
+      "failure-ignored" : null,
+      "no-comparison" : null,
+      "succeeded" : null
+   },
+   "expected-results" : {
+      "invalid.png" : {
+      }
+   }
+}
diff --git a/tools/tests/skimage/input/bad-images/ignore-results.json b/tools/tests/skimage/input/bad-images/ignore-results.json
new file mode 100644
index 0000000000..4a0326e108
--- /dev/null
+++ b/tools/tests/skimage/input/bad-images/ignore-results.json
@@ -0,0 +1,16 @@
+{
+   "actual-results" : {
+      "failed" : null,
+      "failure-ignored" : null,
+      "no-comparison" : null,
+      "succeeded" : null
+   },
+   "expected-results" : {
+      "invalid.png" : {
+         "allowed-digests" : [
+            [ "bitmap-64bitMD5", 8011846386646810712 ]
+         ],
+         "ignore-failure" : true
+      }
+   }
+}
diff --git a/tools/tests/skimage/input/bad-images/incorrect-results.json b/tools/tests/skimage/input/bad-images/incorrect-results.json
new file mode 100644
index 0000000000..67b8bdb0c0
--- /dev/null
+++ b/tools/tests/skimage/input/bad-images/incorrect-results.json
@@ -0,0 +1,16 @@
+{
+   "actual-results" : {
+      "failed" : null,
+      "failure-ignored" : null,
+      "no-comparison" : null,
+      "succeeded" : null
+   },
+   "expected-results" : {
+      "invalid.png" : {
+         "allowed-digests" : [
+            [ "bitmap-64bitMD5", 8011846386646810712 ]
+         ],
+         "ignore-failure" : false
+      }
+   }
+}
diff --git a/tools/tests/skimage/input/bad-images/invalid.png b/tools/tests/skimage/input/bad-images/invalid.png
new file mode 100644
index 0000000000..0dd1608e45
--- /dev/null
+++ b/tools/tests/skimage/input/bad-images/invalid.png
@@ -0,0 +1,2 @@
+‰PNG

+
diff --git a/tools/tests/skimage/input/images-with-known-hashes/ignore-failures.json b/tools/tests/skimage/input/images-with-known-hashes/ignore-failures.json
new file mode 100644
index 0000000000..a99cd3e321
--- /dev/null
+++ b/tools/tests/skimage/input/images-with-known-hashes/ignore-failures.json
@@ -0,0 +1,16 @@
+{
+   "actual-results" : {
+      "failed" : null,
+      "failure-ignored" : null,
+      "no-comparison" : null,
+      "succeeded" : null
+   },
+   "expected-results" : {
+      "1209453360120438698.png" : {
+         "allowed-digests" : [
+            [ "bitmap-64bitMD5", 1111111111111111111 ]
+         ],
+         "ignore-failure" : true
+      }
+   }
+}
diff --git a/tools/tests/skimage/input/images-with-known-hashes/incorrect-results.json b/tools/tests/skimage/input/images-with-known-hashes/incorrect-results.json
new file mode 100644
index 0000000000..9cf72c3cd3
--- /dev/null
+++ b/tools/tests/skimage/input/images-with-known-hashes/incorrect-results.json
@@ -0,0 +1,16 @@
+{
+   "actual-results" : {
+      "failed" : null,
+      "failure-ignored" : null,
+      "no-comparison" : null,
+      "succeeded" : null
+   },
+   "expected-results" : {
+      "1209453360120438698.png" : {
+         "allowed-digests" : [
+            [ "bitmap-64bitMD5", 1111111111111111111 ]
+         ],
+         "ignore-failure" : false
+      }
+   }
+}
diff --git a/tools/tests/skimage_self_test.py b/tools/tests/skimage_self_test.py
index 24164d4a51..6229d6a9ff 100755
--- a/tools/tests/skimage_self_test.py
+++ b/tools/tests/skimage_self_test.py
@@ -39,6 +39,66 @@ def DieIfFilesMismatch(expected, actual):
             expected, actual)
         exit(1)
 
+def test_invalid_file(file_dir, skimage_binary):
+    """ Test the return value of skimage when an invalid file is decoded.
+        If there is no expectation file, or the file expects a particular
+        result, skimage should return nonzero indicating failure.
+        If the file has no expectation, or ignore-failure is set to true,
+        skimage should return zero indicating success. """
+    invalid_file = os.path.join(file_dir, "skimage", "input", "bad-images",
+                                "invalid.png")
+    # No expectations file:
+    args = [skimage_binary, "--readPath", invalid_file]
+    result = subprocess.call(args)
+    if 0 == result:
+      print "'%s' should have reported failure!" % " ".join(args)
+      exit(1)
+
+    # Directory holding all expectations files
+    expectations_dir = os.path.join(file_dir, "skimage", "input", "bad-images")
+
+    # Expectations file expecting a valid decode:
+    incorrect_expectations = os.path.join(expectations_dir,
+                                          "incorrect-results.json")
+    args = [skimage_binary, "--readPath", invalid_file,
+            "--readExpectationsPath", incorrect_expectations]
+    result = subprocess.call(args)
+    if 0 == result:
+      print "'%s' should have reported failure!" % " ".join(args)
+      exit(1)
+
+    # Empty expectations:
+    empty_expectations = os.path.join(expectations_dir, "empty-results.json")
+    subprocess.check_call([skimage_binary, "--readPath", invalid_file,
+                           "--readExpectationsPath", empty_expectations])
+
+    # Ignore failure:
+    ignore_expectations = os.path.join(expectations_dir, "ignore-results.json")
+    subprocess.check_call([skimage_binary, "--readPath", invalid_file,
+                           "--readExpectationsPath", ignore_expectations])
+
+def test_incorrect_expectations(file_dir, skimage_binary):
+    """ Test that comparing to incorrect expectations fails, unless
+        ignore-failures is set to true. """
+    valid_file = os.path.join(file_dir, "skimage", "input",
+                                    "images-with-known-hashes",
+                                    "1209453360120438698.png")
+    expectations_dir = os.path.join(file_dir, "skimage", "input",
+                                    "images-with-known-hashes")
+
+    incorrect_results = os.path.join(expectations_dir,
+                                     "incorrect-results.json")
+    args = [skimage_binary, "--readPath", valid_file, "--readExpectationsPath",
+            incorrect_results]
+    result = subprocess.call(args)
+    if 0 == result:
+      print "'%s' should have reported failure!" % " ".join(args)
+      exit(1)
+
+    ignore_results = os.path.join(expectations_dir, "ignore-failures.json")
+    subprocess.check_call([skimage_binary, "--readPath", valid_file,
+                           "--readExpectationsPath", ignore_results])
+
 def main():
     # Use the directory of this file as the out directory
     file_dir = os.path.abspath(os.path.dirname(__file__))
@@ -68,8 +128,8 @@ def main():
     subprocess.check_call([skimage_binary, "--readPath", images_dir,
                            "--readExpectationsPath", expectations_path])
 
-    # TODO(scroggo): Add a test that compares expectations and image files that
-    # are known to NOT match, and make sure it returns an error.
+    test_incorrect_expectations(file_dir=file_dir,
+                                skimage_binary=skimage_binary)
 
     # Generate an expectations file from an empty directory.
     empty_dir = tempfile.mkdtemp()
@@ -91,6 +151,8 @@ def main():
                                        "nonexistent-dir", "expectations.json")
     DieIfFilesMismatch(expected=golden_expectations, actual=expectations_path)
 
+    test_invalid_file(file_dir=file_dir, skimage_binary=skimage_binary)
+
     # Done with all tests.
     print "Self tests succeeded!"