GM: add --missingExpectationsPath option, similar to --mismatchPath

GM: add --missingExpectationsPath option, similar to --mismatchPath

This will allow us to make the buildbots record all images EXCEPT those which
match expectations to Google Storage.

R=borenet@google.com

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

[epoger, 2013-06-21 17:39:42]

https://codereview.chromium.org/17552012/diff/1/gm/tests/run.sh
File gm/tests/run.sh (right):

https://codereview.chromium.org/17552012/diff/1/gm/tests/run.sh#newcode112
gm/tests/run.sh:112: # Add a file to any empty subdirectories.
Patchset 1 fixes the way we prevent empty subdirectories from being written.  No
changes to the GM binary yet, just the self-test.

[epoger, 2013-06-21 17:46:44]

Ready for review at patchset 2

https://codereview.chromium.org/17552012/diff/4001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/17552012/diff/4001/gm/gmmain.cpp#newcode196
gm/gmmain.cpp:196: fMismatchPath(NULL), fMissingExpectationsPath(NULL),
fTestsRun(0),
Patchset 2 adds the new missingExpectationsPath flag to GM

https://codereview.chromium.org/17552012/diff/4001/gm/tests/outputs/checksum-...
File gm/tests/outputs/checksum-based-filenames/output-expected/command_line
(right):

https://codereview.chromium.org/17552012/diff/4001/gm/tests/outputs/checksum-...
gm/tests/outputs/checksum-based-filenames/output-expected/command_line:1:
out/Debug/gm --verbose --writeChecksumBasedFilenames --match selftest1 --config
8888 565 -r gm/tests/inputs/json/different-pixels-no-hierarchy.json
--writeJsonSummaryPath
gm/tests/outputs/checksum-based-filenames/output-actual/json-summary.txt
--writePath gm/tests/outputs/checksum-based-filenames/output-actual/writePath
--mismatchPath
gm/tests/outputs/checksum-based-filenames/output-actual/mismatchPath
--missingExpectationsPath
gm/tests/outputs/checksum-based-filenames/output-actual/missingExpectationsPath
in all the command_line files, we see this new flag being passed

https://codereview.chromium.org/17552012/diff/4001/gm/tests/outputs/compared-...
File
gm/tests/outputs/compared-against-empty-dir/output-expected/missingExpectationsPath/565/selftest1.png
(right):

https://codereview.chromium.org/17552012/diff/4001/gm/tests/outputs/compared-...
gm/tests/outputs/compared-against-empty-dir/output-expected/missingExpectationsPath/565/selftest1.png:1:
[contents of
gm/tests/outputs/compared-against-empty-dir/output-actual/missingExpectationsPath/565/selftest1.png]
This is one of just a few test cases where we have missing expectations... so,
as expected, the actual images are written out in these cases.

[borenet, 2013-06-21 18:01:14]

https://codereview.chromium.org/17552012/diff/4001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/17552012/diff/4001/gm/gmmain.cpp#newcode196
gm/gmmain.cpp:196: fMismatchPath(NULL), fMissingExpectationsPath(NULL),
fTestsRun(0),
On 2013/06/21 17:46:44, epoger wrote:
> Patchset 2 adds the new missingExpectationsPath flag to GM

So, we put the actuals for GMs with no expectations in a different place from
those with mismatches.  Is this so that we can upload them to different places? 
I think we want to upload them in either case.

https://codereview.chromium.org/17552012/diff/4001/gm/tests/outputs/checksum-...
File gm/tests/outputs/checksum-based-filenames/output-expected/command_line
(right):

https://codereview.chromium.org/17552012/diff/4001/gm/tests/outputs/checksum-...
gm/tests/outputs/checksum-based-filenames/output-expected/command_line:1:
out/Debug/gm --verbose --writeChecksumBasedFilenames --match selftest1 --config
8888 565 -r gm/tests/inputs/json/different-pixels-no-hierarchy.json
--writeJsonSummaryPath
gm/tests/outputs/checksum-based-filenames/output-actual/json-summary.txt
--writePath gm/tests/outputs/checksum-based-filenames/output-actual/writePath
--mismatchPath
gm/tests/outputs/checksum-based-filenames/output-actual/mismatchPath
--missingExpectationsPath
gm/tests/outputs/checksum-based-filenames/output-actual/missingExpectationsPath
These expectations are getting really unwieldy.  Is there a good way to
simplify?

[epoger, 2013-06-21 18:06:46]

