[telemetry] test_runner and run_benchmarks

[telemetry] test_runner and run_benchmarks

- test.Test, which encapsulates a test and page set, and describes how to run it.
- test_runner, which runs test.Tests using a command-based CLI.

- benchmarks dir, which contains a list of test.Tests.
- run_measurement, which just calls test_runner.Main().


BUG=237412
TEST=None.
R=kbr@chromium.org, nduca@chromium.org, tonyg@chromium.org

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

[nduca, 2013-06-18 23:52:23]

this looks pretty damn good
im thinking maybe we land without having it all polished so we can iterate a few
times, assuming tonyg is okay with such a plan...

https://chromiumcodereview.appspot.com/17438002/diff/1001/tools/perf/benchmar...
File tools/perf/benchmarks/benchmarks.py (right):

https://chromiumcodereview.appspot.com/17438002/diff/1001/tools/perf/benchmar...
tools/perf/benchmarks/benchmarks.py:13: class Smoothness(test.TelemetryTest):
This is SmoothnessTop25 no?

https://chromiumcodereview.appspot.com/17438002/diff/1001/tools/perf/benchmar...
tools/perf/benchmarks/benchmarks.py:21: test = measurements.sunspider.Sunspider
can you blow this file out to be one class per file? so sunspider.py and
smoothness_benchmarks.py?

Then move measurement/sunspider.py into this file? Its not really a measurement,
long term I think...

[dtu, 2013-06-19 01:00:50]

https://chromiumcodereview.appspot.com/17438002/diff/1001/tools/perf/benchmar...
File tools/perf/benchmarks/benchmarks.py (right):

https://chromiumcodereview.appspot.com/17438002/diff/1001/tools/perf/benchmar...
tools/perf/benchmarks/benchmarks.py:13: class Smoothness(test.TelemetryTest):
On 2013/06/18 23:52:23, nduca wrote:
> This is SmoothnessTop25 no?

Done.

https://chromiumcodereview.appspot.com/17438002/diff/1001/tools/perf/benchmar...
tools/perf/benchmarks/benchmarks.py:21: test = measurements.sunspider.Sunspider
On 2013/06/18 23:52:23, nduca wrote:
> can you blow this file out to be one class per file? so sunspider.py and
> smoothness_benchmarks.py?
> 
> Then move measurement/sunspider.py into this file? Its not really a
measurement,
> long term I think...

Done.

[nduca, 2013-06-19 05:46:53]

lgtm but a variety of nits

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
File tools/telemetry/telemetry/test.py (right):

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
tools/telemetry/telemetry/test.py:9: class Test(object):
can we just kill this and the more constrained test? I'm not too wild about
arbitrary Run() as a base class, unless there's something I'm missing?

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
tools/telemetry/telemetry/test.py:16: enabled = True
why the python1 style class definition here? We typically do __init__ with
setters even for defaults.

same goes in the benchmarks folder.

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
File tools/telemetry/telemetry/test_runner.py (right):

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
tools/telemetry/telemetry/test_runner.py:53: 
lets get a --json version of this in here quick so that people aren't tempted to
parse it...

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
tools/telemetry/telemetry/test_runner.py:77: def Run(self, test_names, options):
bit awk that you have test_names here instead of args. I had to read the lower
code to realize where was just the args...

also, optparse is always options, args = parser.parse_args() --- eg options
comes first. Maybe swap order throughout to be normative? Minor nit.

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
tools/telemetry/telemetry/test_runner.py:78: if not test_names:
maybe we should parser.error in this case. i think running all benchmarks is
possibly more than someone might intend...

[dtu, 2013-06-19 06:07:10]

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
File tools/telemetry/telemetry/test.py (right):

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
tools/telemetry/telemetry/test.py:9: class Test(object):
On 2013/06/19 05:46:53, nduca wrote:
> can we just kill this and the more constrained test? I'm not too wild about
> arbitrary Run() as a base class, unless there's something I'm missing?

That's fine. So we don't want to run any non-Telemetry tests?

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
tools/telemetry/telemetry/test.py:16: enabled = True
On 2013/06/19 05:46:53, nduca wrote:
> why the python1 style class definition here? We typically do __init__ with
> setters even for defaults.
> 
> same goes in the benchmarks folder.

It's more concise. Also you can get the test's attributes without instantiating
it. Is this discouraged/deprecated in Python?

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
File tools/telemetry/telemetry/test_runner.py (right):

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
tools/telemetry/telemetry/test_runner.py:77: def Run(self, test_names, options):
On 2013/06/19 05:46:53, nduca wrote:
> bit awk that you have test_names here instead of args. I had to read the lower
> code to realize where was just the args...
> 
> also, optparse is always options, args = parser.parse_args() --- eg options
> comes first. Maybe swap order throughout to be normative? Minor nit.

