gm self-test: make sure we report failures in individual rendering modes

gm self-test: make sure we report failures in individual rendering modes
(test: if only pipe playback fails, is it reported properly?)

Closed in favor of https://codereview.chromium.org/12807006 ('gm: report drawing mode discrepancies (e.g. pipe vs tiled) explicitly')

[epoger, 2013-03-14 17:22:19]

Eric: please see important comment in json-summary.txt

https://codereview.chromium.org/12825005/diff/1/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12825005/diff/1/gm/gmmain.cpp#newcode266
gm/gmmain.cpp:266: completeName.append(renderModeDescriptor);
fixing a minor discrepancy revealed by this test

https://codereview.chromium.org/12825005/diff/1/gm/tests/outputs/pipe-playbac...
File gm/tests/outputs/pipe-playback-failure/output-expected/command_line
(right):

https://codereview.chromium.org/12825005/diff/1/gm/tests/outputs/pipe-playbac...
gm/tests/outputs/pipe-playback-failure/output-expected/command_line:1:
out/Debug/gm --simulatePipePlaybackFailure --hierarchy --match selftest1
--config 8888 --config 565 -r gm/tests/inputs/json/identical-pixels.json
--writeJsonSummary
gm/tests/outputs/pipe-playback-failure/output-actual/json-summary.txt
Diffs here are against files in 
outputs/compared-against-identical-bytes-json/output-expected , 
which I "svn cp"ed to the new 
outputs/pipe-playback-failure/output-expected 
directory.

https://codereview.chromium.org/12825005/diff/1/gm/tests/outputs/pipe-playbac...
File gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt
(right):

