Android: first step into making "perf tests" reuse the functional test infra.

Android: first step into making "perf tests" reuse the functional test infra.

Perf tests are currently using build/android/bb_run_sharded_steps.py,
which lacks a lot of functionality to retry on different devices on failure,
plus it adds an entirely separate hierarchy for running tests.
This patch is the first step to deprecate it and reuse the same mechanism
as all other functional tests.

BUG=270165, 268450
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217550

[bulach, 2013-08-12 15:28:38]

frankf, ptal (tonyg@, just FYI).

some quick background:
* sharding for perf tests happens in two separate steps:
1. a single build step that calls into bb_run_sharded_steps.. this step will run
all perf tests across all devices, and save the result to out/step_results..
there's no log parsing at this stage.

2. multiple build steps, one per perf tests.. at this stage, we just "print" /
"cat" the result from above, and the buildbot parses and uploads to the
dashboard...

One day, this will be simplified. Today, is not that day. :)


So here's the plan:
1. introduce these new "perf test runners", which reuses the same mechanism as
the functional tests... We'll get retry for free, and perhaps even the flakyness
dashboard later on?
They still need this "two stage" split for running and printing the results.

2. change bb_run_sharded_steps implementation so it just calls:
python build/android/test_runner.py perf --steps ${foobar}
and
python build/android/test_runner.py perf --print-step ${foobar}

3. once it all works, replace calls in the buildbot side from
bb_run_sharded_steps to directly test_runner.py..

4. delete bb_run_sharded_steps..

thanks!

[frankf, 2013-08-12 23:09:52]

Thanks Marcus, this is much cleaner.

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/setu...
File build/android/pylib/perf/setup.py (right):

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/setu...
build/android/pylib/perf/setup.py:22: pids = cmd_helper.GetCmdOutput(['pgrep',
'-f', server])
Can you use psutil: http://stackoverflow.com/a/4230226

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/setu...
build/android/pylib/perf/setup.py:32: cmd_helper.RunCmd(['taskset', '-c', '0',
'adb', 'start-server'])
Are we going to eventually do this in general for other test types?

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/setu...
build/android/pylib/perf/setup.py:42: if not
ports.ResetTestServerPortAllocation():
Currently we call this from bunch of places:
For gtests: gtest/setup.py
For instrumentation tests: test_runner.py
And now here for perf.

