rebaseline.py: fix --add-new when there are no expectations at all

rebaseline.py: fix --add-new when there are no expectations at all
BUG=skia:1582
R=rmistry@google.com

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

[epoger, 2013-09-03 15:06:09]

https://codereview.chromium.org/23899003/diff/1/tools/tests/rebaseline/input/...
File
tools/tests/rebaseline/input/json1/Test-Mac10.6-MacMini4.1-GeForce320M-x86-Release/expected-results.json
(right):

https://codereview.chromium.org/23899003/diff/1/tools/tests/rebaseline/input/...
tools/tests/rebaseline/input/json1/Test-Mac10.6-MacMini4.1-GeForce320M-x86-Release/expected-results.json:1:
{}
Patchset 1: change the expectations for one self-test platform to be completely
empty.

[epoger, 2013-09-03 15:14:40]

https://codereview.chromium.org/23899003/diff/2/tools/tests/rebaseline/input/...
File
tools/tests/rebaseline/input/json1/Test-Mac10.6-MacMini4.1-GeForce320M-x86-Release/actual-results.json
(right):

https://codereview.chromium.org/23899003/diff/2/tools/tests/rebaseline/input/...
tools/tests/rebaseline/input/json1/Test-Mac10.6-MacMini4.1-GeForce320M-x86-Release/actual-results.json:6:
"aaclip_565.png" : [ "bitmap-64bitMD5", 6190901827590820995 ],
Patchset 2 fixes actual-results.json for the new platform to reflect what the GM
tool would have generated if there were no expectations.  (This should have been
part of patchset 1, but I forgot)

[epoger, 2013-09-03 15:37:54]

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

https://codereview.chromium.org/23899003/diff/7001/tools/tests/run.sh#newcode246
tools/tests/run.sh:246: rebaseline_test "$REBASELINE_INPUT/json1"
"--actuals-base-url $REBASELINE_INPUT/json1 --builders
Test-Android-GalaxyNexus-SGX540-Arm7-Debug
Test-Win7-ShuttleA-HD2000-x86-Release" "$REBASELINE_OUTPUT/add-new"
Patchset 3: create a new "add-new" self-test.  For now, it operates exactly the
same as the "using-json1-expectations" self-test... it the next patchset, I will
add the --add-new flag and we'll confirm that its output changes accordingly.

[epoger, 2013-09-03 15:53:10]

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

https://codereview.chromium.org/23899003/diff/7001/tools/tests/run.sh#newcode246
tools/tests/run.sh:246: rebaseline_test "$REBASELINE_INPUT/json1"
"--actuals-base-url $REBASELINE_INPUT/json1 --builders
Test-Android-GalaxyNexus-SGX540-Arm7-Debug
Test-Win7-ShuttleA-HD2000-x86-Release" "$REBASELINE_OUTPUT/add-new"
On 2013/09/03 15:37:54, epoger wrote:
> Patchset 3: create a new "add-new" self-test.  For now, it operates exactly
the
> same as the "using-json1-expectations" self-test... it the next patchset, I
will
> add the --add-new flag and we'll confirm that its output changes accordingly.

Patchset 4: added the "add-new" flag to this self-test, and rebaseline.py's
output files changed accordingly.

https://codereview.chromium.org/23899003/diff/26001/tools/tests/rebaseline/ou...
File
tools/tests/rebaseline/output/add-new/output-expected/gm-expectations/Test-Android-GalaxyNexus-SGX540-Arm7-Debug/expected-results.json
(right):

https://codereview.chromium.org/23899003/diff/26001/tools/tests/rebaseline/ou...
tools/tests/rebaseline/output/add-new/output-expected/gm-expectations/Test-Android-GalaxyNexus-SGX540-Arm7-Debug/expected-results.json:6:
"bitmap-64bitMD5",
Patchset 4: now that --add-new is being used, the actual checksums from the
"no-comparison" part of this platform's actual-results.json are being filled in
here, as expected.

[epoger, 2013-09-03 16:54:38]

https://codereview.chromium.org/23899003/diff/30001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/23899003/diff/30001/tools/rebaseline.py#newco...
tools/rebaseline.py:124: raise exc_info[1], None, exc_info[2]
Patchset 5: While I was at it, improved error reporting so that it was a lot
easier to see what was going wrong.

https://codereview.chromium.org/23899003/diff/30001/tools/rebaseline.py#newco...
tools/rebaseline.py:257: if not expected_results:
Patchset 5: properly handle the case where there are no expectations at all for
a given builder.

https://codereview.chromium.org/23899003/diff/30001/tools/tests/rebaseline/ou...
File
tools/tests/rebaseline/output/add-new/output-expected/gm-expectations/Test-Mac10.6-MacMini4.1-GeForce320M-x86-Release/expected-results.json
(right):

