GM: allow caller to specify which result types trigger an error

GM: allow caller to specify which result types trigger an error
Committed: https://code.google.com/p/skia/source/detail?r=8652

[epoger, 2013-04-11 21:05:04]

Created to fulfill the need for a --noFailOnMismatch option, as noted in
https://goto.google.com/ChecksumRemainingSteps

[borenet, 2013-04-12 00:29:44]

Nice! LGTM, with a couple of questions. Not sure what happened with the trybots
- maybe a network blip during the upload?

https://codereview.chromium.org/14187007/diff/1/gm/gm_error.h
File gm/gm_error.h (right):

https://codereview.chromium.org/14187007/diff/1/gm/gm_error.h#newcode69
gm/gm_error.h:69: if (0 == strcmp(name, "NoGpuContext")) {
Suggest making these strings constant and using them in getErrorTypeName above. 
There's probably some macro magic that can be done to convert back and forth,
but that probably isn't necessary.

https://codereview.chromium.org/14187007/diff/1/gm/gm_error.h#newcode144
gm/gm_error.h:144: if (0 < howMany++) {
Is this significantly faster than (s.size() == 0)?

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

https://codereview.chromium.org/14187007/diff/1/gm/gmmain.cpp#newcode1205
gm/gmmain.cpp:1205: // values differently, so that the first definition of
ignoreErrorTypes worked?
Good question for scroggo@.  I'm not terribly familiar with SkCommandLineFlags.

[epoger, 2013-04-12 02:44:43]

PTAL at patchset 3...

I'll try the trybots again, while we're at it.

https://codereview.chromium.org/14187007/diff/1/gm/gm_error.h
File gm/gm_error.h (right):

https://codereview.chromium.org/14187007/diff/1/gm/gm_error.h#newcode69
gm/gm_error.h:69: if (0 == strcmp(name, "NoGpuContext")) {
On 2013/04/12 00:29:45, borenet wrote:
> Suggest making these strings constant and using them in getErrorTypeName
above. 
> There's probably some macro magic that can be done to convert back and forth,
> but that probably isn't necessary.

Hey, I came up with an elegant, robust (IMHO) approach in patchset 3.  Whaddaya
think?

https://codereview.chromium.org/14187007/diff/1/gm/gm_error.h#newcode144
gm/gm_error.h:144: if (0 < howMany++) {
On 2013/04/12 00:29:45, borenet wrote:
> Is this significantly faster than (s.size() == 0)?

Probably not.  Now calling s.isEmpty()... thanks for the suggestion

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

https://codereview.chromium.org/14187007/diff/1/gm/gmmain.cpp#newcode1205
gm/gmmain.cpp:1205: // values differently, so that the first definition of
ignoreErrorTypes worked?
On 2013/04/12 00:29:45, borenet wrote:
> Good question for scroggo@.  I'm not terribly familiar with
SkCommandLineFlags.

Yeah, I wanted to commit this change and then see if we can improve
SkCommandLineFlags (or teach me how to use it properly!).

I tried using the ifdef-ed out approach, and whenever gm was run without
--ignoreErrorTypes specified (i.e., when it evaulated the default for this
option), I got this error:

GM: cannot find ErrorType with name 'Exercise the pdf rendering test pass.'

(That string of course coming from the definition of the pdf flag further
down!!!  Yikes!!!)

[epoger, 2013-04-12 03:21:47]

On 2013/04/12 00:29:44, borenet wrote:
> Not sure what happened with the trybots
> - maybe a network blip during the upload?

Filed https://code.google.com/p/skia/issues/detail?id=1233 ('for SOME
changelists, trybots always fail ApplyPatch with "malformed patch" error'). 
It's a consistent problem, for this particular changelist.  More details in the
bug.

[borenet, 2013-04-12 11:55:26]

On 2013/04/12 03:21:47, epoger wrote:
> On 2013/04/12 00:29:44, borenet wrote:
> > Not sure what happened with the trybots
> > - maybe a network blip during the upload?
> 
> Filed https://code.google.com/p/skia/issues/detail?id=1233 ('for SOME
> changelists, trybots always fail ApplyPatch with "malformed patch" error'). 
> It's a consistent problem, for this particular changelist.  More details in
the
> bug.

Did you try submitting the try request from Rietveld?  I wonder if it behaves
differently...

LGTM at patch set 3.

[borenet, 2013-04-12 11:55:37]

https://codereview.chromium.org/14187007/diff/1/gm/gm_error.h
File gm/gm_error.h (right):

https://codereview.chromium.org/14187007/diff/1/gm/gm_error.h#newcode69
gm/gm_error.h:69: if (0 == strcmp(name, "NoGpuContext")) {
On 2013/04/12 02:44:43, epoger wrote:
> On 2013/04/12 00:29:45, borenet wrote:
> > Suggest making these strings constant and using them in getErrorTypeName
> above. 
> > There's probably some macro magic that can be done to convert back and
forth,
> > but that probably isn't necessary.
> 
> Hey, I came up with an elegant, robust (IMHO) approach in patchset 3. 
Whaddaya
> think?

I like it!

[epoger, 2013-04-12 14:07:43]

On 2013/04/12 11:55:26, borenet wrote:
> On 2013/04/12 03:21:47, epoger wrote:
> Did you try submitting the try request from Rietveld?  I wonder if it behaves
> differently...

Good point... no, I had only used submit_try.  I've sent off a couple of trybots
using the Rietveld UI; we'll see how they do.

[epoger, 2013-04-12 14:11:42]

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

[epoger, 2013-04-12 14:16:38]

On 2013/04/12 14:07:43, epoger wrote:
> On 2013/04/12 11:55:26, borenet wrote:
> > On 2013/04/12 03:21:47, epoger wrote:
> > Did you try submitting the try request from Rietveld?  I wonder if it
behaves
> > differently...
> 
> Good point... no, I had only used submit_try.  I've sent off a couple of
trybots
> using the Rietveld UI; we'll see how they do.

Eric, did you submit a try job using the Rietveld UI?  It looks like one was
triggered, and I don't remember doing it...
http://108.170.217.252:10117/builders/Skia_Shuttle_Ubuntu12_ATI5770_Float_Rel...

It still fails, but in a *different* way than the submit_try job.  I'll put
details in https://code.google.com/p/skia/issues/detail?id=1233

[borenet, 2013-04-12 14:18:56]

On 2013/04/12 14:16:38, epoger wrote:
> On 2013/04/12 14:07:43, epoger wrote:
> > On 2013/04/12 11:55:26, borenet wrote:
> > > On 2013/04/12 03:21:47, epoger wrote:
> > > Did you try submitting the try request from Rietveld?  I wonder if it
> behaves
> > > differently...
> > 
> > Good point... no, I had only used submit_try.  I've sent off a couple of
> trybots
> > using the Rietveld UI; we'll see how they do.
> 
> Eric, did you submit a try job using the Rietveld UI?  It looks like one was
> triggered, and I don't remember doing it...
>
http://108.170.217.252:10117/builders/Skia_Shuttle_Ubuntu12_ATI5770_Float_Rel...
> 
> It still fails, but in a *different* way than the submit_try job.  I'll put
> details in https://code.google.com/p/skia/issues/detail?id=1233

Yes, I triggered it to see what would happen.  I think the problem is that Patch
has no concept of a "moved" or "copied" file.

[scroggo, 2013-04-12 15:24:30]

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

https://codereview.chromium.org/14187007/diff/1/gm/gmmain.cpp#newcode1205
gm/gmmain.cpp:1205: // values differently, so that the first definition of
ignoreErrorTypes worked?
On 2013/04/12 02:44:43, epoger wrote:
> On 2013/04/12 00:29:45, borenet wrote:
> > Good question for scroggo@.  I'm not terribly familiar with
> SkCommandLineFlags.
> 
> Yeah, I wanted to commit this change and then see if we can improve
> SkCommandLineFlags (or teach me how to use it properly!).
> 
> I tried using the ifdef-ed out approach, and whenever gm was run without
> --ignoreErrorTypes specified (i.e., when it evaulated the default for this
> option), I got this error:
> 
> GM: cannot find ErrorType with name 'Exercise the pdf rendering test pass.'
> 
> (That string of course coming from the definition of the pdf flag further
> down!!!  Yikes!!!)

Created https://code.google.com/p/skia/issues/detail?id=1237