Add ability to output ImageBaseGSUrl to render_picture and use in rebaseline server

Add ability to output ImageBaseGSUrl to render_picture and use in rebaseline server.

BUG=skia:2230
Committed: https://skia.googlesource.com/skia/+/2529f2e72cddf87904c8ad4b613942cbef802cfb

[epoger, 2014-08-15 17:52:50]

https://codereview.chromium.org/479613002/diff/20001/gm/gm_json.py
File gm/gm_json.py (right):

https://codereview.chromium.org/479613002/diff/20001/gm/gm_json.py#newcode96
gm/gm_json.py:96: JSONKEY_GOOGLE_STORAGE_BUCKET = 'gs-bucket'
I think this CL is fine *except* that I think it would be better to specify it
as IMAGE_BASE_GS_URL ("gs://bucket" or "gs://bucket/path") for flexibility.

[rmistry, 2014-08-21 13:08:29]

https://codereview.chromium.org/479613002/diff/20001/gm/gm_json.py
File gm/gm_json.py (right):

https://codereview.chromium.org/479613002/diff/20001/gm/gm_json.py#newcode96
gm/gm_json.py:96: JSONKEY_GOOGLE_STORAGE_BUCKET = 'gs-bucket'
On 2014/08/15 17:52:50, epoger wrote:
> I think this CL is fine *except* that I think it would be better to specify it
> as IMAGE_BASE_GS_URL ("gs://bucket" or "gs://bucket/path") for flexibility.

Done.

[rmistry, 2014-08-21 15:04:47]

Patchset6 overrides DEFAULT_IMAGE_BASE_GS_URL.
PTAL

[epoger, 2014-08-21 16:15:22]

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
File gm/rebaseline_server/imagepair_test.py (right):

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
gm/rebaseline_server/imagepair_test.py:79: # use alternate imageB base url:
AFAICT, the different settings for this parameter do not affect any of the
unittest results.  So are they being exercised?

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
gm/rebaseline_server/imagepair_test.py:80: True,
Assuming that this parameter is really being exercised meaningfully...

I think it would be more straightforward just to specify the base URLs within
each test case setup.  For each one, either set it to

(IMG_URL_BASE, IMG_URL_BASE)
or
(IMG_URL_BASE, ALT_IMG_URL_BASE)


If you really prefer the boolean, you could do it this way to make it more
self-documenting:

DO_USE_ALTERNATE_IMAGEB_BASE_URL = True
DONT_USE_ALTERNATE_IMAGEB_BASE_URL = False

and then just use those constants.

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
File gm/rebaseline_server/imagepairset.py (right):

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
gm/rebaseline_server/imagepairset.py:77: if(image_pair.imageA_base_url !=
self._imageA_base_url):
So, imageA_base_url has to be the same for all images in the set (and same for
imageB_base_url).  Right?

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
gm/rebaseline_server/imagepairset.py:79: image_pair.imageA_base_url,
self._imageA_base_url))
Can you please add a unittest that demonstrates these parameters getting put
into the output JSON?  Like adjusting a test so that
https://skia.googlesource.com/skia/+/53fecfb15d254397ab03032b888daa9d15c487b6...
would have different baseUrls for imageSet A and imageSet B.

[rmistry, 2014-08-21 18:15:07]

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
File gm/rebaseline_server/imagepair_test.py (right):

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
gm/rebaseline_server/imagepair_test.py:79: # use alternate imageB base url:
On 2014/08/21 16:15:22, epoger wrote:
> AFAICT, the different settings for this parameter do not affect any of the
> unittest results.  So are they being exercised?

This parameter was read and exercised at line 209. Is that what you meant?

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
gm/rebaseline_server/imagepair_test.py:80: True,
On 2014/08/21 16:15:22, epoger wrote:
> Assuming that this parameter is really being exercised meaningfully...
> 
> I think it would be more straightforward just to specify the base URLs within
> each test case setup.  For each one, either set it to
> 
> (IMG_URL_BASE, IMG_URL_BASE)
> or
> (IMG_URL_BASE, ALT_IMG_URL_BASE)
> 
> 
> If you really prefer the boolean, you could do it this way to make it more
> self-documenting:
> 
> DO_USE_ALTERNATE_IMAGEB_BASE_URL = True
> DONT_USE_ALTERNATE_IMAGEB_BASE_URL = False
> 
> and then just use those constants.