Can we unify these into one call in test_runner.py?

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/setu...
build/android/pylib/perf/setup.py:45: forwarder.Forwarder.UseMultiprocessing()
This is now multi-threading. Can we get rid of this?

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/setu...
build/android/pylib/perf/setup.py:78: return (TestRunnerFactory,
sorted(tests.keys()))
Any reasons to sort the tests given you don't sort the devices anymore?

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
File build/android/pylib/perf/test_runner.py (right):

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:15: --flaky_steps.
flaky_steps -> flaky-steps

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:57: class
TestPrinter(base_test_runner.BaseTestRunner):
docstring here and below

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:64: print 'File not found ', file_name
Use logging instead of print?

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:66: return
base_test_result.TestRunResults(file_name, fail), False
file_name -> test_name?

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:66: return
base_test_result.TestRunResults(file_name, fail), False
False -> None

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:73: result =
base_test_result.BaseTestResult(test_name,
You can get rid of intermediate var

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:76: return results, False
None

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:82: def __init__(self, test_options,
device, shard_index, tests, flaky_tests):
add a docstring with Args section

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:94: """Runs perf test.
a perf test

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:95: 
Args section

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:100: ' --keep_test_server_ports')
Use string formatting instead concat

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:102: output, exit_code  = pexpect.run(
one space before =

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:111: exit_code = 0
Do you want to at least log the real exit code?

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:137: result =
base_test_result.BaseTestResult(test_name, result_type)
same here with intermediate var

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:140: if result_type !=
base_test_result.ResultType.PASS:
if not results.DidRunPass()

https://codereview.chromium.org/22854004/diff/1/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/22854004/diff/1/build/android/test_runner.py#...
build/android/test_runner.py:429: '%prog perf')
This is incomplete

https://codereview.chromium.org/22854004/diff/1/build/android/test_runner.py#...
build/android/test_runner.py:455: 
Nit: verify one of {--steps, --print-step} is specified

[frankf, 2013-08-13 00:43:11]

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/setu...
File build/android/pylib/perf/setup.py (right):

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/setu...
build/android/pylib/perf/setup.py:60: return
test_runner.TestPrinter(test_options, device, shard_index)
Also, it's really weird to have the printing logic use the test runner as it's
purely a host-side operation. Can you let _RunPerfTests in test_runner.py decide
what to do?

[bulach, 2013-08-13 08:58:19]

thanks frank! this is indeed much cleaner, I'm very glad to reuse all the retry
and sharding logic you guys implemented, very cool! :)

almost all comments addressed except one that would require changing existing
harness, which I think can be done in a separate step. ptal.

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/setu...
File build/android/pylib/perf/setup.py (right):

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/setu...
build/android/pylib/perf/setup.py:22: pids = cmd_helper.GetCmdOutput(['pgrep',
'-f', server])
On 2013/08/12 23:09:52, frankf wrote:
> Can you use psutil: http://stackoverflow.com/a/4230226

Done.

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/setu...
build/android/pylib/perf/setup.py:32: cmd_helper.RunCmd(['taskset', '-c', '0',
'adb', 'start-server'])
On 2013/08/12 23:09:52, frankf wrote:
> Are we going to eventually do this in general for other test types?

potentially, yes.. but maybe best to start here, then generalize it later?

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/setu...
build/android/pylib/perf/setup.py:42: if not
ports.ResetTestServerPortAllocation():
On 2013/08/12 23:09:52, frankf wrote:
> Currently we call this from bunch of places:
> For gtests: gtest/setup.py
> For instrumentation tests: test_runner.py
> And now here for perf.
> 
> Can we unify these into one call in test_runner.py?

since there's already two other places, would you mind this refactoring as a
follow up?
reason being that this patch will only run downstream, it'd be nice to ensure
this biggie won't affect any other bot... then shortly after this patch
"sticks", I'm happy to do the refactoring across the board.. wdyt?

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/setu...
build/android/pylib/perf/setup.py:45: forwarder.Forwarder.UseMultiprocessing()
On 2013/08/12 23:09:52, frankf wrote:
> This is now multi-threading. Can we get rid of this?

unfortunately not, it's multi-threading only on the first level.. :)
internally, test_runner.py is then calling subprocess.Popen to run the commands
(i.e., telemetry, or downstream startup tests), which then need this envvar to
setup the forwarder accordingly.

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/setu...
build/android/pylib/perf/setup.py:60: return
test_runner.TestPrinter(test_options, device, shard_index)
On 2013/08/13 00:43:12, frankf wrote:
> Also, it's really weird to have the printing logic use the test runner as it's
> purely a host-side operation. Can you let _RunPerfTests in test_runner.py
decide
> what to do?

yeah, I fully agree this is a bizarre shoehorning... :(
moving it up to _RunPerfTests is a good idea! done.

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/setu...
build/android/pylib/perf/setup.py:78: return (TestRunnerFactory,
sorted(tests.keys()))
On 2013/08/12 23:09:52, frankf wrote:
> Any reasons to sort the tests given you don't sort the devices anymore?

that's actually a good point. I added "sorted" in
test_dispatcher.py::_GetAttachedDevices.. I think it won't hurt, and will keep
things a bit more deterministic.

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
File build/android/pylib/perf/test_runner.py (right):

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:15: --flaky_steps.
On 2013/08/12 23:09:52, frankf wrote:
> flaky_steps -> flaky-steps

Done.

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:57: class
TestPrinter(base_test_runner.BaseTestRunner):
On 2013/08/12 23:09:52, frankf wrote:
> docstring here and below

Done.

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:64: print 'File not found ', file_name
On 2013/08/12 23:09:52, frankf wrote:
> Use logging instead of print?

Done.

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:73: result =
base_test_result.BaseTestResult(test_name,
On 2013/08/12 23:09:52, frankf wrote:
> You can get rid of intermediate var

Done.

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:76: return results, False
On 2013/08/12 23:09:52, frankf wrote:
> None

by moving this up to _RunPerfTests, this can return the exit_code directly,
which addressed the past 5 comments. :)

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:82: def __init__(self, test_options,
device, shard_index, tests, flaky_tests):
On 2013/08/12 23:09:52, frankf wrote:
> add a docstring with Args section

Done.

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:94: """Runs perf test.
On 2013/08/12 23:09:52, frankf wrote:
> a perf test

Done.

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:95: 
On 2013/08/12 23:09:52, frankf wrote:
> Args section

Done.

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:100: ' --keep_test_server_ports')
On 2013/08/12 23:09:52, frankf wrote:
> Use string formatting instead concat

Done.

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:102: output, exit_code  = pexpect.run(
On 2013/08/12 23:09:52, frankf wrote:
> one space before =

Done.

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:111: exit_code = 0
On 2013/08/12 23:09:52, frankf wrote:
> Do you want to at least log the real exit code?

yeah, good point. there were a few other things bb_sharded_steps was logging, so
adding those back here.

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:137: result =
base_test_result.BaseTestResult(test_name, result_type)
On 2013/08/12 23:09:52, frankf wrote:
> same here with intermediate var

Done.

https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/test...
build/android/pylib/perf/test_runner.py:140: if result_type !=
base_test_result.ResultType.PASS:
On 2013/08/12 23:09:52, frankf wrote:
> if not results.DidRunPass()

Done.

https://codereview.chromium.org/22854004/diff/1/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/22854004/diff/1/build/android/test_runner.py#...
build/android/test_runner.py:429: '%prog perf')
On 2013/08/12 23:09:52, frankf wrote:
> This is incomplete

