Truly ignore failures in skimage.

tools/skimage_main.cpp

Index: tools/skimage_main.cpp
diff --git a/tools/skimage_main.cpp b/tools/skimage_main.cpp
index 6ff5978f128c5b7316a241f7df24f8ef604b11aa..ee7120e2f42991695fbedcae6a21e37d9e3f35d9 100644
--- a/tools/skimage_main.cpp
+++ b/tools/skimage_main.cpp
@@ -104,6 +104,8 @@ 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 SkBitmap::Config gPrefConfig(SkBitmap::kNo_Config);
 
@@ -188,18 +190,6 @@ 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.
@@ -243,7 +233,10 @@ static bool compare_to_expectations_if_necessary(const skiagm::GmResultDigest& d
         return true;
     }
 
-    if (failureArray != NULL) {
+    if (jsExpectation.ignoreFailure()) {
+        gKnownFailures.push_back().printf("%s does not match expectation, but this is known.",

[epoger, 2013/10/15 15:31:03]

You can handle this in a separate CL if you like (or a TODO), but it seems
strange to me that we add all this verbiage to every entry within gKnownFailures
(and in all the other types of failures also).  Isn't that verbiage implied by
the array we added the filename to?

I think this would be better:
  gKnownFailures.push_back() = filename

And then, when dumping the contents of that array, you can tell the user what it
means for files to be in that array.

EDIT: I guess the reason for the verbiage is that the same array is used to hold
cases that failed to decode, as well as ones that decoded but did not match
expectations.  In the long term, I think it would a good idea to track those in
different arrays.

[scroggo, 2013/10/15 19:50:59]

Added a FIXME (up by the declaration of the arrays).

+                                          filename);
+    } else if (failureArray != NULL) {
         failureArray->push_back().printf("decoded %s, but the result does not match "
                                          "expectations.",
                                          filename);
@@ -410,12 +403,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;
     }
 
@@ -716,6 +723,8 @@ int tool_main(int argc, char** argv) {
         print_strings("Missing subset expectations", gMissingSubsetExpectations);
     }
 
+    print_strings("Known failures", gKnownFailures);
+
     return failed ? -1 : 0;
 }