Add support in Telemetry for specifying exact Android APK

Add support in Telemetry for specifying exact Android APK

Prior to this commit, you could not specify a path to an Android APK to run a
Telemetry test against. Desktop was the only platform that supported it.
This commit provides support support for running reference builds for the
Android platform in buildbot.

To run a Telemetry test against an exact APK, you can run the following
in tools/perf:

./run_benchmark --browser=exact --browser-executable=/path/to/my.apk benchmark

This also adds myself (mstrum@amazon.com) to AUTHORS.

R=tonyg@chromium.org
BUG=435179
Committed: https://crrev.com/d0fa8f06d525c62db23b2bc65147d721b9999015
Cr-Commit-Position: refs/heads/master@{#310346}

[mstrum, 2014-11-20 18:33:14]

Hi Tony,

Here's a proposed solution for:
https://code.google.com/p/chromium/issues/detail?id=435179

Can you take a look?

Thanks,
Matt

[mstrum, 2014-11-30 23:58:30]

ping~

[tonyg, 2014-12-03 18:38:43]

tonyg@chromium.org changed reviewers:
+ aiolos@chromium.org, dtu@chromium.org

[tonyg, 2014-12-03 18:38:49]

https://codereview.chromium.org/745733002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py
(right):

https://codereview.chromium.org/745733002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:253:
except Exception as e:
We should never catch Exception, and the try-block is probably too wide. Let's
make sure we're catching the minimal exceptions necessary with the smallest
try-clause.

https://codereview.chromium.org/745733002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:257:
logging.warning(
Should this and/or any of the other warnings actually be fatal? I'd much rather
fail hard with a descriptive message than try to limp along with a potential
misusage.

https://codereview.chromium.org/745733002/diff/1/tools/telemetry/telemetry/co...
File
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder_unittest.py
(right):

https://codereview.chromium.org/745733002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder_unittest.py:101:
def test_adb_one_device_with_exact_apk(self):
The style guide wants this to be named testAdbOneDeviceWithExactApk(). I see
that the other test names in this file are incorrect. Would you mind updating
them all while you're here?

https://codereview.chromium.org/745733002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py
(right):

https://codereview.chromium.org/745733002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py:161:
elif not normalized_executable.lower().endswith('.apk'):
Ideally, the browser finders wouldn't be aware of each other. I think we need a
better system for each finder to indicate whether it can handle the given path.

[mstrum, 2014-12-03 23:59:04]

PTAL

https://codereview.chromium.org/745733002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py
(right):

https://codereview.chromium.org/745733002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:253:
except Exception as e:
On 2014/12/03 18:38:48, tonyg wrote:
> We should never catch Exception, and the try-block is probably too wide. Let's
> make sure we're catching the minimal exceptions necessary with the smallest
> try-clause.

I've collapsed everything to be closer to the sources of error.

https://codereview.chromium.org/745733002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:257:
logging.warning(
On 2014/12/03 18:38:48, tonyg wrote:
> Should this and/or any of the other warnings actually be fatal? I'd much
rather
> fail hard with a descriptive message than try to limp along with a potential
> misusage.

Yes, having each finder explicitly only look at executables that they know they
can handle makes this warning unneeded and the ones above should fail fast.
Done.

https://codereview.chromium.org/745733002/diff/1/tools/telemetry/telemetry/co...
File
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder_unittest.py
(right):

https://codereview.chromium.org/745733002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder_unittest.py:101:
def test_adb_one_device_with_exact_apk(self):
On 2014/12/03 18:38:48, tonyg wrote:
> The style guide wants this to be named testAdbOneDeviceWithExactApk(). I see
> that the other test names in this file are incorrect. Would you mind updating
> them all while you're here?

Done.

https://codereview.chromium.org/745733002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py
(right):

https://codereview.chromium.org/745733002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py:161:
elif not normalized_executable.lower().endswith('.apk'):
On 2014/12/03 18:38:48, tonyg wrote:
> Ideally, the browser finders wouldn't be aware of each other. I think we need
a
> better system for each finder to indicate whether it can handle the given
path.

I agree, I went with more of a whitelist approach in the newest update.

[mstrum, 2014-12-08 18:01:19]

Monday ping :)

[mstrum, 2014-12-30 23:44:15]

aiolos, could you PTA(post-holiday)L, it seems the other two reviewers are OOTO.

android_browser_finder_unittest went through some large changes, so I've posted
a rebase that works against the tip of master.

[aiolos (Not reviewing), 2015-01-05 18:56:11]

https://codereview.chromium.org/745733002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/adb_commands.py (right):

https://codereview.chromium.org/745733002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/adb_commands.py:29: from pylib.utils
import apk_helper  # pylint: disable=F0401
Either you don't need the pylint disable here, or you should check that
apk_helper isn't None in GetPackageName before you try calling it's functions. I
think it's the latter, but you'll want to check with tonyg, dtu or another of
the owners to be sure.

https://codereview.chromium.org/745733002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py
(right):

https://codereview.chromium.org/745733002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:171:
raise Exception(
You should create an Error and raise that instead of the base Exception.

https://codereview.chromium.org/745733002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:187:
raise Exception(
Same as above.

https://codereview.chromium.org/745733002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py
(right):

https://codereview.chromium.org/745733002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py:95: if
not extension:
Is this to check if the path is correct for the given platform, you should be
checking that the platform is Linux/Mac for the first branch and Windows for the
second.

Otherwise, it's cleaner to just fail early:

if extension and extension != '.exe':
  return false
return true

[mstrum, 2015-01-06 18:45:25]

Addressed aiolos' comments, PTAL~

https://codereview.chromium.org/745733002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/adb_commands.py (right):

https://codereview.chromium.org/745733002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/adb_commands.py:29: from pylib.utils
import apk_helper  # pylint: disable=F0401
On 2015/01/05 18:56:11, aiolos wrote:
> Either you don't need the pylint disable here, or you should check that
> apk_helper isn't None in GetPackageName before you try calling it's functions.
I
> think it's the latter, but you'll want to check with tonyg, dtu or another of
> the owners to be sure.

From what I can tell, this is standard practice in this file.  Is there a reason
why apk_helper would be different than pylib.constants or pylib.android_commands
imported above?

https://codereview.chromium.org/745733002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py
(right):

https://codereview.chromium.org/745733002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:171:
raise Exception(
On 2015/01/05 18:56:11, aiolos wrote:
> You should create an Error and raise that instead of the base Exception.

Done.

https://codereview.chromium.org/745733002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:187:
raise Exception(
On 2015/01/05 18:56:11, aiolos wrote:
> Same as above.

Done.

https://codereview.chromium.org/745733002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py
(right):

https://codereview.chromium.org/745733002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py:95: if
not extension:
On 2015/01/05 18:56:11, aiolos wrote:
> Is this to check if the path is correct for the given platform, you should be
> checking that the platform is Linux/Mac for the first branch and Windows for
the
> second.
> 
> Otherwise, it's cleaner to just fail early:
> 
> if extension and extension != '.exe':
>   return false
> return true

Done.

[tonyg, 2015-01-06 23:06:26]

lgtm

https://codereview.chromium.org/745733002/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py
(right):

https://codereview.chromium.org/745733002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:77: #
Error if the path doesn't exist.
nit: I recommend removing this comment. It just re-iterates what the code says.
Comments ideally answer "why", not "what".

[mstrum, 2015-01-06 23:40:05]

https://codereview.chromium.org/745733002/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py
(right):

https://codereview.chromium.org/745733002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:77: #
Error if the path doesn't exist.
On 2015/01/06 23:06:26, tonyg wrote:
> nit: I recommend removing this comment. It just re-iterates what the code
says.
> Comments ideally answer "why", not "what".

Done.

[mstrum, 2015-01-06 23:40:50]

The CQ bit was checked by mstrum@amazon.com

[commit-bot: I haz the power, 2015-01-06 23:42:20]

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

[commit-bot: I haz the power, 2015-01-06 23:50:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-06 23:51:00]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mstrum, 2015-01-07 01:47:56]

On 2015/01/06 23:51:00, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

android_dbg_tests_recipe and win_chromium_rel_ng have unrelated errors.

chromium_presubmit has errors that I can repro in a local clean repo with a one
line white space change which would indicate to me that there might be a problem
somewhere in the presubmit code.

[mstrum, 2015-01-07 18:33:01]

It looks like nduca submitted a fix:

https://codereview.chromium.org/838443007

[mstrum, 2015-01-07 18:33:51]

The CQ bit was checked by mstrum@amazon.com

[commit-bot: I haz the power, 2015-01-07 18:35:00]

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

[commit-bot: I haz the power, 2015-01-07 20:10:31]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-01-07 20:11:27]

Patchset 7 (id:??) landed as
https://crrev.com/d0fa8f06d525c62db23b2bc65147d721b9999015
Cr-Commit-Position: refs/heads/master@{#310346}