svndiff.py: add ability to compare before-and-after JSON files, not just raw images

svndiff.py: add ability to compare before-and-after JSON files, not just raw images

This should complete step 3 of https://goto.google.com/ChecksumTransitionDetail !

R=borenet@google.com

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

[epoger, 2013-07-12 18:55:00]

Ready for review at patchset 1!

https://codereview.chromium.org/19112002/diff/1/tools/svndiff.py
File tools/svndiff.py (right):

https://codereview.chromium.org/19112002/diff/1/tools/svndiff.py#newcode1
tools/svndiff.py:1: #!/usr/bin/python
Tested as follows:

EXPECTATIONS_DIR=experimental/json-expectations
svn revert -R $EXPECTATIONS_DIR
svn stat $EXPECTATIONS_DIR
tools/rebaseline.py --expectations-root $EXPECTATIONS_DIR \
 && svn stat $EXPECTATIONS_DIR

TMPDIR=/tmp/svndiff
rm -rf $TMPDIR && \
  mkdir -p $TMPDIR && \
  tools/svndiff.py --dest-dir $TMPDIR --source-dir $EXPECTATIONS_DIR

... and viewing the resulting diffs in a browser.  It works...

[epoger, 2013-07-12 18:59:06]

P.S. I guess we'll need a git equivalent of this, too.  It should be able to
share most of the code.  I'd like to commit the svn one first, if that's ok.

[borenet, 2013-07-12 19:48:04]

https://codereview.chromium.org/19112002/diff/1/tools/jsondiff.py
File tools/jsondiff.py (right):

https://codereview.chromium.org/19112002/diff/1/tools/jsondiff.py#newcode162
tools/jsondiff.py:162: if __name__ == '__main__':
I'd prefer having most of this stuff inside of a Main() function.

https://codereview.chromium.org/19112002/diff/1/tools/svndiff.py
File tools/svndiff.py (right):

https://codereview.chromium.org/19112002/diff/1/tools/svndiff.py#newcode55
tools/svndiff.py:55: TESTNAME_PATTERN = re.compile('(\S+)_(\S+).png')
This is now listed in three places.  Not sure how bad that is.

https://codereview.chromium.org/19112002/diff/1/tools/svndiff.py#newcode99
tools/svndiff.py:99: writer = open(dest_path, 'w')
You didn't need to open in binary mode for this to work?

https://codereview.chromium.org/19112002/diff/1/tools/svndiff.py#newcode111
tools/svndiff.py:111: """
Can you add a TODO to consolidate these things?

[epoger, 2013-07-16 17:29:55]

Thanks, Eric, definitely a couple of good catches in there!  Please see patchset
2 (patchset 3 just syncs to head, no surprises there)

https://codereview.chromium.org/19112002/diff/1/tools/jsondiff.py
File tools/jsondiff.py (right):

https://codereview.chromium.org/19112002/diff/1/tools/jsondiff.py#newcode162
tools/jsondiff.py:162: if __name__ == '__main__':
On 2013/07/12 19:48:04, borenet wrote:
> I'd prefer having most of this stuff inside of a Main() function.

Done.

https://codereview.chromium.org/19112002/diff/1/tools/svndiff.py
File tools/svndiff.py (right):

https://codereview.chromium.org/19112002/diff/1/tools/svndiff.py#newcode55
tools/svndiff.py:55: TESTNAME_PATTERN = re.compile('(\S+)_(\S+).png')
On 2013/07/12 19:48:04, borenet wrote:
> This is now listed in three places.  Not sure how bad that is.

Not so hot, thanks for calling it to my attention.  Moved the definition to
gm_json.py, and the 3 places all share the reference.

https://codereview.chromium.org/19112002/diff/1/tools/svndiff.py#newcode99
tools/svndiff.py:99: writer = open(dest_path, 'w')
On 2013/07/12 19:48:04, borenet wrote:
> You didn't need to open in binary mode for this to work?

Good catch!  I was testing it on Linux, where it doesn't matter... but it DOES
matter on Windows.

https://codereview.chromium.org/19112002/diff/1/tools/svndiff.py#newcode111
tools/svndiff.py:111: """
On 2013/07/12 19:48:04, borenet wrote:
> Can you add a TODO to consolidate these things?

Consolidate which things?  Do you mean TESTNAME_PATTERN?  'cause that's
consolidated, now...

[borenet, 2013-07-16 17:42:13]

https://codereview.chromium.org/19112002/diff/1/tools/svndiff.py
File tools/svndiff.py (right):

