Fix ignored errors in GM when no reference images are provided

Fix ignored errors in GM when no reference images are provided
Committed: https://code.google.com/p/skia/source/detail?r=7813

[EricB, 2013-02-20 21:00:16]

I *think* that this is the desired behavior.  An ERROR_READING_REFERENCE_IMAGE
should still be an error unless there is no reference image.

[EricB, 2013-02-20 21:47:37]

Patch set 2 gives an alternative: If there are no reference images, the test is
marked as such if the only error which occurred was reading the reference image.
 If a more "serious" error occurred, the test is marked as failed.

[epoger, 2013-02-21 17:13:09]

This CL addresses http://code.google.com/p/skia/issues/detail?id=1124 ('Hole in
SkPicture testing')

Reviewing now...

[epoger, 2013-02-21 17:29:01]

https://codereview.appspot.com/7400044/diff/3001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.appspot.com/7400044/diff/3001/gm/gmmain.cpp#newcode1650
gm/gmmain.cpp:1650: if (!gmmain.fExpectationsSource.get() &&
I don't think this is quite right.  testsMissingReferenceImages will ONLY be
incremented if gmmain.fExpectationsSource.get() is NULL (--readPath was not
specified at all)

This ugliness has a lot to do with ErrorBitfield's dual life: bitfield by day,
enum by night.  (See
https://code.google.com/p/skia/source/browse/trunk/gm/gmmain.cpp?r=7724#83 )

Until we resolve that larger issue, maybe we could say that a
MissingReferenceImages test ALSO either Failed or Passed, like this:

        if (!gmmain.fExpectationsSource.get() ||
            (ERROR_READING_REFERENCE_IMAGE & testErrors)) {
            testsMissingReferenceImages++;
        }
        if (ERROR_NONE == testErrors || ERROR_READING_REFERENCE_IMAGE ==
testErrors) {
            testsPassed++;
        } else {
            testsFailed++;
        }

I think this will require a small change to the gm self-test expected results...

[EricB, 2013-02-21 18:08:28]

Uploaded patch set 4.

https://codereview.appspot.com/7400044/diff/3001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.appspot.com/7400044/diff/3001/gm/gmmain.cpp#newcode1650
gm/gmmain.cpp:1650: if (!gmmain.fExpectationsSource.get() &&
On 2013/02/21 17:29:01, epoger wrote:
> I don't think this is quite right.  testsMissingReferenceImages will ONLY be
> incremented if gmmain.fExpectationsSource.get() is NULL (--readPath was not
> specified at all)
> 
> This ugliness has a lot to do with ErrorBitfield's dual life: bitfield by day,
> enum by night.  (See
> https://code.google.com/p/skia/source/browse/trunk/gm/gmmain.cpp?r=7724#83 )
> 
> Until we resolve that larger issue, maybe we could say that a
> MissingReferenceImages test ALSO either Failed or Passed, like this:
> 
>         if (!gmmain.fExpectationsSource.get() ||
>             (ERROR_READING_REFERENCE_IMAGE & testErrors)) {
>             testsMissingReferenceImages++;
>         }
>         if (ERROR_NONE == testErrors || ERROR_READING_REFERENCE_IMAGE ==
> testErrors) {
>             testsPassed++;
>         } else {
>             testsFailed++;
>         }
> 
> I think this will require a small change to the gm self-test expected
results...

I had been incorrectly thinking of the result of each test as one of (Passed,
Failed, MissingReference).  It makes more sense that MissingReference can occur
with a Pass or a Fail.  Changed as suggested.  For the record, the GM self-tests
pass on my machine.

[epoger, 2013-02-21 18:12:58]

LGTM at patchset 4
(as long as it still detects the error like it did at patchset 2)

https://codereview.appspot.com/7400044/diff/3001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.appspot.com/7400044/diff/3001/gm/gmmain.cpp#newcode1650
gm/gmmain.cpp:1650: if (!gmmain.fExpectationsSource.get() &&
On 2013/02/21 18:08:28, EricB wrote:
> I had been incorrectly thinking of the result of each test as one of (Passed,
> Failed, MissingReference).  

In your defense, that had been the case thus far!

[EricB, 2013-02-21 18:34:25]

On 2013/02/21 18:12:58, epoger wrote:
> LGTM at patchset 4
> (as long as it still detects the error like it did at patchset 2)
> 
> https://codereview.appspot.com/7400044/diff/3001/gm/gmmain.cpp
> File gm/gmmain.cpp (right):
> 
> https://codereview.appspot.com/7400044/diff/3001/gm/gmmain.cpp#newcode1650
> gm/gmmain.cpp:1650: if (!gmmain.fExpectationsSource.get() &&
> On 2013/02/21 18:08:28, EricB wrote:
> > I had been incorrectly thinking of the result of each test as one of
(Passed,
> > Failed, MissingReference).  
> 
> In your defense, that had been the case thus far!

My try (correctly) failed:
http://70.32.156.53:10115/builders/Skia_Shuttle_Ubuntu12_ATI5770_Float_Debug_...

Committed as r7813.

[Leon, 2013-02-21 18:38:23]

> This ugliness has a lot to do with ErrorBitfield's dual life: bitfield
> by day, enum by night.  (See
> https://code.google.com/p/**skia/source/browse/trunk/gm/**
>
gmmain.cpp?r=7724#83<https://code.google.com/p/skia/source/browse/trunk/gm/gmmain.cpp?r=7724#83>


FWIW, we actually use enums as bitfields in Skia (For example
https://code.google.com/p/skia/source/browse/trunk/include/core/SkPaint.h?r=7...
)

-- 
Leon Scroggins III
scroggo@google.com

[epoger, 2013-02-21 19:25:32]

On Thu, Feb 21, 2013 at 1:38 PM, Leon Scroggins <scroggo@google.com> wrote:
>
>> This ugliness has a lot to do with ErrorBitfield's dual life: bitfield
>> by day, enum by night.  (See
>> https://code.google.com/p/skia/source/browse/trunk/gm/gmmain.cpp?r=7724#83
>
>
> FWIW, we actually use enums as bitfields in Skia (For example
>
https://code.google.com/p/skia/source/browse/trunk/include/core/SkPaint.h?r=7...)

Well, SkPaint::Flags is an enum of bitfield *masks*, which is fine.

What would be bad is if SkPaint::fFlags was expected to be EITHER
kAntiAlias_Flag OR kUnderlineText_Flag , rather than allowing for
arbitrary combinations.

And that's what we do with ErrorBitfield in GM. :-(

>
> --
> Leon Scroggins III
> scroggo@google.com