https://codereview.chromium.org/17552012/diff/4001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/17552012/diff/4001/gm/gmmain.cpp#newcode196
gm/gmmain.cpp:196: fMismatchPath(NULL), fMissingExpectationsPath(NULL),
fTestsRun(0),
On 2013/06/21 18:01:14, borenet wrote:
> On 2013/06/21 17:46:44, epoger wrote:
> > Patchset 2 adds the new missingExpectationsPath flag to GM
> 
> So, we put the actuals for GMs with no expectations in a different place from
> those with mismatches.  Is this so that we can upload them to different
places? 
> I think we want to upload them in either case.

As you'll see in https://codereview.chromium.org/16907006/ ('buildbots: only
store/upload GM results we haven't seen before, to save time'), my plan is to
store both of them in the SAME directory.

Another way to achieve this would be to change the meaning of --mismatchPath to
be: write out any files that either mismatch expectations, OR DON'T HAVE ANY. 
But it seemed safer/cleaner to add a new option, and allow the caller to put the
images wherever he wants them.

https://codereview.chromium.org/17552012/diff/4001/gm/tests/outputs/checksum-...
File gm/tests/outputs/checksum-based-filenames/output-expected/command_line
(right):

https://codereview.chromium.org/17552012/diff/4001/gm/tests/outputs/checksum-...
gm/tests/outputs/checksum-based-filenames/output-expected/command_line:1:
out/Debug/gm --verbose --writeChecksumBasedFilenames --match selftest1 --config
8888 565 -r gm/tests/inputs/json/different-pixels-no-hierarchy.json
--writeJsonSummaryPath
gm/tests/outputs/checksum-based-filenames/output-actual/json-summary.txt
--writePath gm/tests/outputs/checksum-based-filenames/output-actual/writePath
--mismatchPath
gm/tests/outputs/checksum-based-filenames/output-actual/mismatchPath
--missingExpectationsPath
gm/tests/outputs/checksum-based-filenames/output-actual/missingExpectationsPath
On 2013/06/21 18:01:14, borenet wrote:
> These expectations are getting really unwieldy.  Is there a good way to
> simplify?

I agree, especially all the "bogusfile"s to make sure the directories aren't
empty.

1. Do you have a particular simplifying suggestion?

2. Do you think it would be reasonable to commit this change as-is and simplify
the test later, so we can start saving time on our buildbots?

[borenet, 2013-06-21 18:14:09]

LGTM

https://codereview.chromium.org/17552012/diff/4001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/17552012/diff/4001/gm/gmmain.cpp#newcode196
gm/gmmain.cpp:196: fMismatchPath(NULL), fMissingExpectationsPath(NULL),
fTestsRun(0),
On 2013/06/21 18:06:46, epoger wrote:
> On 2013/06/21 18:01:14, borenet wrote:
> > On 2013/06/21 17:46:44, epoger wrote:
> > > Patchset 2 adds the new missingExpectationsPath flag to GM
> > 
> > So, we put the actuals for GMs with no expectations in a different place
from
> > those with mismatches.  Is this so that we can upload them to different
> places? 
> > I think we want to upload them in either case.
> 
> As you'll see in https://codereview.chromium.org/16907006/ ('buildbots: only
> store/upload GM results we haven't seen before, to save time'), my plan is to
> store both of them in the SAME directory.
> 
> Another way to achieve this would be to change the meaning of --mismatchPath
to
> be: write out any files that either mismatch expectations, OR DON'T HAVE ANY. 
> But it seemed safer/cleaner to add a new option, and allow the caller to put
the
> images wherever he wants them.

I think that's fine, I just can't think of a case where we'd actually put them
in different places.

https://codereview.chromium.org/17552012/diff/4001/gm/tests/outputs/checksum-...
File gm/tests/outputs/checksum-based-filenames/output-expected/command_line
(right):

https://codereview.chromium.org/17552012/diff/4001/gm/tests/outputs/checksum-...
gm/tests/outputs/checksum-based-filenames/output-expected/command_line:1:
out/Debug/gm --verbose --writeChecksumBasedFilenames --match selftest1 --config
8888 565 -r gm/tests/inputs/json/different-pixels-no-hierarchy.json
--writeJsonSummaryPath
gm/tests/outputs/checksum-based-filenames/output-actual/json-summary.txt
--writePath gm/tests/outputs/checksum-based-filenames/output-actual/writePath
--mismatchPath
gm/tests/outputs/checksum-based-filenames/output-actual/mismatchPath
--missingExpectationsPath
gm/tests/outputs/checksum-based-filenames/output-actual/missingExpectationsPath
On 2013/06/21 18:06:46, epoger wrote:
> On 2013/06/21 18:01:14, borenet wrote:
> > These expectations are getting really unwieldy.  Is there a good way to
> > simplify?
> 
> I agree, especially all the "bogusfile"s to make sure the directories aren't
> empty.
> 
> 1. Do you have a particular simplifying suggestion?
> 
> 2. Do you think it would be reasonable to commit this change as-is and
simplify
> the test later, so we can start saving time on our buildbots?