Done.

https://codereview.chromium.org/22854004/diff/1/build/android/test_runner.py#...
build/android/test_runner.py:455: 
On 2013/08/12 23:09:52, frankf wrote:
> Nit: verify one of {--steps, --print-step} is specified

Done.

[craigdh, 2013-08-13 16:58:46]

https://codereview.chromium.org/22854004/diff/8001/build/android/pylib/perf/s...
File build/android/pylib/perf/setup.py (right):

https://codereview.chromium.org/22854004/diff/8001/build/android/pylib/perf/s...
build/android/pylib/perf/setup.py:30: # Restart the adb server with taskset to
set a single CPU affinity.
This seems widely applicable. Should this be done for all types of tests?

[bulach, 2013-08-13 17:06:52]

https://codereview.chromium.org/22854004/diff/8001/build/android/pylib/perf/s...
File build/android/pylib/perf/setup.py (right):

https://codereview.chromium.org/22854004/diff/8001/build/android/pylib/perf/s...
build/android/pylib/perf/setup.py:30: # Restart the adb server with taskset to
set a single CPU affinity.
On 2013/08/13 16:58:46, craigdh wrote:
> This seems widely applicable. Should this be done for all types of tests?

yes, just not yet. :)
the main goal of this patch is to start converging the perf tests with
functional tests, and at this stage, I'd like to keep changes to other harnesses
to a minimum, if that's ok?
as soon as this lands, there are a few refactorings that will be done, like this
one as well:
https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/setu...

the main problem is that the perf tests will only be exercised downstream, so
I'd like this patch to stick :)

then it'll be much easier to mess around with other types of tests and change
their implementation, as long as we never need to revert the entire perf
runner... wdyt?

(oh, and while at it: ulan@ from the v8 team, says that this adb trick helped
his tests too, on a completely different environment / harness / buildbots /
etc.., so indeed, this should be shared across all our tests..).

[frankf, 2013-08-13 18:55:02]

lgtm with nit

https://codereview.chromium.org/22854004/diff/8001/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/22854004/diff/8001/build/android/test_runner....
build/android/test_runner.py:605: tests, runner_factory, False, None,
shard=True)
Disable timeouts (default is 7 minutes): test_timeout=None

[craigdh, 2013-08-13 19:03:45]

https://codereview.chromium.org/22854004/diff/8001/build/android/pylib/perf/s...
File build/android/pylib/perf/setup.py (right):

https://codereview.chromium.org/22854004/diff/8001/build/android/pylib/perf/s...
build/android/pylib/perf/setup.py:30: # Restart the adb server with taskset to
set a single CPU affinity.
On 2013/08/13 17:06:53, bulach wrote:
> On 2013/08/13 16:58:46, craigdh wrote:
> > This seems widely applicable. Should this be done for all types of tests?
> 
> yes, just not yet. :)
> the main goal of this patch is to start converging the perf tests with
> functional tests, and at this stage, I'd like to keep changes to other
harnesses
> to a minimum, if that's ok?
> as soon as this lands, there are a few refactorings that will be done, like
this
> one as well:
>
https://codereview.chromium.org/22854004/diff/1/build/android/pylib/perf/setu...
> 
> the main problem is that the perf tests will only be exercised downstream, so
> I'd like this patch to stick :)
> 
> then it'll be much easier to mess around with other types of tests and change
> their implementation, as long as we never need to revert the entire perf
> runner... wdyt?
> 
> (oh, and while at it: ulan@ from the v8 team, says that this adb trick helped
> his tests too, on a completely different environment / harness / buildbots /
> etc.., so indeed, this should be shared across all our tests..).

Sounds most excellent.

[bulach, 2013-08-14 08:14:12]

thanks both! all addressed, CQing.

I'll start ripping out bb_sharded_steps.py to just delegate to this one, then
move the bots to call this directly..

in parallel, I'll do the other refactoring as suggested.

https://codereview.chromium.org/22854004/diff/8001/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/22854004/diff/8001/build/android/test_runner....
build/android/test_runner.py:605: tests, runner_factory, False, None,
shard=True)
On 2013/08/13 18:55:02, frankf wrote:
> Disable timeouts (default is 7 minutes): test_timeout=None

Done.

[commit-bot: I haz the power, 2013-08-14 08:15:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/22854004/22001

[commit-bot: I haz the power, 2013-08-14 08:30:51]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-08-14 10:09:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/22854004/37001

[commit-bot: I haz the power, 2013-08-14 14:39:52]

Change committed as 217550