gm: display summary of all results by ErrorType

gm: display summary of all results by ErrorType
as discussed in https://codereview.chromium.org/12825005/#msg14
Committed: https://code.google.com/p/skia/source/detail?r=8514

[epoger, 2013-03-26 18:42:29]

https://codereview.chromium.org/12851022/diff/1/gm/tests/outputs/compared-aga...
File
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout
(right):

https://codereview.chromium.org/12851022/diff/1/gm/tests/outputs/compared-aga...
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout:3:
GM: Ran 1 GMs, each with 2 configs ["8888", "565"] and 7 modes ["replay",
"serialize", "rtree", "tileGrid1.000000", "pipe", "pipe cross-process", "pipe
cross-process, shared address"], so there should be a total of 9 tests
This stdout diff shows the main thrust of the change.

https://codereview.chromium.org/12851022/diff/1/gm/tests/outputs/no-readpath/...
File gm/tests/outputs/no-readpath/output-expected/stdout (right):

https://codereview.chromium.org/12851022/diff/1/gm/tests/outputs/no-readpath/...
gm/tests/outputs/no-readpath/output-expected/stdout:3: GM: Ran 7 tests:
NoGpuContext=0 ImageMismatch=0 MissingExpectations=0 WritingReferenceImage=0
Now it says ran 7 tests instead of 9, but does NOT indicate that expectations
were missing... that seems wrong.

[epoger, 2013-03-26 19:06:05]

https://codereview.chromium.org/12851022/diff/6001/gm/tests/outputs/no-readpa...
File gm/tests/outputs/no-readpath/output-expected/stdout (right):

https://codereview.chromium.org/12851022/diff/6001/gm/tests/outputs/no-readpa...
gm/tests/outputs/no-readpath/output-expected/stdout:6: GM: [ ] 2
MissingExpectations: 8888/selftest1 565/selftest1
patchset 2 has better reporting for running without --readPath: all tests will
be reported as MissingExpectations

[epoger, 2013-03-26 19:33:24]

Ready for review at patchset 3.

[borenet, 2013-03-29 05:55:14]

This seems like the right way to do it, but I have some concerns, primarily with
the number of added TODOs.

https://codereview.chromium.org/12851022/diff/3004/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12851022/diff/3004/gm/gmmain.cpp#newcode817
gm/gmmain.cpp:817: // mismatches in here with a different ErrorType.
The number of TODOs in this CL is worrisome to me.  Are these things you plan on
addressing very soon?

https://codereview.chromium.org/12851022/diff/3004/gm/tests/outputs/compared-...
File
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout
(right):

https://codereview.chromium.org/12851022/diff/3004/gm/tests/outputs/compared-...
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout:3:
GM: Ran 1 GMs, each with 2 configs ["8888", "565"] and 7 modes ["replay",
"serialize", "rtree", "tileGrid1.000000", "pipe", "pipe cross-process", "pipe
cross-process, shared address"], so there should be a total of 9 tests
This is very wordy...

[epoger, 2013-04-02 16:00:14]

https://codereview.chromium.org/12851022/diff/3004/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12851022/diff/3004/gm/gmmain.cpp#newcode546
gm/gmmain.cpp:546: // TODO(epoger): Don't call RecordTestResults() here...
This TODO indicates a relatively minor problem that already existed before this
CL, and a proposed solution that (IMO) should be handled in its own CL.

https://codereview.chromium.org/12851022/diff/3004/gm/gmmain.cpp#newcode817
gm/gmmain.cpp:817: // mismatches in here with a different ErrorType.
On 2013/03/29 05:55:14, borenet wrote:
> The number of TODOs in this CL is worrisome to me.  Are these things you plan
on
> addressing very soon?

I have added a review comment at each TODO...

This TODO indicates a problem that already existed before this CL, and a
proposed solution that I intend to implement within the next week (I think it
should be in a separate CL).

https://codereview.chromium.org/12851022/diff/3004/gm/gmmain.cpp#newcode934
gm/gmmain.cpp:934: // something like kImageGeneration_ErrorType?
This TODO indicates a relatively minor problem that already existed before this
CL, and a proposed solution that (IMO) should be handled in its own CL.

https://codereview.chromium.org/12851022/diff/3004/gm/gmmain.cpp#newcode1321
gm/gmmain.cpp:1321: // TODO(epoger): We should start recording any per-GM
skipped
This TODO indicates a problem that already existed before this
CL, and a proposed solution that (IMO) should be handled in its own CL.

https://codereview.chromium.org/12851022/diff/3004/gm/gmmain.cpp#newcode1698
gm/gmmain.cpp:1698: // TODO(epoger): Instead of assembling this list of modes
here,
This TODO indicates something suboptimal about code added in *this* CL.  Do you
think the suggested remedy is good?  If so, I'll make the change within this
CL...

https://codereview.chromium.org/12851022/diff/3004/gm/gmmain.cpp#newcode1738
gm/gmmain.cpp:1738: // TODO(epoger): Ultimately, we should signal an error if
the
This TODO indicates a problem that already existed before this
CL, and a proposed solution that (IMO) should be handled in its own CL.

https://codereview.chromium.org/12851022/diff/3004/gm/tests/outputs/compared-...
File
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout
(right):

https://codereview.chromium.org/12851022/diff/3004/gm/tests/outputs/compared-...
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout:3:
GM: Ran 1 GMs, each with 2 configs ["8888", "565"] and 7 modes ["replay",
"serialize", "rtree", "tileGrid1.000000", "pipe", "pipe cross-process", "pipe
cross-process, shared address"], so there should be a total of 9 tests
On 2013/03/29 05:55:14, borenet wrote:
> This is very wordy...

Agreed.  What do you think it should look like?  Should we rearrange the
information, or leave some of it out?

If you tell me what you think this output should look like, I'll make it happen
in this CL.

[borenet, 2013-04-02 16:17:56]

Thanks for justifying the TODOs.

https://codereview.chromium.org/12851022/diff/3004/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12851022/diff/3004/gm/gmmain.cpp#newcode1698
gm/gmmain.cpp:1698: // TODO(epoger): Instead of assembling this list of modes
here,
On 2013/04/02 16:00:14, epoger wrote:
> This TODO indicates something suboptimal about code added in *this* CL.  Do
you
> think the suggested remedy is good?  If so, I'll make the change within this
> CL...

Sounds good to me.

https://codereview.chromium.org/12851022/diff/3004/gm/tests/outputs/compared-...
File
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout
(right):

https://codereview.chromium.org/12851022/diff/3004/gm/tests/outputs/compared-...
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout:3:
GM: Ran 1 GMs, each with 2 configs ["8888", "565"] and 7 modes ["replay",
"serialize", "rtree", "tileGrid1.000000", "pipe", "pipe cross-process", "pipe
cross-process, shared address"], so there should be a total of 9 tests
On 2013/04/02 16:00:14, epoger wrote:
> On 2013/03/29 05:55:14, borenet wrote:
> > This is very wordy...
> 
> Agreed.  What do you think it should look like?  Should we rearrange the
> information, or leave some of it out?
> 
> If you tell me what you think this output should look like, I'll make it
happen
> in this CL.

I don't know how helpful the above information is.  I guess it's primarily for
the self-test?  It might help just to add some line breaks:

GM: Ran 1 GMs
GM: ... over 2 configs ["8888", "565"]
GM: ... and 7 modes [...]
GM: ... for a total of 9 tests.

What is the value of the following line ("Ran 9 tests...")?  It seems that the
"9 tests" is given above, and the falures for each type is given below.

[epoger, 2013-04-03 03:56:50]

Patchset 5 addresses SOME of your comments; more changes coming soon.

https://codereview.chromium.org/12851022/diff/3004/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12851022/diff/3004/gm/gmmain.cpp#newcode1698
gm/gmmain.cpp:1698: // TODO(epoger): Instead of assembling this list of modes
here,
On 2013/04/02 16:17:56, borenet wrote:
> On 2013/04/02 16:00:14, epoger wrote:
> > This TODO indicates something suboptimal about code added in *this* CL.  Do
> you
> > think the suggested remedy is good?  If so, I'll make the change within this
> > CL...
> 
> Sounds good to me.

Done in patchset 5.  Do you think this change is positive, on the whole?  I'm
torn.  (The new way is automated, but incurs some runtime performance cost I
have not measured.)

https://codereview.chromium.org/12851022/diff/21001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12851022/diff/21001/gm/gmmain.cpp#newcode249
gm/gmmain.cpp:249: // TODO(epoger): It seems a bit silly that all of these modes
were
Another TODO, because I know you like them!

I can remove this TODO if you think it's not worth even noting this issue...
unfortunately, I worry that making the change alluded to in this TODO could
completely derail this CL.

[epoger, 2013-04-03 04:18:23]

Ready for review at patchset 6.

https://codereview.chromium.org/12851022/diff/3004/gm/tests/outputs/compared-...
File
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout
(right):

https://codereview.chromium.org/12851022/diff/3004/gm/tests/outputs/compared-...
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout:3:
GM: Ran 1 GMs, each with 2 configs ["8888", "565"] and 7 modes ["replay",
"serialize", "rtree", "tileGrid1.000000", "pipe", "pipe cross-process", "pipe
cross-process, shared address"], so there should be a total of 9 tests
On 2013/04/02 16:17:56, borenet wrote:
> I don't know how helpful the above information is.  I guess it's primarily for
> the self-test?

Actually, no... I wanted to make it easier to notice problems such as
https://code.google.com/p/skia/issues/detail?id=1180 ('image-surface gm test is
failing in "deferred" mode, and gm is not reporting the failure'): types of
tests that the caller ASSUMES are being run by gm, but may not be!

(Looking at this output, you can now tell that gm is not running in deferred
mode, which is a bug as noted above!)

In patchset 6, I split up this single summary line as you suggested. 
Personally, I prefer it all in one line (so it's easier to filter it with grep),
but I'm fine either way.

> What is the value of the following line ("Ran 9 tests...")?  It seems that the
> "9 tests" is given above, and the falures for each type is given below.

To help catch another type of bug: a particular test which runs for MOST gms but
not ALL.  See "Ultimately, we should signal an error" TODO in gmmain.cpp.

[borenet, 2013-04-03 12:46:47]

LGTM with a couple of suggestions.

https://codereview.chromium.org/12851022/diff/21001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12851022/diff/21001/gm/gmmain.cpp#newcode249
gm/gmmain.cpp:249: // TODO(epoger): It seems a bit silly that all of these modes
were
On 2013/04/03 03:56:50, epoger wrote:
> Another TODO, because I know you like them!
> 
> I can remove this TODO if you think it's not worth even noting this issue...
> unfortunately, I worry that making the change alluded to in this TODO could
> completely derail this CL.

I think this is fine.

https://codereview.chromium.org/12851022/diff/32001/gm/gm_error.h
File gm/gm_error.h (right):

https://codereview.chromium.org/12851022/diff/32001/gm/gm_error.h#newcode38
gm/gm_error.h:38: const char *getErrorTypeName(ErrorType type) {
I think this could be done with a macro, but I think this is more readable.

https://codereview.chromium.org/12851022/diff/32001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12851022/diff/32001/gm/gmmain.cpp#newcode68
gm/gmmain.cpp:68: const static int kAnyIntValue = 1; // when it doesn't matter
what int value, just SOMETHING
What if we counted the number of times we encounter each render mode?  I'm not
sure we'll ever care what those numbers will be, but it seems more interesting
than a dummy int.

https://codereview.chromium.org/12851022/diff/32001/gm/gmmain.cpp#newcode181
gm/gmmain.cpp:181: GMMain() : fRenderModesEncountered(1) {
Do you mind moving fUseFileHierarchy, fMismatchPath, and fTestsRuns into this
initialization list?

https://codereview.chromium.org/12851022/diff/32001/gm/tests/outputs/compared...
File
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout
(right):

https://codereview.chromium.org/12851022/diff/32001/gm/tests/outputs/compared...
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout:5:
GM: ...  and  7 modes ["pipe", "pipe cross-process", "pipe cross-process, shared
address", "replay", "rtree", "serialize", "tilegrid"]
Is this extra space here to line things up?  In that case, please bump the list
over by two spaces so that everything lines up.

[epoger, 2013-04-03 14:28:24]

Addressed all comments in patchset 7.  PTAL.

https://codereview.chromium.org/12851022/diff/32001/gm/gm_error.h
File gm/gm_error.h (right):

https://codereview.chromium.org/12851022/diff/32001/gm/gm_error.h#newcode38
gm/gm_error.h:38: const char *getErrorTypeName(ErrorType type) {
On 2013/04/03 12:46:48, borenet wrote:
> I think this could be done with a macro, but I think this is more readable.

If there is some way to automatically return the name of the variable as a
string, I think that would be better... but I couldn't figure out how to do it. 
This way, at least the compiler will warn us if a new value gets added to the
enum and we forget to add it to the switch statement.

https://codereview.chromium.org/12851022/diff/32001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12851022/diff/32001/gm/gmmain.cpp#newcode68
gm/gmmain.cpp:68: const static int kAnyIntValue = 1; // when it doesn't matter
what int value, just SOMETHING
On 2013/04/03 12:46:48, borenet wrote:
> What if we counted the number of times we encounter each render mode?  I'm not
> sure we'll ever care what those numbers will be, but it seems more interesting
> than a dummy int.

Done.  Note that this now takes twice as long, since it has to look up
renderModeDescriptor in the dictionary *twice*.  (It would perhaps be good for
SkTDict to provide a method that returns a pointer to the name/value pair, so
you could modify the value in place... I guess that could lead to some
"interesting" error cases, though.)

https://codereview.chromium.org/12851022/diff/32001/gm/gmmain.cpp#newcode181
gm/gmmain.cpp:181: GMMain() : fRenderModesEncountered(1) {
On 2013/04/03 12:46:48, borenet wrote:
> Do you mind moving fUseFileHierarchy, fMismatchPath, and fTestsRuns into this
> initialization list?

Done.

https://codereview.chromium.org/12851022/diff/32001/gm/tests/outputs/compared...
File
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout
(right):

https://codereview.chromium.org/12851022/diff/32001/gm/tests/outputs/compared...
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout:5:
GM: ...  and  7 modes ["pipe", "pipe cross-process", "pipe cross-process, shared
address", "replay", "rtree", "serialize", "tilegrid"]
On 2013/04/03 12:46:48, borenet wrote:
> Is this extra space here to line things up?  In that case, please bump the
list
> over by two spaces so that everything lines up.

Done.

[borenet, 2013-04-03 15:11:50]

On 2013/04/03 14:28:24, epoger wrote:
> Addressed all comments in patchset 7.  PTAL.
> 
> https://codereview.chromium.org/12851022/diff/32001/gm/gm_error.h
> File gm/gm_error.h (right):
> 
> https://codereview.chromium.org/12851022/diff/32001/gm/gm_error.h#newcode38
> gm/gm_error.h:38: const char *getErrorTypeName(ErrorType type) {
> On 2013/04/03 12:46:48, borenet wrote:
> > I think this could be done with a macro, but I think this is more readable.
> 
> If there is some way to automatically return the name of the variable as a
> string, I think that would be better... but I couldn't figure out how to do
it. 
> This way, at least the compiler will warn us if a new value gets added to the
> enum and we forget to add it to the switch statement.
> 
> https://codereview.chromium.org/12851022/diff/32001/gm/gmmain.cpp
> File gm/gmmain.cpp (right):
> 
> https://codereview.chromium.org/12851022/diff/32001/gm/gmmain.cpp#newcode68
> gm/gmmain.cpp:68: const static int kAnyIntValue = 1; // when it doesn't matter
> what int value, just SOMETHING
> On 2013/04/03 12:46:48, borenet wrote:
> > What if we counted the number of times we encounter each render mode?  I'm
not
> > sure we'll ever care what those numbers will be, but it seems more
interesting
> > than a dummy int.
> 
> Done.  Note that this now takes twice as long, since it has to look up
> renderModeDescriptor in the dictionary *twice*.  (It would perhaps be good for
> SkTDict to provide a method that returns a pointer to the name/value pair, so
> you could modify the value in place... I guess that could lead to some
> "interesting" error cases, though.)
> 
> https://codereview.chromium.org/12851022/diff/32001/gm/gmmain.cpp#newcode181
> gm/gmmain.cpp:181: GMMain() : fRenderModesEncountered(1) {
> On 2013/04/03 12:46:48, borenet wrote:
> > Do you mind moving fUseFileHierarchy, fMismatchPath, and fTestsRuns into
this
> > initialization list?
> 
> Done.
> 
>
https://codereview.chromium.org/12851022/diff/32001/gm/tests/outputs/compared...
> File
>
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout
> (right):
> 
>
https://codereview.chromium.org/12851022/diff/32001/gm/tests/outputs/compared...
>
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout:5:
> GM: ...  and  7 modes ["pipe", "pipe cross-process", "pipe cross-process,
shared
> address", "replay", "rtree", "serialize", "tilegrid"]
> On 2013/04/03 12:46:48, borenet wrote:
> > Is this extra space here to line things up?  In that case, please bump the
> list
> > over by two spaces so that everything lines up.
> 
> Done.

LGTM!

[epoger, 2013-04-03 17:03:03]

Committed patchset #8 manually as r8514 (presubmit successful).