1. Maybe modify the test so that it can handle empty or nonexistent directories?
 FWIW I think "whitespace.txt" is a better name for an empty placeholder file. 
It makes it clear that nobody needs to open it and read it.  For changes which
affect the buildbot factory config test, I typically won't include the
expectations files in the review, or I'll only include one, unless there are
some significant changes.

2. Yes, that's fine.

[epoger, 2013-06-21 18:17:22]

Committed patchset #2 manually as r9725 (presubmit successful).

[epoger, 2013-06-21 18:21:06]

On 2013/06/21 18:14:09, borenet wrote:
> LGTM
> 
> https://codereview.chromium.org/17552012/diff/4001/gm/gmmain.cpp
> File gm/gmmain.cpp (right):
> 
> https://codereview.chromium.org/17552012/diff/4001/gm/gmmain.cpp#newcode196
> gm/gmmain.cpp:196: fMismatchPath(NULL), fMissingExpectationsPath(NULL),
> fTestsRun(0),
> On 2013/06/21 18:06:46, epoger wrote:
> > On 2013/06/21 18:01:14, borenet wrote:
> > > On 2013/06/21 17:46:44, epoger wrote:
> > > > Patchset 2 adds the new missingExpectationsPath flag to GM
> > > 
> > > So, we put the actuals for GMs with no expectations in a different place
> from
> > > those with mismatches.  Is this so that we can upload them to different
> > places? 
> > > I think we want to upload them in either case.
> > 
> > As you'll see in https://codereview.chromium.org/16907006/ ('buildbots: only
> > store/upload GM results we haven't seen before, to save time'), my plan is
to
> > store both of them in the SAME directory.
> > 
> > Another way to achieve this would be to change the meaning of --mismatchPath
> to
> > be: write out any files that either mismatch expectations, OR DON'T HAVE
ANY. 
> > But it seemed safer/cleaner to add a new option, and allow the caller to put
> the
> > images wherever he wants them.
> 
> I think that's fine, I just can't think of a case where we'd actually put them
> in different places.
> 
>
https://codereview.chromium.org/17552012/diff/4001/gm/tests/outputs/checksum-...
> File gm/tests/outputs/checksum-based-filenames/output-expected/command_line
> (right):
> 
>
https://codereview.chromium.org/17552012/diff/4001/gm/tests/outputs/checksum-...
> gm/tests/outputs/checksum-based-filenames/output-expected/command_line:1:
> out/Debug/gm --verbose --writeChecksumBasedFilenames --match selftest1
--config
> 8888 565 -r gm/tests/inputs/json/different-pixels-no-hierarchy.json
> --writeJsonSummaryPath
> gm/tests/outputs/checksum-based-filenames/output-actual/json-summary.txt
> --writePath gm/tests/outputs/checksum-based-filenames/output-actual/writePath
> --mismatchPath
> gm/tests/outputs/checksum-based-filenames/output-actual/mismatchPath
> --missingExpectationsPath
>
gm/tests/outputs/checksum-based-filenames/output-actual/missingExpectationsPath
> On 2013/06/21 18:06:46, epoger wrote:
> > On 2013/06/21 18:01:14, borenet wrote:
> > > These expectations are getting really unwieldy.  Is there a good way to
> > > simplify?
> > 
> > I agree, especially all the "bogusfile"s to make sure the directories aren't
> > empty.
> > 
> > 1. Do you have a particular simplifying suggestion?
> > 
> > 2. Do you think it would be reasonable to commit this change as-is and
> simplify
> > the test later, so we can start saving time on our buildbots?
> 
> 1. Maybe modify the test so that it can handle empty or nonexistent
directories?
>  FWIW I think "whitespace.txt" is a better name for an empty placeholder file.

> It makes it clear that nobody needs to open it and read it.  For changes which
> affect the buildbot factory config test, I typically won't include the
> expectations files in the review, or I'll only include one, unless there are
> some significant changes.
> 
> 2. Yes, that's fine.

Filed https://code.google.com/p/skia/issues/detail?id=1363 ('gm self-tests:
streamline storage/comparison of empty directories'), and made a note to myself
to look into it when I return from vacation.

Thanks.