rebaseline.py: add --bugs and --unreviewed flags

rebaseline.py: add --bugs and --unreviewed flags
BUG=skia:1569
R=borenet@google.com

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

[epoger, 2013-08-29 19:29:20]

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

https://codereview.chromium.org/23478011/diff/1/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/marked-unreviewed"
Patchset 1 adds a new "marked-unreviewed" self-test.  For now, rebaseline.py
still doesn't know how to do this, so these self-test results are "svn cp'ed"
from the "using-json1-expectations" self-test.

[epoger, 2013-08-29 19:45:31]

https://codereview.chromium.org/23478011/diff/5001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/23478011/diff/5001/tools/rebaseline.py#newcod...
tools/rebaseline.py:292: if self._mark_unreviewed:
Patchset 2 adds the --bugs and --unreviewed flags, and sets JSON fields
accordingly.

https://codereview.chromium.org/23478011/diff/5001/tools/tests/rebaseline/out...
File
tools/tests/rebaseline/output/marked-unreviewed/output-expected/gm-expectations/Test-Android-GalaxyNexus-SGX540-Arm7-Debug/expected-results.json
(right):

https://codereview.chromium.org/23478011/diff/5001/tools/tests/rebaseline/out...
tools/tests/rebaseline/output/marked-unreviewed/output-expected/gm-expectations/Test-Android-GalaxyNexus-SGX540-Arm7-Debug/expected-results.json:15:
11899819492385205974
There's a problem here... rebaseline.py didn't actually have to modify the
allowed-digests for this test, because it was already passing.

(see this line in
tools/tests/rebaseline/input/json1/Test-Android-GalaxyNexus-SGX540-Arm7-Debug/actual-results.json)
: 
"aaclip_gpu.png" : [ "bitmap-64bitMD5", 11899819492385205974 ],

I need to adjust rebaseline.py such that we only add the "bugs" and
"reviewed-by-human" fields to records that are actually updated.

[epoger, 2013-08-29 19:56:53]

https://codereview.chromium.org/23478011/diff/5001/tools/tests/rebaseline/out...
File
tools/tests/rebaseline/output/marked-unreviewed/output-expected/gm-expectations/Test-Android-GalaxyNexus-SGX540-Arm7-Debug/expected-results.json
(right):

https://codereview.chromium.org/23478011/diff/5001/tools/tests/rebaseline/out...
tools/tests/rebaseline/output/marked-unreviewed/output-expected/gm-expectations/Test-Android-GalaxyNexus-SGX540-Arm7-Debug/expected-results.json:15:
11899819492385205974
On 2013/08/29 19:45:31, epoger wrote:
> There's a problem here... rebaseline.py didn't actually have to modify the
> allowed-digests for this test, because it was already passing.
> 
> (see this line in
>
tools/tests/rebaseline/input/json1/Test-Android-GalaxyNexus-SGX540-Arm7-Debug/actual-results.json)
> : 
> "aaclip_gpu.png" : [ "bitmap-64bitMD5", 11899819492385205974 ],
> 
> I need to adjust rebaseline.py such that we only add the "bugs" and
> "reviewed-by-human" fields to records that are actually updated.

Patchset 3 fixes this problem.

[epoger, 2013-08-29 20:12:54]

Ready for review at patchset 3.

I'm fine with any one of you as reviewer; by default, I'll pick on Rob this
time.

[epoger, 2013-08-30 09:18:24]

Still ready for review, just added one more little thing...

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

https://codereview.chromium.org/23478011/diff/26001/tools/rebaseline.py#newco...
tools/rebaseline.py:301: if self._notes:
Patchset 4 adds --notes flag, and records those notes in the appropriate place
within the JSON.

[borenet, 2013-08-30 13:26:55]

https://codereview.chromium.org/23478011/diff/26001/gm/gm_json.py
File gm/gm_json.py (right):

https://codereview.chromium.org/23478011/diff/26001/gm/gm_json.py#newcode54
gm/gm_json.py:54: JSONKEY_EXPECTEDRESULTS_BUGS = 'bugs'
Are these just bug numbers?

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

https://codereview.chromium.org/23478011/diff/26001/tools/rebaseline.py#newco...
tools/rebaseline.py:296:
expected_results[image_name][gm_json.JSONKEY_EXPECTEDRESULTS_REVIEWED] = \
Nit: 80 chars

https://codereview.chromium.org/23478011/diff/26001/tools/rebaseline.py#newco...
tools/rebaseline.py:302:
expected_results[image_name][gm_json.JSONKEY_EXPECTEDRESULTS_NOTES] = \
80 chars

[epoger, 2013-08-30 14:50:15]

Thanks!

https://codereview.chromium.org/23478011/diff/26001/gm/gm_json.py
File gm/gm_json.py (right):

https://codereview.chromium.org/23478011/diff/26001/gm/gm_json.py#newcode54
gm/gm_json.py:54: JSONKEY_EXPECTEDRESULTS_BUGS = 'bugs'
On 2013/08/30 13:26:55, borenet wrote:
> Are these just bug numbers?

Yes.  I tried to make that clear with the description, but I guess I failed. :-)
 Any advice on how to improve the description?

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

https://codereview.chromium.org/23478011/diff/26001/tools/rebaseline.py#newco...
tools/rebaseline.py:296:
expected_results[image_name][gm_json.JSONKEY_EXPECTEDRESULTS_REVIEWED] = \
On 2013/08/30 13:26:55, borenet wrote:
> Nit: 80 chars