https://codereview.chromium.org/19112002/diff/1/tools/svndiff.py#newcode111
tools/svndiff.py:111: """
On 2013/07/16 17:29:55, epoger wrote:
> On 2013/07/12 19:48:04, borenet wrote:
> > Can you add a TODO to consolidate these things?
> 
> Consolidate which things?  Do you mean TESTNAME_PATTERN?  'cause that's
> consolidated, now...

I was thinking maybe a BuildGMActualGSURLFromImageName function, or something
like it, since this URL building technique is used in multiple places.

https://codereview.chromium.org/19112002/diff/7001/gm/gm_json.py
File gm/gm_json.py (right):

https://codereview.chromium.org/19112002/diff/7001/gm/gm_json.py#newcode53
gm/gm_json.py:53: IMAGE_FILENAME_PATTERN = '(\S+)_(\S+).png'  # matches
(testname, config)
Any reason we can't just make this:
IMAGE_FILENAME_RE = re.compile('(\S+)_(\S+).png')
?

[epoger, 2013-07-16 18:22:48]

PTAL at patchset 4

https://codereview.chromium.org/19112002/diff/1/tools/svndiff.py
File tools/svndiff.py (right):

https://codereview.chromium.org/19112002/diff/1/tools/svndiff.py#newcode111
tools/svndiff.py:111: """
On 2013/07/16 17:42:13, borenet wrote:
> On 2013/07/16 17:29:55, epoger wrote:
> > On 2013/07/12 19:48:04, borenet wrote:
> > > Can you add a TODO to consolidate these things?
> > 
> > Consolidate which things?  Do you mean TESTNAME_PATTERN?  'cause that's
> > consolidated, now...
> 
> I was thinking maybe a BuildGMActualGSURLFromImageName function, or something
> like it, since this URL building technique is used in multiple places.

Sounds good... I created a new gm_json.CreateGmActualUrl(), and we now call it
from the appropriate places.

https://codereview.chromium.org/19112002/diff/7001/gm/gm_json.py
File gm/gm_json.py (right):

https://codereview.chromium.org/19112002/diff/7001/gm/gm_json.py#newcode53
gm/gm_json.py:53: IMAGE_FILENAME_PATTERN = '(\S+)_(\S+).png'  # matches
(testname, config)
On 2013/07/16 17:42:13, borenet wrote:
> Any reason we can't just make this:
> IMAGE_FILENAME_RE = re.compile('(\S+)_(\S+).png')
> ?

I think it's better to keep it as a pattern string for the canonical definition,
so that the string can be modified (e.g. assembled into a longer regular
expression) if desired.  Can't go back the other way, from regexp to string...

https://codereview.chromium.org/19112002/diff/12002/tools/verify_images_for_g...
File tools/verify_images_for_gm_results.py (right):

https://codereview.chromium.org/19112002/diff/12002/tools/verify_images_for_g...
tools/verify_images_for_gm_results.py:19: # TODO(borenet): Replace some/all of
these with constants from gm/gm_json.py
For now, I just put a TODO here, so as not to expand this CL even further...

[borenet, 2013-07-16 18:31:03]

LGTM with a few nits

https://codereview.chromium.org/19112002/diff/12002/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/19112002/diff/12002/tools/rebaseline.py#newco...
tools/rebaseline.py:188: (test, config) =
self._image_filename_re.match(image_name).groups()
nit: 80 chars

Also, why are we using four space indents in this file?

https://codereview.chromium.org/19112002/diff/12002/tools/svndiff.py
File tools/svndiff.py (right):

https://codereview.chromium.org/19112002/diff/12002/tools/svndiff.py#newcode100
tools/svndiff.py:100: 
Nit: spacing

https://codereview.chromium.org/19112002/diff/12002/tools/svndiff.py#newcode114
tools/svndiff.py:114: 
Spacing

https://codereview.chromium.org/19112002/diff/12002/tools/svndiff.py#newcode149
tools/svndiff.py:149: 
Spacing

[epoger, 2013-07-16 18:42:32]

https://codereview.chromium.org/19112002/diff/12002/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/19112002/diff/12002/tools/rebaseline.py#newco...
tools/rebaseline.py:188: (test, config) =
self._image_filename_re.match(image_name).groups()
On 2013/07/16 18:31:03, borenet wrote:
> nit: 80 chars

Thanks to Python's whitespace-sensitivity, I couldn't think of a way to break
this line that would work.  Can you?

> 
> Also, why are we using four space indents in this file?

I guess because my emacs configuration was doing that by default.

If it's OK with you, I would like to commit this CL as-is and fix the spacing
separately.  I have added TODOs in each of these files that are 4-space
indented.

[borenet, 2013-07-16 18:48:37]

https://codereview.chromium.org/19112002/diff/12002/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/19112002/diff/12002/tools/rebaseline.py#newco...
tools/rebaseline.py:188: (test, config) =
self._image_filename_re.match(image_name).groups()
On 2013/07/16 18:42:32, epoger wrote:
> On 2013/07/16 18:31:03, borenet wrote:
> > nit: 80 chars
> 
> Thanks to Python's whitespace-sensitivity, I couldn't think of a way to break
> this line that would work.  Can you?
> 

It's ugly, but what about:
(test, config) = \
    self._image_filename_re.match(image_name).groups()
?

> > 
> > Also, why are we using four space indents in this file?
> 
> I guess because my emacs configuration was doing that by default.
> 
> If it's OK with you, I would like to commit this CL as-is and fix the spacing
> separately.  I have added TODOs in each of these files that are 4-space
> indented.

SGTM

[epoger, 2013-07-16 18:54:52]

On 2013/07/16 18:48:37, borenet wrote:
> It's ugly, but what about:
> (test, config) = \
>     self._image_filename_re.match(image_name).groups()
> ?

Yeah, that works.

> > If it's OK with you, I would like to commit this CL as-is and fix the
spacing
> > separately.  I have added TODOs in each of these files that are 4-space
> > indented.
> 
> SGTM

Thanks!

[epoger, 2013-07-16 18:56:35]

Committed patchset #7 manually as r10113 (presubmit successful).