Used the tuple suggestion. Done.

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
File gm/rebaseline_server/imagepairset.py (right):

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
gm/rebaseline_server/imagepairset.py:77: if(image_pair.imageA_base_url !=
self._imageA_base_url):
On 2014/08/21 16:15:22, epoger wrote:
> So, imageA_base_url has to be the same for all images in the set (and same for
> imageB_base_url).  Right?

Right, does this sound like a correct assumption?

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
gm/rebaseline_server/imagepairset.py:79: image_pair.imageA_base_url,
self._imageA_base_url))
On 2014/08/21 16:15:22, epoger wrote:
> Can you please add a unittest that demonstrates these parameters getting put
> into the output JSON?  Like adjusting a test so that
>
https://skia.googlesource.com/skia/+/53fecfb15d254397ab03032b888daa9d15c487b6...
> would have different baseUrls for imageSet A and imageSet B.

Attempted to do so in compare_rendered_pictures_test.py

[epoger, 2014-08-21 18:25:06]

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
File gm/rebaseline_server/imagepair_test.py (right):

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
gm/rebaseline_server/imagepair_test.py:79: # use alternate imageB base url:
On 2014/08/21 18:15:06, rmistry wrote:
> On 2014/08/21 16:15:22, epoger wrote:
> > AFAICT, the different settings for this parameter do not affect any of the
> > unittest results.  So are they being exercised?
> 
> This parameter was read and exercised at line 209. Is that what you meant?

I see that the parameter was read, but the parameter's value doesn't modify the
unittest results (the "expected output" above).  So we know it didn't cause an
assertion to fail, but we don't see that it is functioning correctly...

It may be that it just doesn't make sense to test this parameter within this
file, because it only shows up when you dump out an ImagePairSet instead of just
one ImagePair???

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
File gm/rebaseline_server/imagepairset.py (right):

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
gm/rebaseline_server/imagepairset.py:77: if(image_pair.imageA_base_url !=
self._imageA_base_url):
On 2014/08/21 18:15:07, rmistry wrote:
> On 2014/08/21 16:15:22, epoger wrote:
> > So, imageA_base_url has to be the same for all images in the set (and same
for
> > imageB_base_url).  Right?
> 
> Right, does this sound like a correct assumption?

I think so.  I wanted to see where these values come out in the generated JSON,
preferably in a some unittest expectations like
https://skia.googlesource.com/skia/+/53fecfb15d254397ab03032b888daa9d15c487b6...
.

https://codereview.chromium.org/479613002/diff/180001/gm/rebaseline_server/co...
File gm/rebaseline_server/compare_rendered_pictures_test.py (right):

https://codereview.chromium.org/479613002/diff/180001/gm/rebaseline_server/co...
gm/rebaseline_server/compare_rendered_pictures_test.py:131:
'compare_rendered_pictures.json'))
Where is the expectation for this test?  If you don't add a file within
gm/rebaseline_server/testdata/outputs/expected , the file you generate here
won't be compared against anything.

Personally, rather than adding a new test for this, I would recommend modifying
the existing test_endToEnd() test (add specification of image_base_gs_url to
that test).  Then, the changes to the expected results file will show us exactly
what this change will do in practice.

[rmistry, 2014-08-21 19:07:01]

Added the expected file, but note that the test does not seem to be asserting
against it due to the issue I mentioned in the email thread with you and
stephana@

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
File gm/rebaseline_server/imagepair_test.py (right):

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
gm/rebaseline_server/imagepair_test.py:79: # use alternate imageB base url:
On 2014/08/21 18:25:06, epoger wrote:
> On 2014/08/21 18:15:06, rmistry wrote:
> > On 2014/08/21 16:15:22, epoger wrote:
> > > AFAICT, the different settings for this parameter do not affect any of the
> > > unittest results.  So are they being exercised?
> > 
> > This parameter was read and exercised at line 209. Is that what you meant?
> 
> I see that the parameter was read, but the parameter's value doesn't modify
the
> unittest results (the "expected output" above).  So we know it didn't cause an
> assertion to fail, but we don't see that it is functioning correctly...
> 
> It may be that it just doesn't make sense to test this parameter within this
> file, because it only shows up when you dump out an ImagePairSet instead of
just
> one ImagePair???