https://codereview.chromium.org/12825005/diff/1/gm/tests/outputs/pipe-playbac...
gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt:6:
"succeeded" : {
Hmm, note that the error in pipe-playback mode is NOT reported in the JSON
summary!  I think this means that our current plan in
https://goto.google.com/ChecksumRemainingSteps won't quite work.

Specifically, our idea that we will run gm in a --noFailOnMismatch mode, and
then examine the JSON summary in a later step to see if there were any failures.

How should we remedy that?  Maybe we should include all rendering modes in the
JSON summary?  Although that would make the file a lot bigger...

[epoger, 2013-03-14 18:16:21]

https://codereview.chromium.org/12825005/diff/1/gm/tests/outputs/pipe-playbac...
File gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt
(right):

https://codereview.chromium.org/12825005/diff/1/gm/tests/outputs/pipe-playbac...
gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt:6:
"succeeded" : {
On 2013/03/14 17:22:19, epoger wrote:
> Hmm, note that the error in pipe-playback mode is NOT reported in the JSON
> summary!  I think this means that our current plan in
> https://goto.google.com/ChecksumRemainingSteps won't quite work.
> 
> Specifically, our idea that we will run gm in a --noFailOnMismatch mode, and
> then examine the JSON summary in a later step to see if there were any
failures.
> 
> How should we remedy that?  Maybe we should include all rendering modes in the
> JSON summary?  Although that would make the file a lot bigger...

Here's a proposal:

- If rendering the same gm test in two different modes yields different bitmaps,
that will ALWAYS be treated as an error in gm.  (As if gm had thrown a
segfault... it should NEVER happen, and if it does, the JSON summary should say
in effect "we suffered massive failure while trying to run these tests, so we do
not have any results to report")

- Thus, appearance of a test in the "actual-results" portion of the JSON summary
implies that *all* rendering modes generated *exactly* the same results.

- Thus, we don't need to list the actual checksum of every rendering mode... we
list a single actual checksum, and readers know that it was the checksum of
every mode of that test.

[borenet, 2013-03-14 18:48:20]

https://codereview.chromium.org/12825005/diff/1/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12825005/diff/1/gm/gmmain.cpp#newcode197
gm/gmmain.cpp:197: fSimulatePipePlaybackFailure = false;
I'm not sure I like the technique of baking test code directly into the
application itself...

https://codereview.chromium.org/12825005/diff/1/gm/tests/outputs/pipe-playbac...
File gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt
(right):

https://codereview.chromium.org/12825005/diff/1/gm/tests/outputs/pipe-playbac...
gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt:6:
"succeeded" : {
On 2013/03/14 18:16:21, epoger wrote:
> On 2013/03/14 17:22:19, epoger wrote:
> > Hmm, note that the error in pipe-playback mode is NOT reported in the JSON
> > summary!  I think this means that our current plan in
> > https://goto.google.com/ChecksumRemainingSteps won't quite work.
> > 
> > Specifically, our idea that we will run gm in a --noFailOnMismatch mode, and
> > then examine the JSON summary in a later step to see if there were any
> failures.
> > 
> > How should we remedy that?  Maybe we should include all rendering modes in
the
> > JSON summary?  Although that would make the file a lot bigger...
> 
> Here's a proposal:
> 
> - If rendering the same gm test in two different modes yields different
bitmaps,
> that will ALWAYS be treated as an error in gm.  (As if gm had thrown a
> segfault... it should NEVER happen, and if it does, the JSON summary should
say
> in effect "we suffered massive failure while trying to run these tests, so we
do
> not have any results to report")
> 
> - Thus, appearance of a test in the "actual-results" portion of the JSON
summary
> implies that *all* rendering modes generated *exactly* the same results.
> 
> - Thus, we don't need to list the actual checksum of every rendering mode...
we
> list a single actual checksum, and readers know that it was the checksum of
> every mode of that test.

I agree with all of this (it's pretty much what we already do), except I think
we should just leave the failed test out of the json file, as if it wasn't
generated.

[epoger, 2013-03-14 18:57:12]

https://codereview.chromium.org/12825005/diff/1/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12825005/diff/1/gm/gmmain.cpp#newcode197
gm/gmmain.cpp:197: fSimulatePipePlaybackFailure = false;
On 2013/03/14 18:48:21, borenet wrote:
> I'm not sure I like the technique of baking test code directly into the
> application itself...

It rubs me the wrong way too, but I couldn't think of a better way of validating
the behavior.  Any ideas?

Adding Ravi in CC... back in TvAds, I could have done this by mocking out some
resource in such a way that it returned failure.  I can't figure out an
analogous approach here, but maybe there is one...

https://codereview.chromium.org/12825005/diff/1/gm/tests/outputs/pipe-playbac...
File gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt
(right):

https://codereview.chromium.org/12825005/diff/1/gm/tests/outputs/pipe-playbac...
gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt:6:
"succeeded" : {
On 2013/03/14 18:48:21, borenet wrote:
> On 2013/03/14 18:16:21, epoger wrote:
> > On 2013/03/14 17:22:19, epoger wrote:
> > > Hmm, note that the error in pipe-playback mode is NOT reported in the JSON
> > > summary!  I think this means that our current plan in
> > > https://goto.google.com/ChecksumRemainingSteps won't quite work.
> > > 
> > > Specifically, our idea that we will run gm in a --noFailOnMismatch mode,
and
> > > then examine the JSON summary in a later step to see if there were any
> > failures.
> > > 
> > > How should we remedy that?  Maybe we should include all rendering modes in
> the
> > > JSON summary?  Although that would make the file a lot bigger...
> > 
> > Here's a proposal:
> > 
> > - If rendering the same gm test in two different modes yields different
> bitmaps,
> > that will ALWAYS be treated as an error in gm.  (As if gm had thrown a
> > segfault... it should NEVER happen, and if it does, the JSON summary should
> say
> > in effect "we suffered massive failure while trying to run these tests, so
we
> do
> > not have any results to report")
> > 
> > - Thus, appearance of a test in the "actual-results" portion of the JSON
> summary
> > implies that *all* rendering modes generated *exactly* the same results.
> > 
> > - Thus, we don't need to list the actual checksum of every rendering mode...
> we
> > list a single actual checksum, and readers know that it was the checksum of
> > every mode of that test.
> 
> I agree with all of this (it's pretty much what we already do), except I think
> we should just leave the failed test out of the json file, as if it wasn't
> generated.

In a way it seems like there would be value in the broken test showing up in the
"failed" portion of "actual-results"... but for now, I think it's reasonable to
just leave it out of the JSON summary and rely on the caller seeing the failure
while running gm...

I'll work on a change that does exactly that.

[borenet, 2013-03-14 19:02:21]

https://codereview.chromium.org/12825005/diff/1/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12825005/diff/1/gm/gmmain.cpp#newcode197
gm/gmmain.cpp:197: fSimulatePipePlaybackFailure = false;
On 2013/03/14 18:57:12, epoger wrote:
> On 2013/03/14 18:48:21, borenet wrote:
> > I'm not sure I like the technique of baking test code directly into the
> > application itself...
> 
> It rubs me the wrong way too, but I couldn't think of a better way of
validating
> the behavior.  Any ideas?
> 
> Adding Ravi in CC... back in TvAds, I could have done this by mocking out some
> resource in such a way that it returned failure.  I can't figure out an
> analogous approach here, but maybe there is one...

Yeah, I guess there's not a good way to force this to fail.  It might be better
if this were behind a #ifdef SELF_TEST thing, or even only included in debug
builds.

[rmistry, 2013-03-14 19:47:33]

https://codereview.chromium.org/12825005/diff/1/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12825005/diff/1/gm/gmmain.cpp#newcode197
gm/gmmain.cpp:197: fSimulatePipePlaybackFailure = false;
On 2013/03/14 18:57:12, epoger wrote:
> On 2013/03/14 18:48:21, borenet wrote:
> > I'm not sure I like the technique of baking test code directly into the
> > application itself...
> 
> It rubs me the wrong way too, but I couldn't think of a better way of
validating
> the behavior.  Any ideas?
> 
> Adding Ravi in CC... back in TvAds, I could have done this by mocking out some
> resource in such a way that it returned failure.  I can't figure out an
> analogous approach here, but maybe there is one...

Sorry, I may have been able to help for Java or Python but I have no experience
with mocking in C++.

[epoger, 2013-03-14 21:18:02]

Feel free to take a look at patchset 2 if you like, but it's just a
checkpoint... patchset 3 coming soon to actually change the behavior.

https://codereview.chromium.org/12825005/diff/10001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12825005/diff/10001/gm/gmmain.cpp#newcode1150
gm/gmmain.cpp:1150: ErrorBitfield run_multiple_configs(GMMain &gmmain, GM *gm,
const SkTDArray<size_t> &configs,
patchset 2 is a refactoring-only change... it extracts run_multiple_configs()
and run_multiple_drawing_modes() into separate functions, to help with an
upcoming patchset.  For now, behavior is unchanged.

[epoger, 2013-03-14 22:36:26]

PTAL at patchset 4

https://codereview.chromium.org/12825005/diff/1/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12825005/diff/1/gm/gmmain.cpp#newcode197
gm/gmmain.cpp:197: fSimulatePipePlaybackFailure = false;
> Yeah, I guess there's not a good way to force this to fail.  It might be
better
> if this were behind a #ifdef SELF_TEST thing, or even only included in debug
> builds.

I'd rather not require a compile-time flag, because that would require the gm
self-test to compile a special version of the code, which seems like both a
hassle and an opportunity for gm to behave different in the "real world" than it
does in test.

I don't think it's so bad to have this command-line option... the code to
implement it is very tight, and anybody who specifies it knows what they will
get!

https://codereview.chromium.org/12825005/diff/18001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12825005/diff/18001/gm/gmmain.cpp#newcode1
gm/gmmain.cpp:1: /*
As of patchset 3+4, we now report any differences across drawing modes as
testsWithDrawingModeDiscrepancies.

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

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/compared...
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout:3:
GM: Ran 1 tests: 0 passed, 1 failed, 0 with drawing mode discrepancies, 0
missing reference images
we now report the number of drawing mode discrepancies for every run

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/pipe-pla...
File gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt
(right):

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/pipe-pla...
gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt:4:
"comparison/selftest1-pipe" : {
So now we report cross-drawing-mode results in the JSON summary, but *only* in
case of failure.  Seem reasonable?

What I like about this is that our CompareGMs step will, as planned, be able to
just look for anything in this "failed" section to report whether the GM results
were OK or not.

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/pipe-pla...
gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt:29:
"checksums" : [ 14022967492765711532 ],
Regardless of whether multiple checksums were allowed for 8888/selftest1 , only
one expected checksum will show up here: the actual results we got for
8888/selftest1 in this run.

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/pipe-pla...
File gm/tests/outputs/pipe-playback-failure/output-expected/stdout (right):

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/pipe-pla...
gm/tests/outputs/pipe-playback-failure/output-expected/stdout:3: GM: Ran 1
tests: 0 passed, 1 failed, 1 with drawing mode discrepancies, 0 missing
reference images
Here's one that actually reports a drawing mode discrepancy.

[borenet, 2013-03-15 20:15:53]

I have a few concerns...

https://codereview.chromium.org/12825005/diff/1/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12825005/diff/1/gm/gmmain.cpp#newcode197
gm/gmmain.cpp:197: fSimulatePipePlaybackFailure = false;
On 2013/03/14 22:36:26, epoger wrote:
> > Yeah, I guess there's not a good way to force this to fail.  It might be
> better
> > if this were behind a #ifdef SELF_TEST thing, or even only included in debug
> > builds.
> 
> I'd rather not require a compile-time flag, because that would require the gm
> self-test to compile a special version of the code, which seems like both a
> hassle and an opportunity for gm to behave different in the "real world" than
it
> does in test.
> 
> I don't think it's so bad to have this command-line option... the code to
> implement it is very tight, and anybody who specifies it knows what they will
> get!

Fair enough.

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

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/compared...
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout:3:
GM: Ran 1 tests: 0 passed, 1 failed, 0 with drawing mode discrepancies, 0
missing reference images
On 2013/03/14 22:36:27, epoger wrote:
> we now report the number of drawing mode discrepancies for every run

I like this, but I might rather the wording change to something like, "%d
passed, %d did not match expectations, %d had differences between drawing modes,
%d missing reference images"  Just to make it clear what we now mean by
"failed."

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/pipe-pla...
File gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt
(right):

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/pipe-pla...
gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt:4:
"comparison/selftest1-pipe" : {
On 2013/03/14 22:36:27, epoger wrote:
> So now we report cross-drawing-mode results in the JSON summary, but *only* in
> case of failure.  Seem reasonable?
> 
> What I like about this is that our CompareGMs step will, as planned, be able
to
> just look for anything in this "failed" section to report whether the GM
results
> were OK or not.

This effectively treats the built-in comparisons, which I had thought of as
assertions, as additional GMs.  Will we also write out images for these on
failure?  While that wasn't really considered before, it might be helpful in
diagnosing problems.

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/pipe-pla...
gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt:29:
"checksums" : [ 14022967492765711532 ],
On 2013/03/14 22:36:27, epoger wrote:
> Regardless of whether multiple checksums were allowed for 8888/selftest1 ,
only
> one expected checksum will show up here: the actual results we got for
> 8888/selftest1 in this run.

I'm a bit confused by this.  Is the expectation for comparison/selftest1-pipe
defined as the actual result for 8888/selftest1 for THIS run?  This breaks the
assumption that we know all of our expectations BEFORE running gm, even though
it kind of makes sense in a this-is-how-it-currently-works kind of way.  This
may not be a bad thing, as long as it's well-documented.  What's the expected
behavior when the expectations file that we give to gm contains an expectation
for comparison/selftest1-pipe?  What if it's different from 8888/selftest1?

Alternatively, the expectation for comparison/selftest1-pipe could be made to be
the same as the pre-gm expectation for 8888/selftest1.  This would cause *both*
tests to fail when the rendering changes but the two draw the same new image and
would cause only comparison/selftest1-pipe in the case that it's designed to
test.  In that case we wouldn't really need to include comparison/* in the
expecations at all, which is consistent with what we do now.

[epoger, 2013-03-15 22:05:55]

Thanks, Eric...

I don't have access to my checkout at the moment to update this CL anyway... but
please see my responses below and let me know if you have any bright ideas.

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

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/compared...
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout:3:
GM: Ran 1 tests: 0 passed, 1 failed, 0 with drawing mode discrepancies, 0
missing reference images
On 2013/03/15 20:15:53, borenet wrote:
> On 2013/03/14 22:36:27, epoger wrote:
> > we now report the number of drawing mode discrepancies for every run
> 
> I like this, but I might rather the wording change to something like, "%d
> passed, %d did not match expectations, %d had differences between drawing
modes,
> %d missing reference images"  Just to make it clear what we now mean by
> "failed."

One thing you should be aware of is that a single test can fail in more than one
way.  For example, a test could have differences across drawing modes and ALSO
be missing expectations.  So this could get confusing.  ("Hey, it says 1 test
had failure A, and 1 test had failure B, but only 1 test ran!")

I don't know the best way to communicate that.  Any ideas?

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/pipe-pla...
File gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt
(right):

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/pipe-pla...
gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt:4:
"comparison/selftest1-pipe" : {
> This effectively treats the built-in comparisons, which I had thought of as
> assertions, as additional GMs.  Will we also write out images for these on
> failure?  

I hadn't thought about that either way.  Seems like it would be useful (when we
add the option of writing out failed images only).

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/pipe-pla...
gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt:29:
"checksums" : [ 14022967492765711532 ],
On 2013/03/15 20:15:53, borenet wrote:
> On 2013/03/14 22:36:27, epoger wrote:
> > Regardless of whether multiple checksums were allowed for 8888/selftest1 ,
> only
> > one expected checksum will show up here: the actual results we got for
> > 8888/selftest1 in this run.
> 
> I'm a bit confused by this.  Is the expectation for comparison/selftest1-pipe
> defined as the actual result for 8888/selftest1 for THIS run?  This breaks the
> assumption that we know all of our expectations BEFORE running gm, even though
> it kind of makes sense in a this-is-how-it-currently-works kind of way.  This
> may not be a bad thing, as long as it's well-documented.  What's the expected
> behavior when the expectations file that we give to gm contains an expectation
> for comparison/selftest1-pipe?  What if it's different from 8888/selftest1?

Yes, this could get confusing, or we could end up with an explosion of
expectations...

Presently:
1. The expectation for comparison/selftest1-pipe is defined as the actual result
for 8888/selftest1 for THIS run.
2. Any expectations for comparison/selftest1-pipe (in the JSON expected
checksums file) will be IGNORED, as if they were for 8888/nonexistent_test.

These behaviors are motivated by:
A. Our belief that all rendering modes should result in exactly the same bitmap.
B. Our desire to not have a ridiculously large expectations file, with identical
expectations for every drawing mode of each test.

> 
> Alternatively, the expectation for comparison/selftest1-pipe could be made to
be
> the same as the pre-gm expectation for 8888/selftest1.  This would cause
*both*
> tests to fail when the rendering changes but the two draw the same new image
and
> would cause only comparison/selftest1-pipe in the case that it's designed to
> test.  In that case we wouldn't really need to include comparison/* in the
> expecations at all, which is consistent with what we do now.

If we truly believe that results should be the same across all drawing modes at
all times, then I think the present behavior is better.  That way, even if we
don't have expectations for the test, we will fail if there is a difference
across drawing modes.

But I'm certainly open to change on this...

[borenet, 2013-03-18 12:26:37]

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

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/compared...
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout:3:
GM: Ran 1 tests: 0 passed, 1 failed, 0 with drawing mode discrepancies, 0
missing reference images
On 2013/03/15 22:05:55, epoger wrote:
> On 2013/03/15 20:15:53, borenet wrote:
> > On 2013/03/14 22:36:27, epoger wrote:
> > > we now report the number of drawing mode discrepancies for every run
> > 
> > I like this, but I might rather the wording change to something like, "%d
> > passed, %d did not match expectations, %d had differences between drawing
> modes,
> > %d missing reference images"  Just to make it clear what we now mean by
> > "failed."
> 
> One thing you should be aware of is that a single test can fail in more than
one
> way.  For example, a test could have differences across drawing modes and ALSO
> be missing expectations.  So this could get confusing.  ("Hey, it says 1 test
> had failure A, and 1 test had failure B, but only 1 test ran!")
> 
> I don't know the best way to communicate that.  Any ideas?

Is it critical that the summary be one line?  What if we listed failures by gm:

selftest1: did not match expecations
othergm02: missing reference image
othergm03: drawing mode discrepancy, missing reference image
othergm04: drawing mode discrepancy (can we be more specific?)

The downside here is that we'd get much more output, especially when we don't
provide any expecations, but I can't think of a scenario where we wouldn't want
to know exactly which gms had failures.  As for the missing expectations, I
guess we could summarize those in a single line, although it's probably also
helpful to know which are missing expecations so that we can provide
expectations.

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/pipe-pla...
File gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt
(right):

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/pipe-pla...
gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt:29:
"checksums" : [ 14022967492765711532 ],
On 2013/03/15 22:05:55, epoger wrote:
> On 2013/03/15 20:15:53, borenet wrote:
> > On 2013/03/14 22:36:27, epoger wrote:
> > > Regardless of whether multiple checksums were allowed for 8888/selftest1 ,
> > only
> > > one expected checksum will show up here: the actual results we got for
> > > 8888/selftest1 in this run.
> > 
> > I'm a bit confused by this.  Is the expectation for
comparison/selftest1-pipe
> > defined as the actual result for 8888/selftest1 for THIS run?  This breaks
the
> > assumption that we know all of our expectations BEFORE running gm, even
though
> > it kind of makes sense in a this-is-how-it-currently-works kind of way. 
This
> > may not be a bad thing, as long as it's well-documented.  What's the
expected
> > behavior when the expectations file that we give to gm contains an
expectation
> > for comparison/selftest1-pipe?  What if it's different from 8888/selftest1?
> 
> Yes, this could get confusing, or we could end up with an explosion of
> expectations...
> 
> Presently:
> 1. The expectation for comparison/selftest1-pipe is defined as the actual
result
> for 8888/selftest1 for THIS run.
> 2. Any expectations for comparison/selftest1-pipe (in the JSON expected
> checksums file) will be IGNORED, as if they were for 8888/nonexistent_test.

Should this be an error?  "Invalid expectation" or "Unknown test"?

> 
> These behaviors are motivated by:
> A. Our belief that all rendering modes should result in exactly the same
bitmap.
> B. Our desire to not have a ridiculously large expectations file, with
identical
> expectations for every drawing mode of each test.
> 
> > 
> > Alternatively, the expectation for comparison/selftest1-pipe could be made
to
> be
> > the same as the pre-gm expectation for 8888/selftest1.  This would cause
> *both*
> > tests to fail when the rendering changes but the two draw the same new image
> and
> > would cause only comparison/selftest1-pipe in the case that it's designed to
> > test.  In that case we wouldn't really need to include comparison/* in the
> > expecations at all, which is consistent with what we do now.
> 
> If we truly believe that results should be the same across all drawing modes
at
> all times, then I think the present behavior is better.  That way, even if we
> don't have expectations for the test, we will fail if there is a difference
> across drawing modes.
> 
> But I'm certainly open to change on this...

This sounds fine with me, as long as it's made very clear where the extra
expectations are coming from.

[epoger, 2013-03-20 17:27:51]

I tried to sync this to HEAD today, and there have been so many changes to
gmmain that it got quite difficult... I've created a new
https://codereview.chromium.org/12536007/ ('gm refactor: extract
run_multiple_configs() and run_multiple_modes()') to land part of this, and I'll
take it from there.

[epoger, 2013-03-20 19:06:59]

On 2013/03/20 17:27:51, epoger wrote:
> I tried to sync this to HEAD today, and there have been so many changes to
> gmmain that it got quite difficult... I've created a new
> https://codereview.chromium.org/12536007/ ('gm refactor: extract
> run_multiple_configs() and run_multiple_modes()') to land part of this, and
I'll
> take it from there.

Copied all these changes into a new workspace, synced to r8270, as
https://codereview.chromium.org/12807006

I will make some changes there based on Eric's comments above, and send mail on
that OTHER codereview when it's ready for review.

[epoger, 2013-03-21 16:08:32]

Eric- I think we are down to one last item that should be resolved before I can
commit the updated version of this CL ( https://codereview.chromium.org/12807006
).  PTAL and tell me what you think...

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

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/compared...
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout:3:
GM: Ran 1 tests: 0 passed, 1 failed, 0 with drawing mode discrepancies, 0
missing reference images
On 2013/03/18 12:26:37, borenet wrote:
> Is it critical that the summary be one line?  What if we listed failures by
gm:
> 
> selftest1: did not match expecations
> othergm02: missing reference image
> othergm03: drawing mode discrepancy, missing reference image
> othergm04: drawing mode discrepancy (can we be more specific?)
> 
> The downside here is that we'd get much more output, especially when we don't
> provide any expecations, but I can't think of a scenario where we wouldn't
want
> to know exactly which gms had failures.  As for the missing expectations, I
> guess we could summarize those in a single line, although it's probably also
> helpful to know which are missing expecations so that we can provide
> expectations.

How about if gm generates something similar to the skdiff output (e.g.
https://code.google.com/p/skia/source/browse/trunk/tools/tests/skdiff/test2/o...
), but with the addition of one single-line summary at the top?  (That
single-line summary is probably handy in many cases, and cheap to generate.)

So, gm/tests/outputs/pipe-playback-failure/output-expected/stdout would look
like this:

GM: summary: ran 1 tests: 0 NoGpuContext, 1 MismatchRenderingMode, 0
MismatchExpectations, 0 MissingExpectations, 0 ErrorWritingImage
GM: [*] 0 NoGpuContext :
GM: [*] 1 MismatchRenderingMode : comparison/selftest1-pipe
GM: [*] 0 MismatchExpectations :
GM: [_] 0 MissingExpectations :
GM: [*] 0 ErrorWritingImage :
GM: (results marked with [*] will cause nonzero return value)

(I have also changed the categories a bit, making MismatchRenderingMode distinct
from MismatchExpectations.)

And
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout
would look like this:

GM: summary: ran 1 tests: 0 NoGpuContext, 0 MismatchRenderingMode, 2
MismatchExpectations, 0 MissingExpectations, 0 ErrorWritingImage
GM: [*] 0 NoGpuContext :
GM: [*] 0 MismatchRenderingMode :
GM: [*] 2 MismatchExpectations : 8888/selftest1 565/selftest1
GM: [_] 0 MissingExpectations :
GM: [*] 0 ErrorWritingImage :
GM: (results marked with [*] will cause nonzero return value)

One thing you will note in this latter case is that just 1 test was run, but 2
errors were reported!  That's because the test was run in 2 different config
modes.  Do you think that would be problematic?  I think we could find ways to
improve that part of the reporting over time...

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/pipe-pla...
File gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt
(right):

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/pipe-pla...
gm/tests/outputs/pipe-playback-failure/output-expected/json-summary.txt:29:
"checksums" : [ 14022967492765711532 ],
On 2013/03/18 12:26:37, borenet wrote:
> > 2. Any expectations for comparison/selftest1-pipe (in the JSON expected
> > checksums file) will be IGNORED, as if they were for 8888/nonexistent_test.
> 
> Should this be an error?  "Invalid expectation" or "Unknown test"?

Ideally, yes.  I think it's a long-term issue, not a short-term one... filed as
https://code.google.com/p/skia/issues/detail?id=1177 ('gm: report error if any
expected gms did not run')

[borenet, 2013-03-21 18:12:47]

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

https://codereview.chromium.org/12825005/diff/18001/gm/tests/outputs/compared...
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout:3:
GM: Ran 1 tests: 0 passed, 1 failed, 0 with drawing mode discrepancies, 0
missing reference images
On 2013/03/21 16:08:32, epoger wrote:
> On 2013/03/18 12:26:37, borenet wrote:
> > Is it critical that the summary be one line?  What if we listed failures by
> gm:
> > 
> > selftest1: did not match expecations
> > othergm02: missing reference image
> > othergm03: drawing mode discrepancy, missing reference image
> > othergm04: drawing mode discrepancy (can we be more specific?)
> > 
> > The downside here is that we'd get much more output, especially when we
don't
> > provide any expecations, but I can't think of a scenario where we wouldn't
> want
> > to know exactly which gms had failures.  As for the missing expectations, I
> > guess we could summarize those in a single line, although it's probably also
> > helpful to know which are missing expecations so that we can provide
> > expectations.
> 
> How about if gm generates something similar to the skdiff output (e.g.
>
https://code.google.com/p/skia/source/browse/trunk/tools/tests/skdiff/test2/o...
> ), but with the addition of one single-line summary at the top?  (That
> single-line summary is probably handy in many cases, and cheap to generate.)
> 
> So, gm/tests/outputs/pipe-playback-failure/output-expected/stdout would look
> like this:
> 
> GM: summary: ran 1 tests: 0 NoGpuContext, 1 MismatchRenderingMode, 0
> MismatchExpectations, 0 MissingExpectations, 0 ErrorWritingImage
> GM: [*] 0 NoGpuContext :
> GM: [*] 1 MismatchRenderingMode : comparison/selftest1-pipe
> GM: [*] 0 MismatchExpectations :
> GM: [_] 0 MissingExpectations :
> GM: [*] 0 ErrorWritingImage :
> GM: (results marked with [*] will cause nonzero return value)
> 
> (I have also changed the categories a bit, making MismatchRenderingMode
distinct
> from MismatchExpectations.)
> 
> And
>
gm/tests/outputs/compared-against-different-pixels-images/output-expected/stdout
> would look like this:
> 
> GM: summary: ran 1 tests: 0 NoGpuContext, 0 MismatchRenderingMode, 2
> MismatchExpectations, 0 MissingExpectations, 0 ErrorWritingImage
> GM: [*] 0 NoGpuContext :
> GM: [*] 0 MismatchRenderingMode :
> GM: [*] 2 MismatchExpectations : 8888/selftest1 565/selftest1
> GM: [_] 0 MissingExpectations :
> GM: [*] 0 ErrorWritingImage :
> GM: (results marked with [*] will cause nonzero return value)
> 
> One thing you will note in this latter case is that just 1 test was run, but 2
> errors were reported!  That's because the test was run in 2 different config
> modes.  Do you think that would be problematic?  I think we could find ways to
> improve that part of the reporting over time...

Consistency is good!  I like it, but as you demonstrated, it doesn't solve the
issue of a single GM failing in multiple ways.  However, I don't really think
that's a big problem, since we rarely (to my knowledge) use the number of
failures as opposed to wanting to know *what* failed and why.

[epoger, 2013-03-21 18:17:46]

On 2013/03/21 18:12:47, borenet wrote:
> Consistency is good!  I like it, but as you demonstrated, it doesn't solve the
> issue of a single GM failing in multiple ways.  However, I don't really think
> that's a big problem, since we rarely (to my knowledge) use the number of
> failures as opposed to wanting to know *what* failed and why.

So are you in favor of me changing it to give output as I described above?  From
your reply, I *think* so, but I'm not sure. :-)

[borenet, 2013-03-21 18:19:50]

On 2013/03/21 18:17:46, epoger wrote:
> On 2013/03/21 18:12:47, borenet wrote:
> > Consistency is good!  I like it, but as you demonstrated, it doesn't solve
the
> > issue of a single GM failing in multiple ways.  However, I don't really
think
> > that's a big problem, since we rarely (to my knowledge) use the number of
> > failures as opposed to wanting to know *what* failed and why.
> 
> So are you in favor of me changing it to give output as I described above? 
From
> your reply, I *think* so, but I'm not sure. :-)

I guess both consistency *and* clarity are good.  Yes, I think that's a good way
to go.