Try to expand the revision range to include a change to .DEPS.git if the script detects a change to…

Try to expand the revision range to include a change to .DEPS.git if the script detects a change to DEPS

BUG=
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202880

[tonyg, 2013-05-28 23:16:07]

Just some nits, overall looks good. Thanks for putting this together so quickly.

https://codereview.chromium.org/16023021/diff/1/tools/bisect-perf-regression.py
File tools/bisect-perf-regression.py (right):

https://codereview.chromium.org/16023021/diff/1/tools/bisect-perf-regression....
tools/bisect-perf-regression.py:512: assert not return_code, 'An error occurred
while running'\
We already do this assert not return_code dance in a lot of places and this
patch adds 3 new ones. What do you think about factoring out a CheckRunGit(cmd)
method that does the assert and only returns output.

https://codereview.chromium.org/16023021/diff/1/tools/bisect-perf-regression....
tools/bisect-perf-regression.py:1075: bad_rev: First known bad revision.
Nit: args are in the wrong order here

https://codereview.chromium.org/16023021/diff/1/tools/bisect-perf-regression....
tools/bisect-perf-regression.py:1078: A tuple with the new good and bad
revisions.
A tuple with the new bad and good revisions

https://codereview.chromium.org/16023021/diff/1/tools/bisect-perf-regression....
tools/bisect-perf-regression.py:1087: oldest_deps_change =
changes_to_deps[len(changes_to_deps) - 1]
Neat python trick:
oldest_deps_change = changes_to_deps[-1]

https://codereview.chromium.org/16023021/diff/1/tools/bisect-perf-regression....
tools/bisect-perf-regression.py:1102: # next 5 minutes after the DEPS file
change.
I didn't look how long it usually takes, but 5 minutes seems slightly brittle.
Maybe 15 or 20 mins?

https://codereview.chromium.org/16023021/diff/1/tools/bisect-perf-regression....
tools/bisect-perf-regression.py:1461: 'deviations from "good" value. Results may
not be accurate.')
Something for a future CL that I'll mention while it's on my mind: It would be
nice if the script printed the magnitude of the change that it detected. That
is, the percent difference of the average of all the "good" revisions and the
average of all the "bad" revisions.

Then the sheriff can more easily tell whether the change that it detected
matched the size of the change they were looking for.

[shatch, 2013-05-28 23:38:17]

New snapshot uploaded.

https://codereview.chromium.org/16023021/diff/1/tools/bisect-perf-regression.py
File tools/bisect-perf-regression.py (right):

https://codereview.chromium.org/16023021/diff/1/tools/bisect-perf-regression....
tools/bisect-perf-regression.py:512: assert not return_code, 'An error occurred
while running'\
On 2013/05/28 23:16:07, tonyg wrote:
> We already do this assert not return_code dance in a lot of places and this
> patch adds 3 new ones. What do you think about factoring out a
CheckRunGit(cmd)
> method that does the assert and only returns output.

Done.

https://codereview.chromium.org/16023021/diff/1/tools/bisect-perf-regression....
tools/bisect-perf-regression.py:1075: bad_rev: First known bad revision.
On 2013/05/28 23:16:07, tonyg wrote:
> Nit: args are in the wrong order here

Done.

https://codereview.chromium.org/16023021/diff/1/tools/bisect-perf-regression....
tools/bisect-perf-regression.py:1078: A tuple with the new good and bad
revisions.
On 2013/05/28 23:16:07, tonyg wrote:
> A tuple with the new bad and good revisions

Done.

https://codereview.chromium.org/16023021/diff/1/tools/bisect-perf-regression....
tools/bisect-perf-regression.py:1087: oldest_deps_change =
changes_to_deps[len(changes_to_deps) - 1]
On 2013/05/28 23:16:07, tonyg wrote:
> Neat python trick:
> oldest_deps_change = changes_to_deps[-1]

Cool!

https://codereview.chromium.org/16023021/diff/1/tools/bisect-perf-regression....
tools/bisect-perf-regression.py:1102: # next 5 minutes after the DEPS file
change.
On 2013/05/28 23:16:07, tonyg wrote:
> I didn't look how long it usually takes, but 5 minutes seems slightly brittle.
> Maybe 15 or 20 mins?

Done.

https://codereview.chromium.org/16023021/diff/1/tools/bisect-perf-regression....
tools/bisect-perf-regression.py:1461: 'deviations from "good" value. Results may
not be accurate.')
On 2013/05/28 23:16:07, tonyg wrote:
> Something for a future CL that I'll mention while it's on my mind: It would be
> nice if the script printed the magnitude of the change that it detected. That
> is, the percent difference of the average of all the "good" revisions and the
> average of all the "bad" revisions.
> 
> Then the sheriff can more easily tell whether the change that it detected
> matched the size of the change they were looking for.

Yeah that sounds reasonable, I'll get a version with that change up soon.

[tonyg, 2013-05-28 23:45:06]

lgtm

https://codereview.chromium.org/16023021/diff/5001/tools/bisect-perf-regressi...
File tools/bisect-perf-regression.py (right):

https://codereview.chromium.org/16023021/diff/5001/tools/bisect-perf-regressi...
tools/bisect-perf-regression.py:268: return (output, return_code)
Since we know return_code is 0 at this point, I'd just return output instead of
returning a tuple. It will make the callsites cleaner too.

https://codereview.chromium.org/16023021/diff/5001/tools/bisect-perf-regressi...
tools/bisect-perf-regression.py:368: results = RunGit(['checkout', revision])[1]
CheckRunGit?

https://codereview.chromium.org/16023021/diff/5001/tools/bisect-perf-regressi...
tools/bisect-perf-regression.py:1539: (output, return_code) =
RunGit(['rev-parse', '--is-inside-work-tree'])
CheckRunGit?

[shatch, 2013-05-29 16:25:28]

https://codereview.chromium.org/16023021/diff/5001/tools/bisect-perf-regressi...
File tools/bisect-perf-regression.py (right):

https://codereview.chromium.org/16023021/diff/5001/tools/bisect-perf-regressi...
tools/bisect-perf-regression.py:268: return (output, return_code)
On 2013/05/28 23:45:06, tonyg wrote:
> Since we know return_code is 0 at this point, I'd just return output instead
of
> returning a tuple. It will make the callsites cleaner too.

Done.

https://codereview.chromium.org/16023021/diff/5001/tools/bisect-perf-regressi...
tools/bisect-perf-regression.py:368: results = RunGit(['checkout', revision])[1]
On 2013/05/28 23:45:06, tonyg wrote:
> CheckRunGit?

Not sure under which conditions checkout can fail. At the moment, the script
will continue and mark that build as bad, changing it to CheckRunGit would
assert and stop the run completely.

https://codereview.chromium.org/16023021/diff/5001/tools/bisect-perf-regressi...
tools/bisect-perf-regression.py:1539: (output, return_code) =
RunGit(['rev-parse', '--is-inside-work-tree'])
On 2013/05/28 23:45:06, tonyg wrote:
> CheckRunGit?

Prefer not to here, since a more descriptive error message is displayed after if
this function fails.

[commit-bot: I haz the power, 2013-05-29 16:25:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonhatch@chromium.org/16023021/10001

[commit-bot: I haz the power, 2013-05-29 16:28:54]

Change committed as 202880