Yes, but for these lines (as well as line 293 above) I couldn't think of another
way of breaking the lines without making it harder to understand.  If you have a
suggestion, I'll take it...

[epoger, 2013-09-03 13:11:23]

Ping?  (Eric / Rob / anyone else)

[borenet, 2013-09-03 15:42:53]

LGTM, just some style comments.

https://codereview.chromium.org/23478011/diff/26001/gm/gm_json.py
File gm/gm_json.py (right):

https://codereview.chromium.org/23478011/diff/26001/gm/gm_json.py#newcode54
gm/gm_json.py:54: JSONKEY_EXPECTEDRESULTS_BUGS = 'bugs'
On 2013/08/30 14:50:15, epoger wrote:
> On 2013/08/30 13:26:55, borenet wrote:
> > Are these just bug numbers?
> 
> Yes.  I tried to make that clear with the description, but I guess I failed.
:-)
>  Any advice on how to improve the description?

Reading it again, I think it looks fine.

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

https://codereview.chromium.org/23478011/diff/26001/tools/rebaseline.py#newco...
tools/rebaseline.py:296:
expected_results[image_name][gm_json.JSONKEY_EXPECTEDRESULTS_REVIEWED] = \
On 2013/08/30 14:50:15, epoger wrote:
> On 2013/08/30 13:26:55, borenet wrote:
> > Nit: 80 chars
> 
> Yes, but for these lines (as well as line 293 above) I couldn't think of
another
> way of breaking the lines without making it harder to understand.  If you have
a
> suggestion, I'll take it...

Maybe something like this?

if self._mark_unreviewed:
  expected_results[image_name]
      [gm_json.JSONKEY_EXPECTEDRESULTS_REVIEWED] = False

You might be able to line up the ['s as well.

It's admittedly ugly.  Otherwise, if you can't break it up nicely and you're
going over the 80 character limit anyway, I suggest putting the "= False" (etc)
on the same line for easier reading.

https://codereview.chromium.org/23478011/diff/26001/tools/rebaseline.py#newco...
tools/rebaseline.py:337: 'pertain to this set of rebaselines.')
I'd prefer this syntax for multi-line strings:

parser.add_argument('--bugs', metavar='BUG', type=int, nargs='+',
                    help=('Skia bug numbers (under '
                          'https://code.google.com/p/skia/issues/list ) which '
                          'pertain to this set of rebaselines.'))

[epoger, 2013-09-03 17:28:13]

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

https://codereview.chromium.org/23478011/diff/26001/tools/rebaseline.py#newco...
tools/rebaseline.py:296:
expected_results[image_name][gm_json.JSONKEY_EXPECTEDRESULTS_REVIEWED] = \
On 2013/09/03 15:42:53, borenet wrote:
> Maybe something like this?
> 
> if self._mark_unreviewed:
>   expected_results[image_name]
>       [gm_json.JSONKEY_EXPECTEDRESULTS_REVIEWED] = False
> 
> You might be able to line up the ['s as well.

I took that idea and ran with it.  What do you think of the results?

https://codereview.chromium.org/23478011/diff/26001/tools/rebaseline.py#newco...
tools/rebaseline.py:337: 'pertain to this set of rebaselines.')
On 2013/09/03 15:42:53, borenet wrote:
> I'd prefer this syntax for multi-line strings:
> 
> parser.add_argument('--bugs', metavar='BUG', type=int, nargs='+',
>                     help=('Skia bug numbers (under '
>                           'https://code.google.com/p/skia/issues/list ) which
'
>                           'pertain to this set of rebaselines.'))

Done.

[borenet, 2013-09-03 17:30:56]

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

https://codereview.chromium.org/23478011/diff/26001/tools/rebaseline.py#newco...
tools/rebaseline.py:296:
expected_results[image_name][gm_json.JSONKEY_EXPECTEDRESULTS_REVIEWED] = \
On 2013/09/03 17:28:14, epoger wrote:
> On 2013/09/03 15:42:53, borenet wrote:
> > Maybe something like this?
> > 
> > if self._mark_unreviewed:
> >   expected_results[image_name]
> >       [gm_json.JSONKEY_EXPECTEDRESULTS_REVIEWED] = False
> > 
> > You might be able to line up the ['s as well.
> 
> I took that idea and ran with it.  What do you think of the results?

LGTM.  Are the backslashes required?

[epoger, 2013-09-03 17:31:46]

On 2013/09/03 17:30:56, borenet wrote:
> https://codereview.chromium.org/23478011/diff/26001/tools/rebaseline.py
> File tools/rebaseline.py (right):
> 
>
https://codereview.chromium.org/23478011/diff/26001/tools/rebaseline.py#newco...
> tools/rebaseline.py:296:
> expected_results[image_name][gm_json.JSONKEY_EXPECTEDRESULTS_REVIEWED] = \
> On 2013/09/03 17:28:14, epoger wrote:
> > On 2013/09/03 15:42:53, borenet wrote:
> > > Maybe something like this?
> > > 
> > > if self._mark_unreviewed:
> > >   expected_results[image_name]
> > >       [gm_json.JSONKEY_EXPECTEDRESULTS_REVIEWED] = False
> > > 
> > > You might be able to line up the ['s as well.
> > 
> > I took that idea and ran with it.  What do you think of the results?
> 
> LGTM.  Are the backslashes required?

Yes \
they \
are. \
I \
sure \
love \
Python.

[epoger, 2013-09-03 17:32:27]

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