Improve tool that analyzes gm JSON summary

Improve tool that analyzes gm JSON summary
BUG=https://code.google.com/p/skia/issues/detail?id=1300
R=borenet@google.com

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

[epoger, 2013-05-17 17:56:45]

Ready for review at patchset 1.

https://codereview.chromium.org/15298010/diff/1/gm/display_json_results.py
File gm/display_json_results.py (right):

https://codereview.chromium.org/15298010/diff/1/gm/display_json_results.py#ne...
gm/display_json_results.py:1: #!/usr/bin/env python
This file was renamed from "confirm_no_failures_in_json.py" using svn mv

https://codereview.chromium.org/15298010/diff/1/gm/display_json_results.py#ne...
gm/display_json_results.py:114: if '__main__' == __name__:
Here are two sample runs:

epoger@wpgntat-ubiq141:~/src/skia/white/trunk$ python gm/display_json_results.py
gm/tests/outputs/compared-against-identical-pixels-json/output-expected/json-summary.txt
; echo $?
[ ] 0 IgnoredExpectationsMismatch: 
[*] 0 ExpectationsMismatch: 
[ ] 0 MissingExpectations
[ ] 2 Passed
(results marked with [*] will cause nonzero return value)
0

epoger@wpgntat-ubiq141:~/src/skia/white/trunk$ python gm/display_json_results.py
gm/tests/outputs/compared-against-different-pixels-json/output-expected/json-summary.txt
; echo $?
[ ] 0 IgnoredExpectationsMismatch: 
[*] 2 ExpectationsMismatch: 565/selftest1.png 8888/selftest1.png 
[ ] 0 MissingExpectations
[ ] 0 Passed
(results marked with [*] will cause nonzero return value)
1

[borenet, 2013-05-17 19:48:33]

I like it!  Just a couple of comments.

https://codereview.chromium.org/15298010/diff/1/gm/display_json_results.py
File gm/display_json_results.py (right):

https://codereview.chromium.org/15298010/diff/1/gm/display_json_results.py#ne...
gm/display_json_results.py:13: error types cause a nonzero return code.
Other options might be nice as well, for example to clean up the output, we
might want to only list the first 5 or 10 failures in each category.

