rebaseline.py : add self-tests

rebaseline.py : add self-tests

R=senorblanco@chromium.org

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

[epoger, 2013-05-30 19:44:12]

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

https://codereview.chromium.org/15660014/diff/1/tools/rebaseline.py#newcode91
tools/rebaseline.py:91: cmd = [ 'curl', '--fail', '--silent', url, '--output',
outfilename ]
Patchset 1 changes the "curl" command so that a temporary file is no longer
needed... this is necessary so that our dry_run output is deterministic.

Validated that the tool still works properly.

[epoger, 2013-05-30 19:45:53]

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

https://codereview.chromium.org/15660014/diff/4001/tools/tests/run.sh#newcode133
tools/tests/run.sh:133: # Run skdiff tests...
Patchset 2 rearranges some sections within run.sh before adding any
rebaseline.py self-tests to it.

[epoger, 2013-05-30 20:25:17]

Ready for review at patchset 3...

https://codereview.chromium.org/15660014/diff/8001/tools/tests/rebaseline.sh
File tools/tests/rebaseline.sh (right):

https://codereview.chromium.org/15660014/diff/8001/tools/tests/rebaseline.sh#...
tools/tests/rebaseline.sh:64: TOOLS="skdiff benchgraphs rebaseline"
Confusingly, this tells rebaseline.sh (the script for rebaselining tool
self-tests) to update expected test results for rebaseline.py (the tool for GM
image rebaselining)

[Stephen White, 2013-05-30 20:34:16]

https://codereview.chromium.org/15660014/diff/8001/tools/rebaseline.py
File tools/rebaseline.py (left):

https://codereview.chromium.org/15660014/diff/8001/tools/rebaseline.py#oldcode95
tools/rebaseline.py:95: temp = tempfile.NamedTemporaryFile()
IIRC, the reason I used a temp file is that if the fetch fails, it'll clobber
the existing result with a 0-length file. I don't think we should do that.

https://codereview.chromium.org/15660014/diff/8001/tools/tests/rebaseline.sh
File tools/tests/rebaseline.sh (right):

https://codereview.chromium.org/15660014/diff/8001/tools/tests/rebaseline.sh#...
tools/tests/rebaseline.sh:64: TOOLS="skdiff benchgraphs rebaseline"
On 2013/05/30 20:25:17, epoger wrote:
> Confusingly, this tells rebaseline.sh (the script for rebaselining tool
> self-tests) to update expected test results for rebaseline.py (the tool for GM
> image rebaselining)

We must go deeper..

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

https://codereview.chromium.org/15660014/diff/8001/tools/tests/rebaseline/sub...
tools/tests/rebaseline/subset/output-expected/stdout:36: svn propset --quiet
svn:mime-type image/png base-shuttle-win7-intel-float/test2_8888.png
Hmm.. it does seem like a pretty fragile test, in that we'll have to update this
every time there's a new config or the bots change names, or we change the
output. But I can't think of anything better, so this'll have to do.

[epoger, 2013-05-31 03:54:06]

https://codereview.chromium.org/15660014/diff/8001/tools/rebaseline.py
File tools/rebaseline.py (left):

https://codereview.chromium.org/15660014/diff/8001/tools/rebaseline.py#oldcode95
tools/rebaseline.py:95: temp = tempfile.NamedTemporaryFile()
On 2013/05/30 20:34:16, Stephen White wrote:
> IIRC, the reason I used a temp file is that if the fetch fails, it'll clobber
> the existing result with a 0-length file. I don't think we should do that.

Thanks for the explanation... although, if you run curl with --output instead of
piping to stdout, I don't know if that would still happen.  At any rate,
download-and-then-rename seems reasonable.

I have implemented a version of that with deterministic temporary filenames, so
that --dry_run works for the self-test.

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

https://codereview.chromium.org/15660014/diff/8001/tools/tests/rebaseline/sub...
tools/tests/rebaseline/subset/output-expected/stdout:36: svn propset --quiet
svn:mime-type image/png base-shuttle-win7-intel-float/test2_8888.png
On 2013/05/30 20:34:16, Stephen White wrote:
> Hmm.. it does seem like a pretty fragile test, in that we'll have to update
this
> every time there's a new config or the bots change names, or we change the
> output. But I can't think of anything better, so this'll have to do.

I'm with you.  If we think of anything clever, we can change this anytime.  For
now, at least rebaseline.sh makes it easy to update the expected results.

https://codereview.chromium.org/15660014/diff/14001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/15660014/diff/14001/tools/rebaseline.py#newco...
tools/rebaseline.py:88: raise CommandFailedException('error running command: ' +
It seems to me that the common case ought to be: if a shell command fails, error
out.

If certain cases want to catch the shell command failure and move on, that's
fine... hence the custom CommandFailedException class, which makes sure we're
not catching terrible low-level Exceptions (AssertionError, SyntaxError, etc.)

https://codereview.chromium.org/15660014/diff/14001/tools/rebaseline.py#newco...
tools/rebaseline.py:92: def _DownloadFile(self, source_url, dest_filename):
Extracted the "curl" call into a new _DownloadFile() method, to make it easier
to use standard Python HTTP library calls instead of Unix shell commands later
on.  (See TODO below)

https://codereview.chromium.org/15660014/diff/14001/tools/rebaseline.py#newco...
tools/rebaseline.py:113: # TODO(senorblanco): Why is this not treated as a
serious failure?
Stephen, any thoughts on this question?

https://codereview.chromium.org/15660014/diff/14001/tools/tests/rebaseline/al...
File tools/tests/rebaseline/all/output-expected/stdout (right):

https://codereview.chromium.org/15660014/diff/14001/tools/tests/rebaseline/al...
tools/tests/rebaseline/all/output-expected/stdout:4: mv
base-android-galaxy-nexus/.temp-test1_565.png
base-android-galaxy-nexus/test1_565.png
See the new "curl"/"mv" combos, instead of the old just "curl".

I have confirmed, via manual testing, that the results of a real run are
unchanged.

[epoger, 2013-06-02 21:24:41]

Stephen- please re-review...

[Stephen White, 2013-06-03 14:09:30]

LGTM

https://codereview.chromium.org/15660014/diff/14001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/15660014/diff/14001/tools/rebaseline.py#newco...
tools/rebaseline.py:113: # TODO(senorblanco): Why is this not treated as a
serious failure?
On 2013/05/31 03:54:06, epoger wrote:
> Stephen, any thoughts on this question?

I think it's because not all platforms generate all configs (e.g., Android
doesn't generate pdf). We could (and should) probably tweak the list of configs
in this tool to reflect what the bots actually generate.

I don't think we should abort, though, since otherwise the tool will become
useless every time someone tweaks the configs on the bots without updating this
script.

[epoger, 2013-06-04 14:58:53]

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