Add gm/confirm_no_failures_in_json.py

Add gm/confirm_no_failures_in_json.py
The CompareGMs step will use this (instead of running skdiff over the image
files) to decide whether to go red or green.

R=rmistry@google.com

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

[epoger, 2013-05-10 19:15:00]

Ready for review...

[rmistry, 2013-05-13 12:25:39]

Looks good overall. Some minor comments:

https://codereview.chromium.org/15100003/diff/1/gm/confirm_no_failures_in_jso...
File gm/confirm_no_failures_in_json.py (right):

https://codereview.chromium.org/15100003/diff/1/gm/confirm_no_failures_in_jso...
gm/confirm_no_failures_in_json.py:26: 
Nit: Please add one more new line here.

https://codereview.chromium.org/15100003/diff/1/gm/confirm_no_failures_in_jso...
gm/confirm_no_failures_in_json.py:27: def Assert(filepath):
Should this function (and the one below) be private? Do you envision them being
used from any other modules?

https://codereview.chromium.org/15100003/diff/1/gm/confirm_no_failures_in_jso...
gm/confirm_no_failures_in_json.py:39: actual_results =
json_dict.get(JSONKEY_ACTUALRESULTS)
If the json_dict does not have an 'actual-results' key then it will return a
None here instead of failing immediately. This will cause the below line to fail
with:
AttributeError: 'NoneType' object has no attribute 'get'.

We can instead throw a better exception by changing this line to:
actual_results = json_dict[JSONKEY_ACTUALRESULTS]
then it will fail with:
KeyError: actual-results.

https://codereview.chromium.org/15100003/diff/1/gm/tests/run.sh
File gm/tests/run.sh (right):

https://codereview.chromium.org/15100003/diff/1/gm/tests/run.sh#newcode211
gm/tests/run.sh:211: # Exercise confirm_no_failures_in_json.py
Everything looks the same in the below 3 lines except the directory after
$GM_OUTPUTS. Does it make sense to have a loop instead?

[epoger, 2013-05-14 15:03:00]

https://codereview.chromium.org/15100003/diff/1/gm/confirm_no_failures_in_jso...
File gm/confirm_no_failures_in_json.py (right):

https://codereview.chromium.org/15100003/diff/1/gm/confirm_no_failures_in_jso...
gm/confirm_no_failures_in_json.py:26: 
On 2013/05/13 12:25:39, rmistry wrote:
> Nit: Please add one more new line here.

Done.

https://codereview.chromium.org/15100003/diff/1/gm/confirm_no_failures_in_jso...
gm/confirm_no_failures_in_json.py:27: def Assert(filepath):
On 2013/05/13 12:25:39, rmistry wrote:
> Should this function (and the one below) be private? Do you envision them
being
> used from any other modules?

I actually do envision them being used by other modules.  Particularly:
- the buildbot's CompareGMs step will call Assert()
- some process that wants to upload the results of failed tests may call
GetFailedTests(), and then use the resulting dictionary

Does that sound reasonable?

https://codereview.chromium.org/15100003/diff/1/gm/confirm_no_failures_in_jso...
gm/confirm_no_failures_in_json.py:39: actual_results =
json_dict.get(JSONKEY_ACTUALRESULTS)
On 2013/05/13 12:25:39, rmistry wrote:
> If the json_dict does not have an 'actual-results' key then it will return a
> None here instead of failing immediately. This will cause the below line to
fail
> with:
> AttributeError: 'NoneType' object has no attribute 'get'.
> 
> We can instead throw a better exception by changing this line to:
> actual_results = json_dict[JSONKEY_ACTUALRESULTS]
> then it will fail with:
> KeyError: actual-results.

That's much better, thanks!  It also signals an error if there is no "failed"
key within the "actual-results" dictionary, rather than acting the same as if
the "failed" key pointed to an empty sub-dictionary.

https://codereview.chromium.org/15100003/diff/1/gm/tests/run.sh
File gm/tests/run.sh (right):

https://codereview.chromium.org/15100003/diff/1/gm/tests/run.sh#newcode211
gm/tests/run.sh:211: # Exercise confirm_no_failures_in_json.py
On 2013/05/13 12:25:39, rmistry wrote:
> Everything looks the same in the below 3 lines except the directory after
> $GM_OUTPUTS. Does it make sense to have a loop instead?

