Truly ignore failures in skimage.

tools/skimage_main.cpp

 /*
  * Copyright 2011 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "gm_expectations.h"
 #include "SkBitmap.h"
 #include "SkColorPriv.h"
@@ skipping 86 matching lines @@
 static SkTArray<SkString, false> gMissingCodecs;
 static SkTArray<SkString, false> gDecodeFailures;
 static SkTArray<SkString, false> gEncodeFailures;
 static SkTArray<SkString, false> gSuccessfulDecodes;
 static SkTArray<SkString, false> gSuccessfulSubsetDecodes;
 static SkTArray<SkString, false> gFailedSubsetDecodes;
 // Files/subsets that do not have expectations. Not reported as a failure of the test so
 // 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);
 
 // Expections read from a file specified by readExpectationsPath. The expectations must have been
 // previously written using createExpectationsPath.
 SkAutoTUnref<skiagm::JsonExpectationsSource> gJsonExpectations;
 
 static bool write_bitmap(const char outName[], const SkBitmap& bm) {
     if (SkImageEncoder::EncodeFile(outName, bm, SkImageEncoder::kPNG_Type, 100)) {
         return true;
@@ skipping 64 matching lines @@
 static void write_expectations(const SkBitmap& bitmap, const char* filename) {
     if (!FLAGS_createExpectationsPath.isEmpty()) {
         // Creates an Expectations object, and add it to the list to write.
         skiagm::Expectations expectation(bitmap);
         Json::Value value = expectation.asJsonValue();
         gExpectationsToWrite[filename] = value;
     }
 }
 
 /**
- *  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.
  *  @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.

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

maybe instead...

@param missingArray Array to add failure message to when missing image
expectation

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

Done.

  *  @return bool True in any of these cases:
  *                  - the bitmap matches the expectation.
  *               False in any of these cases:
  *                  - there is no expectations file.
  *                  - there is an expectations file, but no expectation for this bitmap.
  *                  - there is an expectation for this bitmap, but it did not match.
  *                  - expectation could not be computed from the bitmap.
  */
 static bool compare_to_expectations_if_necessary(const skiagm::GmResultDigest& digest,
                                                  const char* filename,
                                                  SkTArray<SkString, false>* failureArray,
                                                  SkTArray<SkString, false>* missingArray) {
     if (!digest.isValid()) {
         if (failureArray != NULL) {
             failureArray->push_back().printf("decoded %s, but could not create a GmResultDigest.",
                                              filename);
         }
         return false;
     }
 
     if (NULL == gJsonExpectations.get()) {
         return false;
     }
 
     skiagm::Expectations jsExpectation = gJsonExpectations->get(filename);
     if (jsExpectation.empty()) {
         if (missingArray != NULL) {
             missingArray->push_back().printf("decoded %s, but could not find expectation.",

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

Why are failureArray and missingArray passed as parameters, but for
gKnownFailures we refer to the global directly?

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

Good question. The first two are passed as parameters so I can use a different
array for subsets versus full decodes.

I cannot think of a good reason that gKnownFailures should not behave the same
way. Fixed in the next CL.

                                              filename);
         }
         return false;
     }
 
     if (jsExpectation.match(digest)) {
         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);
     }
     return false;
 }
 
 /**
  *  Helper function to write a bitmap subset to a file. Only called if subsets were created
  *  and a writePath was provided. Creates a subdirectory called 'subsets' and writes a PNG to
@@ skipping 146 matching lines @@
     codec->setSkipWritingZeroes(FLAGS_skip);
     codec->setSampleSize(FLAGS_sampleSize);
     stream.rewind();
 
     // Create a string representing just the filename itself, for use in json expectations.
     SkString basename = SkOSPath::SkBasename(srcPath);
     const char* filename = basename.c_str();
 
     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;
     }
 
     // Test decoding just the bounds. The bounds should always match.
     {
         stream.rewind();
         SkBitmap dim;
         if (!codec->decode(&stream, &dim, SkImageDecoder::kDecodeBounds_Mode)) {
             SkString failure = SkStringPrintf("failed to decode bounds for %s", srcPath);
             gDecodeFailures.push_back() = failure;
@@ skipping 280 matching lines @@
     failed |= print_strings("Failed to encode", gEncodeFailures);
     print_strings("Decoded", gSuccessfulDecodes);
     print_strings("Missing expectations", gMissingExpectations);
 
     if (FLAGS_testSubsetDecoding) {
         failed |= print_strings("Failed subset decodes", gFailedSubsetDecodes);
         print_strings("Decoded subsets", gSuccessfulSubsetDecodes);
         print_strings("Missing subset expectations", gMissingSubsetExpectations);
     }
 
+    print_strings("Known failures", gKnownFailures);
+
     return failed ? -1 : 0;
 }
 
 #if !defined SK_BUILD_FOR_IOS
 int main(int argc, char * const argv[]) {
     return tool_main(argc, (char**) argv);
 }
 #endif