rebaseline.py: if --tests is not specified, get test list from actual-results.json

rebaseline.py: if --tests is not specified, get test list from actual-results.json

R=scroggo@google.com

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

[epoger, 2013-06-04 15:54:50]

https://codereview.chromium.org/16093025/diff/1/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/16093025/diff/1/tools/rebaseline.py#newcode190
tools/rebaseline.py:190: parser.add_argument('--json_base_url',
Patchset 1 adds json_base_url argument (not used yet, though)

[epoger, 2013-06-04 16:01:22]

https://codereview.chromium.org/16093025/diff/1002/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/16093025/diff/1002/tools/rebaseline.py#newcod...
tools/rebaseline.py:173: for test in self._tests:
Patchset 2 loops over subdirs first, and then in the *inner* loop loops over
tests.  This will allow us to rebaseline a different set of tests for each
subdir, once we rely on actual-results.json to tell us which tests failed on
each platform.

https://codereview.chromium.org/16093025/diff/1002/tools/tests/rebaseline/sub...
File tools/tests/rebaseline/subset/output-expected/stdout (right):

https://codereview.chromium.org/16093025/diff/1002/tools/tests/rebaseline/sub...
tools/tests/rebaseline/subset/output-expected/stdout:24: # test1_565.png
Same operations, but now in a different order because we swapped the inner and
outer loops.

[epoger, 2013-06-04 18:52:23]

https://codereview.chromium.org/16093025/diff/5002/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/16093025/diff/5002/tools/rebaseline.py#newcod...
tools/rebaseline.py:223: if self._tests:
Patchset 3: If --tests has been specified, just use that list... but if not,
download the latest actual-results.json file for the given platform, and
rebaseline all tests marked as failing there.

The fact that the existing tests still pass indicates that the "--tests have
been specified" path still works as before.  I will add more self-tests to
exercise the JSON path...

[epoger, 2013-06-05 02:42:56]

https://codereview.chromium.org/16093025/diff/11008/tools/tests/rebaseline/us...
File
tools/tests/rebaseline/using-json/base-android-galaxy-nexus/Test-Android-GalaxyNexus-SGX540-Arm7-Debug/base-android-galaxy-nexus/actual-results.json
(right):

https://codereview.chromium.org/16093025/diff/11008/tools/tests/rebaseline/us...
tools/tests/rebaseline/using-json/base-android-galaxy-nexus/Test-Android-GalaxyNexus-SGX540-Arm7-Debug/base-android-galaxy-nexus/actual-results.json:2:
"actual-results" : {
The actual-results.json files in patchset 4 were made by copying in real ones
from production, and cutting out most of the results to keep them reasonably
sized.

https://codereview.chromium.org/16093025/diff/11008/tools/tests/rebaseline/us...
File tools/tests/rebaseline/using-json/output-expected/stdout (right):

https://codereview.chromium.org/16093025/diff/11008/tools/tests/rebaseline/us...
tools/tests/rebaseline/using-json/output-expected/stdout:9: #
imageblur_565.png_8888.png
These self-test results aren't quite right.  We have gotten the list of tests to
rebaseline from the JSON file as [u'imageblur_565.png', u'imageblur_8888.png',
u'shadertext3_8888.png'], and then multiplying those by all the config
combinations:

imageblur_565.png_565.png
imageblur_565.png_8888.png
imageblur_565.png_gpu.png
etc.

Instead, if we are getting the list of tests to rebaseline from the JSON file,
that list *includes* the configs we need to rebaseline.

https://codereview.chromium.org/16093025/diff/11008/tools/tests/run.sh
File tools/tests/run.sh (right):

https://codereview.chromium.org/16093025/diff/11008/tools/tests/run.sh#newcod...
tools/tests/run.sh:200: rebaseline_test "--json_base_url
file:$REBASELINE_TESTDIR/using-json --subdirs base-android-galaxy-nexus
base-shuttle-win7-intel-float" "$REBASELINE_TESTDIR/using-json"
Patchset 4 adds a test case that gets the list of tests to rebaseline from
actual-results.json files.

[epoger, 2013-06-05 03:16:20]

Ready to review at patchset 5.

I've nominated Leon as the official reviewer for this, but if someone else
wishes to pass judgment please feel free.

https://codereview.chromium.org/16093025/diff/20001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/16093025/diff/20001/tools/rebaseline.py#newcode1
tools/rebaseline.py:1: #!/usr/bin/python
Toplevel comment: This change is part of "Step 1" in
https://goto.google.com/ChecksumTransitionDetail .

With this CL we can get the list of images that need rebaselining from
actual-results.json, and the typical rebaselining use case becomes simply this:

python .../tools/rebaseline.py

In an upcoming CL, I will complete "Step 1" by using actual-results.json to also
give me the actual image checksums, which tells me where they might be available
in Google Storage.

https://codereview.chromium.org/16093025/diff/20001/tools/rebaseline.py#newco...
tools/rebaseline.py:244: self._RebaselineOneFile(expectations_subdir=subdir,
Patchset 5 fixes the JSON case: when rebaselining based on actual-results.json,
we don't need to multiply the test cases by config types.  The JSON file gives
us the full set of files that need rebaselining.

https://codereview.chromium.org/16093025/diff/20001/tools/tests/rebaseline/us...
File tools/tests/rebaseline/using-json/output-expected/stdout (right):

https://codereview.chromium.org/16093025/diff/20001/tools/tests/rebaseline/us...
tools/tests/rebaseline/using-json/output-expected/stdout:4: curl --fail --silent
http://skia-autogen.googlecode.com/svn/gm-actual/base-android-galaxy-nexus/Te...
--output base-android-galaxy-nexus/.temp-imageblur_565.png
From manual inspection, the new results look right to me.

[epoger, 2013-06-05 03:41:00]

https://codereview.chromium.org/16093025/diff/26001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/16093025/diff/26001/tools/rebaseline.py#newco...
tools/rebaseline.py:84: if configs and not tests:
Patchset 6 throws an error message if --configs is specified without --tests...

https://codereview.chromium.org/16093025/diff/26001/tools/rebaseline.py#newco...
tools/rebaseline.py:154: print ('# Getting files to rebaseline from JSON summary
URL %s ...'
... and is more consistent about "files" vs "tests".  (each "file" is a
test+config combination)

[epoger, 2013-06-05 15:02:31]

Still awaiting review, just adding one more note for discussion...

https://codereview.chromium.org/16093025/diff/26001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/16093025/diff/26001/tools/rebaseline.py#newco...
tools/rebaseline.py:76: #  configs: which configs to run for each test; this
should only be
I just spoke to Rob about the behavior when --tests is specified vs when --tests
is not specified, and here's my plan for an upcoming CL:

Keep the --tests and --configs options, but if they are specified, they are
FILTERS (you can use them to limit which tests are rebaselined, but the script
will still only rebaseline tests that were reported as failures in the
actual-results.json file)

I will also add a flag ("--add_new" ?) that will download baselines for tests in
the "no-comparison" section, as well as the "failed" section that we ALWAYS
rebaseline.

Any thoughts on those plans?

For now, let's see if we can get agreement on whether this is a reasonable
interim CL. :-)

[scroggo, 2013-06-05 15:11:14]

lgtm

https://codereview.chromium.org/16093025/diff/26001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/16093025/diff/26001/tools/rebaseline.py#newco...
tools/rebaseline.py:76: #  configs: which configs to run for each test; this
should only be
On 2013/06/05 15:02:31, epoger wrote:
> I just spoke to Rob about the behavior when --tests is specified vs when
--tests
> is not specified, and here's my plan for an upcoming CL:
> 
> Keep the --tests and --configs options, but if they are specified, they are
> FILTERS (you can use them to limit which tests are rebaselined, but the script
> will still only rebaseline tests that were reported as failures in the
> actual-results.json file)
> 
> I will also add a flag ("--add_new" ?) that will download baselines for tests
in
> the "no-comparison" section, as well as the "failed" section that we ALWAYS
> rebaseline.
> 
> Any thoughts on those plans?
> 
> For now, let's see if we can get agreement on whether this is a reasonable
> interim CL. :-)

--add_new sounds fine to me.

[epoger, 2013-06-05 15:43:43]

Committed patchset #6 manually as r9443 (presubmit successful).

[Stephen White, 2013-06-05 16:44:20]

Some post-commit comments.

https://codereview.chromium.org/16093025/diff/26001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/16093025/diff/26001/tools/rebaseline.py#newco...
tools/rebaseline.py:30: sys.path.append(GM_DIRECTORY)
A comment about why this is necessary would be helpful.

Also, if we're assuming (or checking for) the gm directory, should we also check
that we're in (or near) the gm-expected directory? Do we now assume that this
script will always be run from a whole-enchilada (root) skia checkout?

https://codereview.chromium.org/16093025/diff/26001/tools/rebaseline.py#newco...
tools/rebaseline.py:76: #  configs: which configs to run for each test; this
should only be
On 2013/06/05 15:02:31, epoger wrote:
> I just spoke to Rob about the behavior when --tests is specified vs when
--tests
> is not specified, and here's my plan for an upcoming CL:
> 
> Keep the --tests and --configs options, but if they are specified, they are
> FILTERS (you can use them to limit which tests are rebaselined, but the script
> will still only rebaseline tests that were reported as failures in the
> actual-results.json file)
> 
> I will also add a flag ("--add_new" ?) that will download baselines for tests
in
> the "no-comparison" section, as well as the "failed" section that we ALWAYS
> rebaseline.

My OCD nit: please don't mix underscores and dashes.  --add-new, or perhaps
--new-test-results perhaps (this is what the blink tools use).

> Any thoughts on those plans?
> 
> For now, let's see if we can get agreement on whether this is a reasonable
> interim CL. :-)

[epoger, 2013-06-05 17:46:39]

Thanks for the comments, Stephen.  Congratulations- you've just volunteered to
be the reviewer for my next CL!

https://codereview.chromium.org/16093025/diff/26001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/16093025/diff/26001/tools/rebaseline.py#newco...
tools/rebaseline.py:30: sys.path.append(GM_DIRECTORY)
On 2013/06/05 16:44:20, Stephen White wrote:
> A comment about why this is necessary would be helpful.
> 
> Also, if we're assuming (or checking for) the gm directory, should we also
check
> that we're in (or near) the gm-expected directory? Do we now assume that this
> script will always be run from a whole-enchilada (root) skia checkout?

I'll add a comment in an upcoming CL.

This *does* now assume that the "gm" directory has been checked out as a sibling
of the "tools" directory (which is the case if "trunk" has been checked out as a
unit).

It does NOT assume that we have a whole-enchilada checkout.

If you think it's cleaner, I could move the rebaseline.py tool from "tools" to
"gm"... in a way that would be cleaner, because it is the GM rebaselining tool,
after all... what do you think?  Then we wouldn't need this cross-directory
import at all.  Or I'm open to other suggestions...

https://codereview.chromium.org/16093025/diff/26001/tools/rebaseline.py#newco...
tools/rebaseline.py:76: #  configs: which configs to run for each test; this
should only be
On 2013/06/05 16:44:20, Stephen White wrote:
> On 2013/06/05 15:02:31, epoger wrote:
> > I just spoke to Rob about the behavior when --tests is specified vs when
> --tests
> > is not specified, and here's my plan for an upcoming CL:
> > 
> > Keep the --tests and --configs options, but if they are specified, they are
> > FILTERS (you can use them to limit which tests are rebaselined, but the
script
> > will still only rebaseline tests that were reported as failures in the
> > actual-results.json file)
> > 
> > I will also add a flag ("--add_new" ?) that will download baselines for
tests
> in
> > the "no-comparison" section, as well as the "failed" section that we ALWAYS
> > rebaseline.
> 
> My OCD nit: please don't mix underscores and dashes.  --add-new, or perhaps
> --new-test-results perhaps (this is what the blink tools use).

I don't think that's a nit at all.  I'll make it --add-new , and I will change
--dry_run to --dry-run.

[Stephen White, 2013-06-05 20:01:45]

https://codereview.chromium.org/16093025/diff/26001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/16093025/diff/26001/tools/rebaseline.py#newco...
tools/rebaseline.py:30: sys.path.append(GM_DIRECTORY)
On 2013/06/05 17:46:39, epoger wrote:
> On 2013/06/05 16:44:20, Stephen White wrote:
> > A comment about why this is necessary would be helpful.
> > 
> > Also, if we're assuming (or checking for) the gm directory, should we also
> check
> > that we're in (or near) the gm-expected directory? Do we now assume that
this
> > script will always be run from a whole-enchilada (root) skia checkout?
> 
> I'll add a comment in an upcoming CL.
> 
> This *does* now assume that the "gm" directory has been checked out as a
sibling
> of the "tools" directory (which is the case if "trunk" has been checked out as
a
> unit).

OK, I think that's a fair assumption to make: I don't think there are any useful
workflows that check out "gm" without checking out "tools".

(I mostly use a trunk checkout for day-to-day work, and a whole-enchilada (skia
root) checkout for rebaselining. If updating the whole-enchilada checkout is too
slow (e.g., on Windows), I also sometimes use a gm-expected only checkout, but
in that case I'm running rebaseline.py from my trunk checkout anyway, so gm
should be a sibling of tools regardless.)

All that to say, I'd say leave rebaseline.py where it is.

> It does NOT assume that we have a whole-enchilada checkout.
> 
> If you think it's cleaner, I could move the rebaseline.py tool from "tools" to
> "gm"... in a way that would be cleaner, because it is the GM rebaselining
tool,
> after all... what do you think?  Then we wouldn't need this cross-directory
> import at all.  Or I'm open to other suggestions...