Make android-chrome bisect work with archives.

Make android-chrome bisect work with archives.

BUG=465868
NOTRY=true

Committed: https://crrev.com/8235d84d7493f31b23f54c67d92a7115edaf4c8d
Cr-Commit-Position: refs/heads/master@{#320142}

[prasadv, 2015-03-10 21:07:03]

prasadv@chromium.org changed reviewers:
+ prasadv@chromium.org, qyearsley@chromium.org, robertocn@chromium.org

[qyearsley, 2015-03-10 23:52:32]

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:820: if depot not in ['chromium',
'android-chrome']:
Note: a tuple is used at
https://code.google.com/p/chromium/codesearch#chromium/src/tools/auto_bisect/....


This way is fine, but optionally you could switch to using a tuple to be
consistent with that line in that other module.

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:1023: # Incase of android-chrome
platform, download archives only for
Incase -> In case

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:1024: # for android-chrome depot,
for other depots such as chromium, v8, skia
The first "for" can be removed (it's already on the line above).
Also, I'd use a semicolon or period to separate "...for android-chrome depot"
and "for other depots...".

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:1025: # etc build the binary
locally.
"etc" -> "etc.,"

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:1027: return depot ==
'android-chrome'
Indentation should be two spaces.

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
File tools/auto_bisect/bisect_perf_regression_test.py (right):

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression_test.py:490:
bisect_perf_regression._TryJobSvnRepo(fetch_build.FULL_BUILDER))
Optional:

def testTryJobSvnRepoWithUnknownBuilderTypeThrowsError(self):
  with self.assertRaises(NotImplementedError):
    bisect_perf_regression._TryJobSvnRepo('foo')

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression_test.py:498: def
test_IsDownloadable_No_BuilderType(self):
Although I like underscores in test method names, and readability is the most
important thing for test method names, to be consistent with the other test
method names here I would not use underscores.

I changed all the test method names in the perf dashboard tests to have the
format test<Thing>_<Condition>_<Behavior> based on this doc:
http://go/unit-test-practices#naming. If you want to use this convention, then
this method name would be testIsDownloadable_NoBuilderType_ReturnsFalse

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression_test.py:526:
self.assertFalse(bisect_instance.IsDownloadable('v8'))
I like the fact that these test methods are quite simple and only test one thing
each.

If you want to factor out some of the repetition, you could also do:

def _CheckIsDownloadable(depot, target_platform='chromium',
builder_type='perf'):
  opts = dict(DEFAULT_OPTIONS)
  opts.update({'target_platform': target_platform, 'builder_type':
builder_type})
  bisect_instance = _GetBisectPerformanceMetricsInstance(opts)
  return bisect_instance.IsDownloadable(depot)

and then the other ones could be a bit shorter, e.g.:

def test_IsDownloadable_android_chrome_DEPS_V8(self):
  self.assertFalse(self._CheckIsDownloadable('v8', 'android-chrome', 'perf')

[prasadv, 2015-03-11 18:50:39]

ptal

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:820: if depot not in ['chromium',
'android-chrome']:
On 2015/03/10 23:52:32, qyearsley wrote:
> Note: a tuple is used at
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/auto_bisect/....
> 
> 
> This way is fine, but optionally you could switch to using a tuple to be
> consistent with that line in that other module.

Done.

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:1023: # Incase of android-chrome
platform, download archives only for
On 2015/03/10 23:52:32, qyearsley wrote:
> Incase -> In case

Done.

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:1024: # for android-chrome depot,
for other depots such as chromium, v8, skia
On 2015/03/10 23:52:32, qyearsley wrote:
> The first "for" can be removed (it's already on the line above).
> Also, I'd use a semicolon or period to separate "...for android-chrome depot"
> and "for other depots...".

Done.

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:1025: # etc build the binary
locally.
On 2015/03/10 23:52:32, qyearsley wrote:
> "etc" -> "etc.,"

Done.

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:1027: return depot ==
'android-chrome'
On 2015/03/10 23:52:32, qyearsley wrote:
> Indentation should be two spaces.

Done.

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
File tools/auto_bisect/bisect_perf_regression_test.py (right):

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression_test.py:490:
bisect_perf_regression._TryJobSvnRepo(fetch_build.FULL_BUILDER))
On 2015/03/10 23:52:32, qyearsley wrote:
> Optional:
> 
> def testTryJobSvnRepoWithUnknownBuilderTypeThrowsError(self):
>   with self.assertRaises(NotImplementedError):
>     bisect_perf_regression._TryJobSvnRepo('foo')

Done.

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression_test.py:498: def
test_IsDownloadable_No_BuilderType(self):
On 2015/03/10 23:52:32, qyearsley wrote:
> Although I like underscores in test method names, and readability is the most
> important thing for test method names, to be consistent with the other test
> method names here I would not use underscores.
> 
> I changed all the test method names in the perf dashboard tests to have the
> format test<Thing>_<Condition>_<Behavior> based on this doc:
> http://go/unit-test-practices#naming. If you want to use this convention, then
> this method name would be testIsDownloadable_NoBuilderType_ReturnsFalse

Done.

https://codereview.chromium.org/994183002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression_test.py:526:
self.assertFalse(bisect_instance.IsDownloadable('v8'))
On 2015/03/10 23:52:32, qyearsley wrote:
> I like the fact that these test methods are quite simple and only test one
thing
> each.
> 
> If you want to factor out some of the repetition, you could also do:
> 
> def _CheckIsDownloadable(depot, target_platform='chromium',
> builder_type='perf'):
>   opts = dict(DEFAULT_OPTIONS)
>   opts.update({'target_platform': target_platform, 'builder_type':
> builder_type})
>   bisect_instance = _GetBisectPerformanceMetricsInstance(opts)
>   return bisect_instance.IsDownloadable(depot)
> 
> and then the other ones could be a bit shorter, e.g.:
> 
> def test_IsDownloadable_android_chrome_DEPS_V8(self):
>   self.assertFalse(self._CheckIsDownloadable('v8', 'android-chrome', 'perf')

Done.

[qyearsley, 2015-03-11 18:58:22]

lgtm

[prasadv, 2015-03-11 20:49:32]

The CQ bit was checked by prasadv@chromium.org

[commit-bot: I haz the power, 2015-03-11 20:49:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/994183002/20001

[commit-bot: I haz the power, 2015-03-11 20:51:38]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-11 20:52:21]

Patchset 2 (id:??) landed as
https://crrev.com/8235d84d7493f31b23f54c67d92a7115edaf4c8d
Cr-Commit-Position: refs/heads/master@{#320142}