Ugh, okay. It was like this because positional args always come before keyword
args.

[tonyg, 2013-06-19 16:40:15]

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/perf/benchmar...
File tools/perf/benchmarks/smoothness_benchmarks.py (right):

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/perf/benchmar...
tools/perf/benchmarks/smoothness_benchmarks.py:1: # Copyright (c) 2013 The
Chromium Authors. All rights reserved.
Should this file just be called "smoothness.py" since the directory name already
implies benchmarks?

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/perf/benchmar...
File tools/perf/benchmarks/sunspider.py (right):

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/perf/benchmar...
tools/perf/benchmarks/sunspider.py:14: class
SunspiderMeasurement(page_measurement.PageMeasurement):
Does run_measurement sunspider still work (at least temporarily) after this
patch? We need to keep the bots happy during the transition.

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/perf/benchmar...
tools/perf/benchmarks/sunspider.py:48: 'serving_dirs': [''],
Can serving_dirs be omitted?

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/perf/run_benc...
File tools/perf/run_benchmarks (right):

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/perf/run_benc...
tools/perf/run_benchmarks:1: #!/usr/bin/env python
Should this file be called run_benchmark instead of run_benchmarks?

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/perf/run_benc...
tools/perf/run_benchmarks:8:
sys.path.append(os.path.join(os.path.dirname(__file__), os.pardir, 'telemetry'))
Is this core.GetTelemetryDir()?

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
File tools/telemetry/telemetry/test.py (right):

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
tools/telemetry/telemetry/test.py:14: class TelemetryTest(Test):
In light of our new documentation efforts, these key classes and methods should
probably have pydocs.

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
File tools/telemetry/telemetry/test_runner.py (right):

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
tools/telemetry/telemetry/test_runner.py:7: actually running the test is in
PageRunner."""
Recommend line breaks around the docstring

[dtu, 2013-06-19 21:22:57]

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/perf/benchmar...
File tools/perf/benchmarks/smoothness_benchmarks.py (right):

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/perf/benchmar...
tools/perf/benchmarks/smoothness_benchmarks.py:1: # Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2013/06/19 16:40:15, tonyg wrote:
> Should this file just be called "smoothness.py" since the directory name
already
> implies benchmarks?

Done.

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/perf/benchmar...
File tools/perf/benchmarks/sunspider.py (right):

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/perf/benchmar...
tools/perf/benchmarks/sunspider.py:14: class
SunspiderMeasurement(page_measurement.PageMeasurement):
On 2013/06/19 16:40:15, tonyg wrote:
> Does run_measurement sunspider still work (at least temporarily) after this
> patch? We need to keep the bots happy during the transition.

Done. I re-added the existing measurements to perf_tools/. New measurements
should go in measurements/.

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/perf/benchmar...
tools/perf/benchmarks/sunspider.py:48: 'serving_dirs': [''],
On 2013/06/19 16:40:15, tonyg wrote:
> Can serving_dirs be omitted?

No, otherwise it will only serve data/sunspider/sunspider-1.0/, and not the
dependencies in data/sunspider/.

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/perf/run_benc...
File tools/perf/run_benchmarks (right):

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/perf/run_benc...
tools/perf/run_benchmarks:1: #!/usr/bin/env python
On 2013/06/19 16:40:15, tonyg wrote:
> Should this file be called run_benchmark instead of run_benchmarks?

Shrug, could go either way. It can run both a single benchmark or multiple
benchmarks.

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/perf/run_benc...
tools/perf/run_benchmarks:8:
sys.path.append(os.path.join(os.path.dirname(__file__), os.pardir, 'telemetry'))
On 2013/06/19 16:40:15, tonyg wrote:
> Is this core.GetTelemetryDir()?

Yes, but we need to do this first in order to import core :)

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
File tools/telemetry/telemetry/test.py (right):

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
tools/telemetry/telemetry/test.py:9: class Test(object):
On 2013/06/19 06:07:10, Dave Tu wrote:
> On 2013/06/19 05:46:53, nduca wrote:
> > can we just kill this and the more constrained test? I'm not too wild about
> > arbitrary Run() as a base class, unless there's something I'm missing?
> 
> That's fine. So we don't want to run any non-Telemetry tests?

Done.

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
tools/telemetry/telemetry/test.py:14: class TelemetryTest(Test):
On 2013/06/19 16:40:15, tonyg wrote:
> In light of our new documentation efforts, these key classes and methods
should
> probably have pydocs.

Done.

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
File tools/telemetry/telemetry/test_runner.py (right):

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
tools/telemetry/telemetry/test_runner.py:7: actually running the test is in
PageRunner."""
On 2013/06/19 16:40:15, tonyg wrote:
> Recommend line breaks around the docstring

