rebaseline.py: add --add-new option, and only add new expectations if it is set

rebaseline.py: add --add-new option, and only add new expectations if it is set

R=senorblanco@chromium.org

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

[epoger, 2013-06-11 17:11:53]

Ready for review at patchset 3.

[Stephen White, 2013-06-11 17:29:39]

LGTM

https://codereview.chromium.org/16363006/diff/10001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/16363006/diff/10001/tools/rebaseline.py#newco...
tools/rebaseline.py:27: # written out by the GM tool.
Thinking about this some more, maybe gm_json.py should be in tools instead? I
guess that just moves the problem to display_json_results.py. Maybe that should
be in tools too, actually. I think having all the python in one place and all
the C++ in another is kind of nice, but I defer to the greater team on that.

https://codereview.chromium.org/16363006/diff/10001/tools/rebaseline.py#newco...
tools/rebaseline.py:238: if self._add_new:
Actually, at least for git, I think we want to do this regardless of --add-new.
This will put them in the index/staging area, ready for commit, which is
consistent with the blink rebaselining tools. And although my svn-fu seems to be
atrophying, I don't think there's any harm with adding them in the svn case as
well.

[epoger, 2013-06-11 18:10:20]

Thanks, Stephen.  I won't commit this until we get a better handle on the "svn
add" question below.

https://codereview.chromium.org/16363006/diff/10001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/16363006/diff/10001/tools/rebaseline.py#newco...
tools/rebaseline.py:27: # written out by the GM tool.
On 2013/06/11 17:29:39, Stephen White wrote:
> Thinking about this some more, maybe gm_json.py should be in tools instead? I
> guess that just moves the problem to display_json_results.py. Maybe that
should
> be in tools too, actually. I think having all the python in one place and all
> the C++ in another is kind of nice, but I defer to the greater team on that.

I think this is very topical in the context of recent discussions elsewhere
about directory organization, moving to git repository(ies), etc.

My goal was to maintain the JSON file schema definition (embodied by gm_json.py
and gm_expectations.cpp) in one place, not two places that could get out of
sync.

Ultimately, gm and all this other stuff is all tools (stuff that developers and
buildbots might run to exercise the Skia library).

https://codereview.chromium.org/16363006/diff/10001/tools/rebaseline.py#newco...
tools/rebaseline.py:238: if self._add_new:
On 2013/06/11 17:29:39, Stephen White wrote:
> Actually, at least for git, I think we want to do this regardless of
--add-new.
> This will put them in the index/staging area, ready for commit, which is
> consistent with the blink rebaselining tools. And although my svn-fu seems to
be
> atrophying, I don't think there's any harm with adding them in the svn case as
> well.

Brian and/or Rob, do you have any thoughts on this?

My understanding is that there was some frustration about having to manually
un-add expectations after running rebaseline.py, and that this might make it
better.  But maybe I misunderstood the situation...

[bsalomon, 2013-06-12 14:13:10]

On 2013/06/11 18:10:20, epoger wrote:
> Thanks, Stephen.  I won't commit this until we get a better handle on the "svn
> add" question below.
> 
> https://codereview.chromium.org/16363006/diff/10001/tools/rebaseline.py
> File tools/rebaseline.py (right):
> 
>
https://codereview.chromium.org/16363006/diff/10001/tools/rebaseline.py#newco...
> tools/rebaseline.py:27: # written out by the GM tool.
> On 2013/06/11 17:29:39, Stephen White wrote:
> > Thinking about this some more, maybe gm_json.py should be in tools instead?
I
> > guess that just moves the problem to display_json_results.py. Maybe that
> should
> > be in tools too, actually. I think having all the python in one place and
all
> > the C++ in another is kind of nice, but I defer to the greater team on that.
> 
> I think this is very topical in the context of recent discussions elsewhere
> about directory organization, moving to git repository(ies), etc.
> 
> My goal was to maintain the JSON file schema definition (embodied by
gm_json.py
> and gm_expectations.cpp) in one place, not two places that could get out of
> sync.
> 
> Ultimately, gm and all this other stuff is all tools (stuff that developers
and
> buildbots might run to exercise the Skia library).
> 
>
https://codereview.chromium.org/16363006/diff/10001/tools/rebaseline.py#newco...
> tools/rebaseline.py:238: if self._add_new:
> On 2013/06/11 17:29:39, Stephen White wrote:
> > Actually, at least for git, I think we want to do this regardless of
> --add-new.
> > This will put them in the index/staging area, ready for commit, which is
> > consistent with the blink rebaselining tools. And although my svn-fu seems
to
> be
> > atrophying, I don't think there's any harm with adding them in the svn case
as
> > well.
> 
> Brian and/or Rob, do you have any thoughts on this?
> 
> My understanding is that there was some frustration about having to manually
> un-add expectations after running rebaseline.py, and that this might make it
> better.  But maybe I misunderstood the situation...

It has been a pain in the past. I use some combo of piping svn stat through grep
and sed to revert and delete them when I don't want to add the new baselines.
I'd be nice to be able to skip that.

[robertphillips, 2013-06-12 14:13:41]

https://codereview.chromium.org/16363006/diff/10001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/16363006/diff/10001/tools/rebaseline.py#newco...
tools/rebaseline.py:238: if self._add_new:
I would greatly prefer if the addition of new baselines was opt-in (as is
proposed in this CL). The current behavior of the rebaseline script (always add
new baselines) makes for more typing on my part (to remove them).

[epoger, 2013-06-12 14:24:11]

https://codereview.chromium.org/16363006/diff/10001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/16363006/diff/10001/tools/rebaseline.py#newco...
tools/rebaseline.py:169: #            - JSONKEY_ACTUALRESULTS_FAILUREIGNORED
Patchset 4 removes FAILUREIGNORED from the list of result types to rebaseline,
based on live discussion with Rob and Brian.

Although we don't actually mark any tests as FAILUREIGNORED yet, our plan is to
do that for tests that are known to fail... which means that we typically DON'T
want to check in their actual results as their expectations.

[epoger, 2013-06-12 14:25:12]

On 2013/06/12 14:13:41, robertphillips wrote:
> https://codereview.chromium.org/16363006/diff/10001/tools/rebaseline.py
> File tools/rebaseline.py (right):
> 
>
https://codereview.chromium.org/16363006/diff/10001/tools/rebaseline.py#newco...
> tools/rebaseline.py:238: if self._add_new:
> I would greatly prefer if the addition of new baselines was opt-in (as is
> proposed in this CL). The current behavior of the rebaseline script (always
add
> new baselines) makes for more typing on my part (to remove them).

Given Rob and Brian's comments, I'm gonna go ahead and commit.  I hope that's OK
with you, Stephen... we can of course make adjustments later as needed.

[epoger, 2013-06-12 14:25:34]

Committed patchset #4 manually as r9527 (presubmit successful).