The parameter's value did modify the results of this test. Setting it to True in
any of the selftests here (except the first one) fails the test because the
image does not exist in the new location. It only exists for selftest 1.

The failure if you change any of the other values is:

FAIL: test_endToEnd (__main__.ImagePairTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "imagepair_test.py", line 217, in test_endToEnd
    self.assertEqual(image_pair.as_dict(), selftest[4])
AssertionError: {'isDifferent': True, 'imageBUrl':
'arcofzorro/13786535001616823825.png', 'image [truncated]... != {'isDifferent':
True, 'imageBUrl': 'arcofzorro/13786535001616823825.png', 'image [truncated]...
+ {'differenceData': {'diffUrl':
'arcofzorro_16206093933823793653_png_png-vs-arcofzorro_13786535001616823825_png_png.png',
+                     'maxDiffPerChannel': [255, 255, 247],
+                     'numDifferingPixels': 662,
+                     'percentDifferingPixels': 0.0662,
+                     'perceptualDifference': 0.06620300000000157,
+                     'whiteDiffUrl':
'arcofzorro_16206093933823793653_png_png-vs-arcofzorro_13786535001616823825_png_png.png'},
- {'imageAUrl': 'arcofzorro/16206093933823793653.png',
? ^

+  'imageAUrl': 'arcofzorro/16206093933823793653.png',
? ^

   'imageBUrl': 'arcofzorro/13786535001616823825.png',
   'isDifferent': True}

----------------------------------------------------------------------
Ran 1 test in 1.006s

FAILED (failures=1)



The expected output above was not modified because it never contained the base
url, I'm not sure if it needs to, please let me know if I missed something here.

https://codereview.chromium.org/479613002/diff/180001/gm/rebaseline_server/co...
File gm/rebaseline_server/compare_rendered_pictures_test.py (right):

https://codereview.chromium.org/479613002/diff/180001/gm/rebaseline_server/co...
gm/rebaseline_server/compare_rendered_pictures_test.py:131:
'compare_rendered_pictures.json'))
On 2014/08/21 18:25:06, epoger wrote:
> Where is the expectation for this test?  If you don't add a file within
> gm/rebaseline_server/testdata/outputs/expected , the file you generate here
> won't be compared against anything.
> 
> Personally, rather than adding a new test for this, I would recommend
modifying
> the existing test_endToEnd() test (add specification of image_base_gs_url to
> that test).  Then, the changes to the expected results file will show us
exactly
> what this change will do in practice.

I wanted to add a new test so that the case where --imageBaseGSUrl is not
specified is also tested, I figured it will not hurt to have a new test.
And I also went ahead and added the expected file.

[epoger, 2014-08-21 19:20:30]

lgtm

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
File gm/rebaseline_server/imagepair_test.py (right):

https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
gm/rebaseline_server/imagepair_test.py:79: # use alternate imageB base url:
On 2014/08/21 19:07:01, rmistry wrote:
> The parameter's value did modify the results of this test. Setting it to True
in
> any of the selftests here (except the first one) fails the test because the
> image does not exist in the new location. It only exists for selftest 1.
> 

Ah, so you added a new file to GS (
gs://chromium-skia/gm/test/gm/bitmap-64bitMD5/arcofzorro/16206093933823793653.png
) ...

If I'm not mistaken, the first selftest will succeed with either
(IMG_URL_BASE, ALT_IMG_URL_BASE)
or
(IMG_URL_BASE, IMG_URL_BASE)

(the latter is the same functionality that was being tested before your CL)

I think it would be a good idea to note the assumptions being made here about
which files are present in Google Storage, and which are not.

At any rate, I think it's a good idea to focus on the other test case, where we
make the code spit out a whole JSON file and we make sure it looks OK to us.

[rmistry, 2014-08-21 20:16:04]

On 2014/08/21 19:20:30, epoger wrote:
> lgtm
> 
>
https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
> File gm/rebaseline_server/imagepair_test.py (right):
> 
>
https://codereview.chromium.org/479613002/diff/160001/gm/rebaseline_server/im...
> gm/rebaseline_server/imagepair_test.py:79: # use alternate imageB base url:
> On 2014/08/21 19:07:01, rmistry wrote:
> > The parameter's value did modify the results of this test. Setting it to
True
> in
> > any of the selftests here (except the first one) fails the test because the
> > image does not exist in the new location. It only exists for selftest 1.
> > 
> 
> Ah, so you added a new file to GS (
>
gs://chromium-skia/gm/test/gm/bitmap-64bitMD5/arcofzorro/16206093933823793653.png
> ) ...
> 
> If I'm not mistaken, the first selftest will succeed with either
> (IMG_URL_BASE, ALT_IMG_URL_BASE)
> or
> (IMG_URL_BASE, IMG_URL_BASE)
> 
> (the latter is the same functionality that was being tested before your CL)

Yes this is true.

> 
> I think it would be a good idea to note the assumptions being made here about
> which files are present in Google Storage, and which are not.
> 
> At any rate, I think it's a good idea to focus on the other test case, where
we
> make the code spit out a whole JSON file and we make sure it looks OK to us.

Do you recommend I revert the changes in imagepair_test.py ? (I have no problem
throwing away those changes).

Also I ran python gm/test_all.py and saw there are a few failing tests which I
am going to take a look at.

[epoger, 2014-08-21 20:18:57]

On 2014/08/21 20:16:04, rmistry wrote:
> > I think it would be a good idea to note the assumptions being made here
about
> > which files are present in Google Storage, and which are not.
> > 
> > At any rate, I think it's a good idea to focus on the other test case, where
> we
> > make the code spit out a whole JSON file and we make sure it looks OK to us.
> 
> Do you recommend I revert the changes in imagepair_test.py ? (I have no
problem
> throwing away those changes).
> 
> Also I ran python gm/test_all.py and saw there are a few failing tests which I
> am going to take a look at.

I suspect the new expectations for the now-failing tests will provide sufficient
exercise for your change.  If so, I would recommend reverting the changes within
imagepair_test.py.

[rmistry, 2014-08-21 20:53:11]

On 2014/08/21 20:18:57, epoger wrote:
> On 2014/08/21 20:16:04, rmistry wrote:
> > > I think it would be a good idea to note the assumptions being made here
> about
> > > which files are present in Google Storage, and which are not.
> > > 
> > > At any rate, I think it's a good idea to focus on the other test case,
where
> > we
> > > make the code spit out a whole JSON file and we make sure it looks OK to
us.
> > 
> > Do you recommend I revert the changes in imagepair_test.py ? (I have no
> problem
> > throwing away those changes).
> > 
> > Also I ran python gm/test_all.py and saw there are a few failing tests which
I
> > am going to take a look at.
> 
> I suspect the new expectations for the now-failing tests will provide
sufficient
> exercise for your change.  If so, I would recommend reverting the changes
within
> imagepair_test.py.

* Reverted changes in imagepair_test.py
* python gm/test_all.py now passes

[epoger, 2014-08-21 20:55:17]

lgtm

[rmistry, 2014-08-22 11:03:43]

On 2014/08/21 20:55:17, epoger wrote:
> lgtm

I ran the housekeeper trybot and render_pictures_test failed. Investigating..

[rmistry, 2014-08-22 11:39:40]

On 2014/08/22 11:03:43, rmistry wrote:
> On 2014/08/21 20:55:17, epoger wrote:
> > lgtm
> 
> I ran the housekeeper trybot and render_pictures_test failed. Investigating..

Got Housekeeper-PerCommit-Trybot to pass. Rebased and it failed again.
Fixed Housekeeper-PerCommit-Trybot again.

Sending this CL to the CQ..

[rmistry, 2014-08-22 11:39:49]

The CQ bit was checked by rmistry@google.com

[commit-bot: I haz the power, 2014-08-22 11:40:15]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/rmistry@google.com/479613002/320001

[commit-bot: I haz the power, 2014-08-22 11:46:37]

Committed patchset #15 (320001) as 2529f2e72cddf87904c8ad4b613942cbef802cfb