https://codereview.chromium.org/23899003/diff/30001/tools/tests/rebaseline/ou...
tools/tests/rebaseline/output/add-new/output-expected/gm-expectations/Test-Mac10.6-MacMini4.1-GeForce320M-x86-Release/expected-results.json:2:
"expected-results": {
Patchset 5: the expectations coming out of rebaseline.py for the new platform
now include all the checksums from actual-results.json , as they should.

[epoger, 2013-09-03 16:55:44]

Ready for review at patchset 5.

[rmistry, 2013-09-03 17:09:35]

https://codereview.chromium.org/23899003/diff/30001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/23899003/diff/30001/tools/rebaseline.py#newco...
tools/rebaseline.py:110: # or exit the program right away.
Make this a method level comment.

https://codereview.chromium.org/23899003/diff/30001/tools/rebaseline.py#newco...
tools/rebaseline.py:117: # exc_info[1] , but for some reason the former
preserves more information.
Thats because exc_info[1] is like calling str(exc_info[1]) which is a user
friendly string.
For [exc_info[1]] it calls repr(exc_info[1]).
You should get the output you want by changing [exc_info[1]] to
repr(exc_info[1])

https://codereview.chromium.org/23899003/diff/30001/tools/rebaseline.py#newco...
tools/rebaseline.py:258: expected_results = {}
I think you should be able to get rid of this line by changing line:256 to

expected_results = expectations_dict.get(gm_json.JSONKEY_EXPECTEDRESULTS, {})

https://codereview.chromium.org/23899003/diff/30001/tools/rebaseline.py#newco...
tools/rebaseline.py:381: except BaseException:
Why is this catching BaseException instead of Exception which is what
_InternalException inherits from?

[epoger, 2013-09-03 18:01:33]

Thanks for the suggestions...

https://codereview.chromium.org/23899003/diff/30001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/23899003/diff/30001/tools/rebaseline.py#newco...
tools/rebaseline.py:110: # or exit the program right away.
On 2013/09/03 17:09:35, rmistry wrote:
> Make this a method level comment.

Done throughout ExceptionHandler object.

https://codereview.chromium.org/23899003/diff/30001/tools/rebaseline.py#newco...
tools/rebaseline.py:117: # exc_info[1] , but for some reason the former
preserves more information.
On 2013/09/03 17:09:35, rmistry wrote:
> Thats because exc_info[1] is like calling str(exc_info[1]) which is a user
> friendly string.
> For [exc_info[1]] it calls repr(exc_info[1]).
> You should get the output you want by changing [exc_info[1]] to
> repr(exc_info[1])

Works as you describe... thanks!  Done.

https://codereview.chromium.org/23899003/diff/30001/tools/rebaseline.py#newco...
tools/rebaseline.py:258: expected_results = {}
On 2013/09/03 17:09:35, rmistry wrote:
> I think you should be able to get rid of this line by changing line:256 to
> 
> expected_results = expectations_dict.get(gm_json.JSONKEY_EXPECTEDRESULTS, {})

Done, but we still need to set
expectations_dict[gm_json.JSONKEY_EXPECTEDRESULTS] = expected_results if there
weren't any expectations yet.

Do you find the new code preferable?  I actually prefer the old code... one more
line of code, but (IMHO) easier to follow.

if

https://codereview.chromium.org/23899003/diff/30001/tools/rebaseline.py#newco...
tools/rebaseline.py:381: except BaseException:
On 2013/09/03 17:09:35, rmistry wrote:
> Why is this catching BaseException instead of Exception which is what
> _InternalException inherits from?

I don't think it really matters.  Got rid of the explicit exception type.

[rmistry, 2013-09-03 18:17:18]

LGTM

https://codereview.chromium.org/23899003/diff/30001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/23899003/diff/30001/tools/rebaseline.py#newco...
tools/rebaseline.py:258: expected_results = {}
On 2013/09/03 18:01:33, epoger wrote:
> On 2013/09/03 17:09:35, rmistry wrote:
> > I think you should be able to get rid of this line by changing line:256 to
> > 
> > expected_results = expectations_dict.get(gm_json.JSONKEY_EXPECTEDRESULTS,
{})
> 
> Done, but we still need to set
> expectations_dict[gm_json.JSONKEY_EXPECTEDRESULTS] = expected_results if there
> weren't any expectations yet.
> 
> Do you find the new code preferable?  I actually prefer the old code... one
more
> line of code, but (IMHO) easier to follow.
> 
> if 

Lets go with what you prefer, not a big deal either way to me.

https://codereview.chromium.org/23899003/diff/30001/tools/rebaseline.py#newco...
tools/rebaseline.py:381: except BaseException:
On 2013/09/03 18:01:33, epoger wrote:
> On 2013/09/03 17:09:35, rmistry wrote:
> > Why is this catching BaseException instead of Exception which is what
> > _InternalException inherits from?
> 
> I don't think it really matters.  Got rid of the explicit exception type.

I think it does slightly matter because 'except BaseException' catches more than
the subclasses of Exception.

[epoger, 2013-09-03 18:23:07]

https://codereview.chromium.org/23899003/diff/30001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/23899003/diff/30001/tools/rebaseline.py#newco...
tools/rebaseline.py:258: expected_results = {}
On 2013/09/03 18:17:18, rmistry wrote:
> On 2013/09/03 18:01:33, epoger wrote:
> > On 2013/09/03 17:09:35, rmistry wrote:
> > > I think you should be able to get rid of this line by changing line:256 to
> > > 
> > > expected_results = expectations_dict.get(gm_json.JSONKEY_EXPECTEDRESULTS,
> {})
> > 
> > Done, but we still need to set
> > expectations_dict[gm_json.JSONKEY_EXPECTEDRESULTS] = expected_results if
there
> > weren't any expectations yet.
> > 
> > Do you find the new code preferable?  I actually prefer the old code... one
> more
> > line of code, but (IMHO) easier to follow.
> > 
> > if 
> 
> Lets go with what you prefer, not a big deal either way to me.

Went back to my old way, then. :-)

[epoger, 2013-09-03 18:27:30]

Committed patchset #8 manually as r11063 (presubmit successful).