Done.

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
tools/telemetry/telemetry/test_runner.py:53: 
On 2013/06/19 05:46:53, nduca wrote:
> lets get a --json version of this in here quick so that people aren't tempted
to
> parse it...

Done.

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
tools/telemetry/telemetry/test_runner.py:77: def Run(self, test_names, options):
On 2013/06/19 06:07:10, Dave Tu wrote:
> On 2013/06/19 05:46:53, nduca wrote:
> > bit awk that you have test_names here instead of args. I had to read the
lower
> > code to realize where was just the args...
> > 
> > also, optparse is always options, args = parser.parse_args() --- eg options
> > comes first. Maybe swap order throughout to be normative? Minor nit.
> 
> Ugh, okay. It was like this because positional args always come before keyword
> args.

Done.

https://chromiumcodereview.appspot.com/17438002/diff/6001/tools/telemetry/tel...
tools/telemetry/telemetry/test_runner.py:78: if not test_names:
On 2013/06/19 05:46:53, nduca wrote:
> maybe we should parser.error in this case. i think running all benchmarks is
> possibly more than someone might intend...

Done.

[tonyg, 2013-06-19 21:36:10]

Dave and I talked offline about finding a way to avoid duplicating the sunspider
benchmark/measurement files for the interim. Everything else lg.

[nduca, 2013-06-19 22:04:02]

i'd go with run_benchmark

in my review notes, i took issue with being able to run mulitple benchmarks at
once. do we have any strong reason to run multiple at once?

[dtu, 2013-06-19 22:08:48]

On 2013/06/19 22:04:02, nduca wrote:
> i'd go with run_benchmark
> 
> in my review notes, i took issue with being able to run mulitple benchmarks at
> once. do we have any strong reason to run multiple at once?

No, other than convenience.

[dtu, 2013-06-19 22:32:12]

On 2013/06/19 21:36:10, tonyg wrote:
> Dave and I talked offline about finding a way to avoid duplicating the
sunspider
> benchmark/measurement files for the interim. Everything else lg.

How about this? The measurement just creates an instance of the Test to call its
CreatePageSet().
And adjust the discovery to search for measurements in all of tools/perf/, to
include measurements/ and benchmarks/.

[nduca, 2013-06-19 22:32:46]

Hmmm

lets lets file a bug for "revisit where waterfall configuration parameters are
stored with run_benchmarks" --- right now we do it in the buildbot config files,
which means we have to do the configuration twice, possibly three times, one for
chromium.perf, once for cros perf, and once for clank downstream.

Might be better if we could do something like @RunOnWaterfall('chromium.perf')
around Benchmarks that did this. Then the buildbot would do something like
run_benchmark list --waterfall-filter=chromium.perf and, then do run_benchmark
on what it finds...

[nduca, 2013-06-19 22:37:34]

Can we just make run_measurement hand-recognize the not-a-measurement-anymore
class and handle it as a call to run_benchmark plus warning? We've got a few
workarounds in that file already. The idea being, this doesn't need to be
supported long term, right? Just for the duration where buildbot points at the
old name?

[dtu, 2013-06-20 22:20:11]

On 2013/06/19 22:37:34, nduca wrote:
> Can we just make run_measurement hand-recognize the not-a-measurement-anymore
> class and handle it as a call to run_benchmark plus warning? We've got a few
> workarounds in that file already. The idea being, this doesn't need to be
> supported long term, right? Just for the duration where buildbot points at the
> old name?

Okay, I undid PS4, and modified page_test_runner/run_measurements/run_gpu_tests
to run both PageTests and test.Tests. If there's a name conflict, it'll pick the
test.Test. (e.g. smoothness -> measurements.Smoothness; smoothness_top25 ->
benchmarks.SmoothnessTop25; sunspider -> benchmarks.Sunspider)

I'll need OWNERs for run_gpu_tests.

[nduca, 2013-06-20 22:24:23]

lgtm if needed

[dtu, 2013-06-20 22:33:14]

+kbr OWNER for content/test/gpu

[tonyg, 2013-06-21 03:47:47]

lgtm2

[Ken Russell (switch to Gerrit), 2013-06-21 15:42:31]

content/test/gpu LGTM

[dtu, 2013-06-21 17:41:23]

Committed patchset #6 manually as r207874 (presubmit successful).