Truly ignore failures in skimage.

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

Committed: https://code.google.com/p/skia/source/detail?r=11790

[epoger, 2013-08-07 15:03:57]

LGTM

Do we have any self-tests for this code, like we do for gm?  That would help
catch problems like this...

[scroggo, 2013-10-14 21:44:27]

On 2013/08/07 15:03:57, epoger wrote:
> LGTM
> 
> Do we have any self-tests for this code, like we do for gm?  That would help
> catch problems like this...

Sorry for the long wait, but I have finally added some tests! I also modfied
skimage_main a little more. Please take another look.

[scroggo, 2013-10-14 21:44:38]

https://codereview.chromium.org/22293006/diff/10001/tools/tests/skimage/input...
File tools/tests/skimage/input/images-with-known-hashes/ignore-failures.json
(right):

https://codereview.chromium.org/22293006/diff/10001/tools/tests/skimage/input...
tools/tests/skimage/input/images-with-known-hashes/ignore-failures.json:11: [
"bitmap-64bitMD5", 1209453360120438600 ]
Should I add a comment that I've manually modified this hash to be invalid?
(Would that be done by using a '#'? in a json file?)

https://codereview.chromium.org/22293006/diff/10001/tools/tests/skimage/input...
File tools/tests/skimage/input/images-with-known-hashes/incorrect-results.json
(right):

https://codereview.chromium.org/22293006/diff/10001/tools/tests/skimage/input...
tools/tests/skimage/input/images-with-known-hashes/incorrect-results.json:11: [
"bitmap-64bitMD5", 1209453360120438600 ]
Same question.

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

Thanks for adding the tests!

As a reward, I have new complaints/suggestions to register, since I haven't
looked at this code for 2 months.

https://codereview.chromium.org/22293006/diff/10001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/22293006/diff/10001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:198: *  @param missingArray Array to add missing
expectation to on failure.
maybe instead...

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

https://codereview.chromium.org/22293006/diff/10001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:226: missingArray->push_back().printf("decoded %s, but
could not find expectation.",
Why are failureArray and missingArray passed as parameters, but for
gKnownFailures we refer to the global directly?

https://codereview.chromium.org/22293006/diff/10001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:237: gKnownFailures.push_back().printf("%s does not match
expectation, but this is known.",
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.

https://codereview.chromium.org/22293006/diff/10001/tools/tests/skimage/input...
File tools/tests/skimage/input/images-with-known-hashes/ignore-failures.json
(right):

https://codereview.chromium.org/22293006/diff/10001/tools/tests/skimage/input...
tools/tests/skimage/input/images-with-known-hashes/ignore-failures.json:11: [
"bitmap-64bitMD5", 1209453360120438600 ]
On 2013/10/14 21:44:38, scroggo wrote:
> Should I add a comment that I've manually modified this hash to be invalid?
> (Would that be done by using a '#'? in a json file?)

JSON does not allow comments.  See
http://stackoverflow.com/questions/244777/can-i-comment-a-json-file

It's good to know we have new religious wars that can take over for vi-vs-emacs.

Maybe just make this an obviously made-up digest, like 111111 ?

https://codereview.chromium.org/22293006/diff/10001/tools/tests/skimage/input...
File tools/tests/skimage/input/images-with-known-hashes/incorrect-results.json
(right):

https://codereview.chromium.org/22293006/diff/10001/tools/tests/skimage/input...
tools/tests/skimage/input/images-with-known-hashes/incorrect-results.json:11: [
"bitmap-64bitMD5", 1209453360120438600 ]
On 2013/10/14 21:44:38, scroggo wrote:
> Same question.

Same answer.

https://codereview.chromium.org/22293006/diff/10001/tools/tests/skimage_self_...
File tools/tests/skimage_self_test.py (right):

https://codereview.chromium.org/22293006/diff/10001/tools/tests/skimage_self_...
tools/tests/skimage_self_test.py:54: print "\"%s\" should have reported
failure!" % " ".join(args)
Personally, I would write it this way to avoid the backslashes:

print "'%s' should have reported failure!" % " ".join(args)

I will note that, in the long term, we should move toward following
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html , which calls for
single-quoted strings in Python.

print '"%s" should have reported failure!' % ' '.join(args)

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

https://codereview.chromium.org/22293006/diff/10001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/22293006/diff/10001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:198: *  @param missingArray Array to add missing
expectation to on failure.
On 2013/10/15 15:31:03, epoger wrote:
> maybe instead...
> 
> @param missingArray Array to add failure message to when missing image
> expectation

Done.