https://codereview.chromium.org/15298010/diff/1/gm/display_json_results.py#ne...
gm/display_json_results.py:80: RESULTS_MAP = {
Is there any reason for this to be global?  I'm also wondering if it should be
named like a constant if we're modifying it.

https://codereview.chromium.org/15298010/diff/1/gm/display_json_results.py#ne...
gm/display_json_results.py:117: failure = Display(sys.argv[1])
You could also do:

sys.exit(Display(sys.argv[1]))

and change "failure" to be 0 or 1.  That has the nice benefit that you aren't
defining a global variable called "failure."

[epoger, 2013-05-18 03:09:54]

https://codereview.chromium.org/15298010/diff/1/gm/display_json_results.py
File gm/display_json_results.py (right):

https://codereview.chromium.org/15298010/diff/1/gm/display_json_results.py#ne...
gm/display_json_results.py:13: error types cause a nonzero return code.
On 2013/05/17 19:48:33, borenet wrote:
> Other options might be nice as well, for example to clean up the output, we
> might want to only list the first 5 or 10 failures in each category.

Good idea... added to the TODO.  If it's OK with you, I'd like to commit this
as-is for now (since it is already useful in present form) and add the other
stuff later as needed.

https://codereview.chromium.org/15298010/diff/1/gm/display_json_results.py#ne...
gm/display_json_results.py:80: RESULTS_MAP = {
On 2013/05/17 19:48:33, borenet wrote:
> Is there any reason for this to be global?  I'm also wondering if it should be
> named like a constant if we're modifying it.

No need for it to be global... I moved its definition into Display(), because
that's the only place it is used anyway.

I think it was fine for it to be a constant, anyway, because the dictionary
itself is not modified.  (The ResultAccumulators inside are modified, though.)

https://codereview.chromium.org/15298010/diff/1/gm/display_json_results.py#ne...
gm/display_json_results.py:117: failure = Display(sys.argv[1])
On 2013/05/17 19:48:33, borenet wrote:
> You could also do:
> 
> sys.exit(Display(sys.argv[1]))
> 
> and change "failure" to be 0 or 1.  That has the nice benefit that you aren't
> defining a global variable called "failure."

If it's OK with you, I would really rather keep the boolean return type for
Display(), because the "return-0-to-signal-success" custom is much more of a
shell-level concept than a Python concept.  A boolean is a cleaner
representation of "did it fail or not".

But I did rewrite this slightly, I hope it gives some of the improvement you
were looking for...

[borenet, 2013-05-20 12:09:55]

LGTM

https://codereview.chromium.org/15298010/diff/1/gm/display_json_results.py
File gm/display_json_results.py (right):

https://codereview.chromium.org/15298010/diff/1/gm/display_json_results.py#ne...
gm/display_json_results.py:13: error types cause a nonzero return code.
On 2013/05/18 03:09:54, epoger wrote:
> On 2013/05/17 19:48:33, borenet wrote:
> > Other options might be nice as well, for example to clean up the output, we
> > might want to only list the first 5 or 10 failures in each category.
> 
> Good idea... added to the TODO.  If it's OK with you, I'd like to commit this
> as-is for now (since it is already useful in present form) and add the other
> stuff later as needed.

Agreed.

https://codereview.chromium.org/15298010/diff/1/gm/display_json_results.py#ne...
gm/display_json_results.py:117: failure = Display(sys.argv[1])
On 2013/05/18 03:09:54, epoger wrote:
> On 2013/05/17 19:48:33, borenet wrote:
> > You could also do:
> > 
> > sys.exit(Display(sys.argv[1]))
> > 
> > and change "failure" to be 0 or 1.  That has the nice benefit that you
aren't
> > defining a global variable called "failure."
> 
> If it's OK with you, I would really rather keep the boolean return type for
> Display(), because the "return-0-to-signal-success" custom is much more of a
> shell-level concept than a Python concept.  A boolean is a cleaner
> representation of "did it fail or not".
> 
> But I did rewrite this slightly, I hope it gives some of the improvement you
> were looking for...

Yep!  Now the only thing that's slightly confusing to me is that I've typically
seen functions which return booleans return True on success and False on
failure.

[epoger, 2013-05-21 16:02:51]

https://codereview.chromium.org/15298010/diff/1/gm/display_json_results.py
File gm/display_json_results.py (right):

https://codereview.chromium.org/15298010/diff/1/gm/display_json_results.py#ne...
gm/display_json_results.py:117: failure = Display(sys.argv[1])
On 2013/05/20 12:09:55, borenet wrote:
> On 2013/05/18 03:09:54, epoger wrote:
> > On 2013/05/17 19:48:33, borenet wrote:
> > > You could also do:
> > > 
> > > sys.exit(Display(sys.argv[1]))
> > > 
> > > and change "failure" to be 0 or 1.  That has the nice benefit that you
> aren't
> > > defining a global variable called "failure."
> > 
> > If it's OK with you, I would really rather keep the boolean return type for
> > Display(), because the "return-0-to-signal-success" custom is much more of a
> > shell-level concept than a Python concept.  A boolean is a cleaner
> > representation of "did it fail or not".
> > 
> > But I did rewrite this slightly, I hope it gives some of the improvement you
> > were looking for...
> 
> Yep!  Now the only thing that's slightly confusing to me is that I've
typically
> seen functions which return booleans return True on success and False on
> failure.

Good point.  I inverted the meaning of the returned boolean (and updated the
documentation accordingly)

[borenet, 2013-05-21 16:04:00]

On 2013/05/21 16:02:51, epoger wrote:
> https://codereview.chromium.org/15298010/diff/1/gm/display_json_results.py
> File gm/display_json_results.py (right):
> 
>
https://codereview.chromium.org/15298010/diff/1/gm/display_json_results.py#ne...
> gm/display_json_results.py:117: failure = Display(sys.argv[1])
> On 2013/05/20 12:09:55, borenet wrote:
> > On 2013/05/18 03:09:54, epoger wrote:
> > > On 2013/05/17 19:48:33, borenet wrote:
> > > > You could also do:
> > > > 
> > > > sys.exit(Display(sys.argv[1]))
> > > > 
> > > > and change "failure" to be 0 or 1.  That has the nice benefit that you
> > aren't
> > > > defining a global variable called "failure."
> > > 
> > > If it's OK with you, I would really rather keep the boolean return type
for
> > > Display(), because the "return-0-to-signal-success" custom is much more of
a
> > > shell-level concept than a Python concept.  A boolean is a cleaner
> > > representation of "did it fail or not".
> > > 
> > > But I did rewrite this slightly, I hope it gives some of the improvement
you
> > > were looking for...
> > 
> > Yep!  Now the only thing that's slightly confusing to me is that I've
> typically
> > seen functions which return booleans return True on success and False on
> > failure.
> 
> Good point.  I inverted the meaning of the returned boolean (and updated the
> documentation accordingly)

Thanks!  LGTM

[epoger, 2013-05-21 16:06:47]

Committed patchset #4 manually as r9217 (presubmit successful).