Made it two loops (one for passing cases, one for failing cases).  Do you think
this is better?

[rmistry, 2013-05-14 18:36:44]

LGTM

https://codereview.chromium.org/15100003/diff/1/gm/confirm_no_failures_in_jso...
File gm/confirm_no_failures_in_json.py (right):

https://codereview.chromium.org/15100003/diff/1/gm/confirm_no_failures_in_jso...
gm/confirm_no_failures_in_json.py:27: def Assert(filepath):
On 2013/05/14 15:03:00, epoger wrote:
> On 2013/05/13 12:25:39, rmistry wrote:
> > Should this function (and the one below) be private? Do you envision them
> being
> > used from any other modules?
> 
> I actually do envision them being used by other modules.  Particularly:
> - the buildbot's CompareGMs step will call Assert()
> - some process that wants to upload the results of failed tests may call
> GetFailedTests(), and then use the resulting dictionary
> 
> Does that sound reasonable?

Yes.

https://codereview.chromium.org/15100003/diff/1/gm/confirm_no_failures_in_jso...
gm/confirm_no_failures_in_json.py:39: actual_results =
json_dict.get(JSONKEY_ACTUALRESULTS)
On 2013/05/14 15:03:00, epoger wrote:
> On 2013/05/13 12:25:39, rmistry wrote:
> > If the json_dict does not have an 'actual-results' key then it will return a
> > None here instead of failing immediately. This will cause the below line to
> fail
> > with:
> > AttributeError: 'NoneType' object has no attribute 'get'.
> > 
> > We can instead throw a better exception by changing this line to:
> > actual_results = json_dict[JSONKEY_ACTUALRESULTS]
> > then it will fail with:
> > KeyError: actual-results.
> 
> That's much better, thanks!  It also signals an error if there is no "failed"
> key within the "actual-results" dictionary, rather than acting the same as if
> the "failed" key pointed to an empty sub-dictionary.

The failed key will always be present? or are we assuming that when
GetFailedTests is called it should always exist?

[epoger, 2013-05-14 18:56:08]

https://codereview.chromium.org/15100003/diff/1/gm/confirm_no_failures_in_jso...
File gm/confirm_no_failures_in_json.py (right):

https://codereview.chromium.org/15100003/diff/1/gm/confirm_no_failures_in_jso...
gm/confirm_no_failures_in_json.py:39: actual_results =
json_dict.get(JSONKEY_ACTUALRESULTS)
On 2013/05/14 18:36:44, rmistry wrote:
> On 2013/05/14 15:03:00, epoger wrote:
> > On 2013/05/13 12:25:39, rmistry wrote:
> > > If the json_dict does not have an 'actual-results' key then it will return
a
> > > None here instead of failing immediately. This will cause the below line
to
> > fail
> > > with:
> > > AttributeError: 'NoneType' object has no attribute 'get'.
> > > 
> > > We can instead throw a better exception by changing this line to:
> > > actual_results = json_dict[JSONKEY_ACTUALRESULTS]
> > > then it will fail with:
> > > KeyError: actual-results.
> > 
> > That's much better, thanks!  It also signals an error if there is no
"failed"
> > key within the "actual-results" dictionary, rather than acting the same as
if
> > the "failed" key pointed to an empty sub-dictionary.
> 
> The failed key will always be present? or are we assuming that when
> GetFailedTests is called it should always exist?

The "failed" key should always be present... if there were no failures, it will
point to an empty subdictionary.  See for example
https://code.google.com/p/skia/source/browse/trunk/gm/tests/outputs/compared-...
:

   "actual-results" : {
      "failed" : null,
      "failure-ignored" : null,
      "no-comparison" : null,
      "succeeded" : {
         "565/selftest1.png" : {
            "bitmap-64bitMD5" : 12927999507540085554
         },
       ...

[rmistry, 2013-05-14 18:57:14]

LGTM

[epoger, 2013-05-14 18:58:16]

Committed patchset #3 manually as r9127 (presubmit successful).