https://codereview.chromium.org/22293006/diff/10001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:226: missingArray->push_back().printf("decoded %s, but
could not find expectation.",
On 2013/10/15 15:31:03, epoger wrote:
> Why are failureArray and missingArray passed as parameters, but for
> gKnownFailures we refer to the global directly?

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.

https://codereview.chromium.org/22293006/diff/10001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:237: gKnownFailures.push_back().printf("%s does not match
expectation, but this is known.",
On 2013/10/15 15:31:03, epoger wrote:
> 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.

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

https://codereview.chromium.org/22293006/diff/10001/tools/tests/skimage/input...
File tools/tests/skimage/input/images-with-known-hashes/ignore-failures.json
(right):

https://codereview.chromium.org/22293006/diff/10001/tools/tests/skimage/input...
tools/tests/skimage/input/images-with-known-hashes/ignore-failures.json:11: [
"bitmap-64bitMD5", 1209453360120438600 ]
On 2013/10/15 15:31:03, epoger wrote:
> On 2013/10/14 21:44:38, scroggo wrote:
> > Should I add a comment that I've manually modified this hash to be invalid?
> > (Would that be done by using a '#'? in a json file?)
> 
> JSON does not allow comments.  See
> http://stackoverflow.com/questions/244777/can-i-comment-a-json-file
> 
> It's good to know we have new religious wars that can take over for
vi-vs-emacs.
> 
> Maybe just make this an obviously made-up digest, like 111111 ?

Done.

https://codereview.chromium.org/22293006/diff/10001/tools/tests/skimage/input...
File tools/tests/skimage/input/images-with-known-hashes/incorrect-results.json
(right):

https://codereview.chromium.org/22293006/diff/10001/tools/tests/skimage/input...
tools/tests/skimage/input/images-with-known-hashes/incorrect-results.json:11: [
"bitmap-64bitMD5", 1209453360120438600 ]
On 2013/10/15 15:31:03, epoger wrote:
> On 2013/10/14 21:44:38, scroggo wrote:
> > Same question.
> 
> Same answer.

Done.

https://codereview.chromium.org/22293006/diff/10001/tools/tests/skimage_self_...
File tools/tests/skimage_self_test.py (right):

https://codereview.chromium.org/22293006/diff/10001/tools/tests/skimage_self_...
tools/tests/skimage_self_test.py:54: print "\"%s\" should have reported
failure!" % " ".join(args)
On 2013/10/15 15:31:03, epoger wrote:
> Personally, I would write it this way to avoid the backslashes:
> 
> print "'%s' should have reported failure!" % " ".join(args)

Done.

> 
> I will note that, in the long term, we should move toward following
> http://google-styleguide.googlecode.com/svn/trunk/pyguide.html , which calls
for
> single-quoted strings in Python.
> 
> print '"%s" should have reported failure!' % ' '.join(args)

Maybe I'm looking in the wrong place, but inside
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=String...
, it appears that the preference is to be consistent (which this file is not...)
rather than choosing one over the other.

[epoger, 2013-10-15 20:20:55]

lgtm

https://codereview.chromium.org/22293006/diff/10001/tools/tests/skimage_self_...
File tools/tests/skimage_self_test.py (right):

https://codereview.chromium.org/22293006/diff/10001/tools/tests/skimage_self_...
tools/tests/skimage_self_test.py:54: print "\"%s\" should have reported
failure!" % " ".join(args)
On 2013/10/15 19:50:59, scroggo wrote:
> On 2013/10/15 15:31:03, epoger wrote:
> > Personally, I would write it this way to avoid the backslashes:
> > 
> > print "'%s' should have reported failure!" % " ".join(args)
> 
> Done.
> 
> > 
> > I will note that, in the long term, we should move toward following
> > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html , which calls
> for
> > single-quoted strings in Python.
> > 
> > print '"%s" should have reported failure!' % ' '.join(args)
> 
> Maybe I'm looking in the wrong place, but inside
>
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=String...
> , it appears that the preference is to be consistent (which this file is
not...)
> rather than choosing one over the other.

Right.  I think consistency within the file is more important.  I just mentioned
this for future reference.

https://codereview.chromium.org/22293006/diff/23001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/22293006/diff/23001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:216: *  @param ignoreArray Array to add failure message
to when the image doe not match
doe -> does

[scroggo, 2013-10-15 20:29:10]

https://codereview.chromium.org/22293006/diff/23001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/22293006/diff/23001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:216: *  @param ignoreArray Array to add failure message
to when the image doe not match
On 2013/10/15 20:20:55, epoger wrote:
> doe -> does

Done.

[scroggo, 2013-10-15 20:29:55]

Committed patchset #5 manually as r11790 (presubmit successful).