Plumb Telemetry test name through to results object creation

Plumb Telemetry test name through to results object creation

We want to be able to ask for the current benchmark's name when generating results. This threads the benchmark through so that it will be available to results objects.

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

[nduca, 2014-07-11 18:57:33]

why not make the test object available to the reuslts object upon construction?

[eakuefner, 2014-07-11 20:31:55]

On 2014/07/11 18:57:33, nduca wrote:
> why not make the test object available to the reuslts object upon
construction?

Done.

[nduca, 2014-07-11 20:39:21]

https://codereview.chromium.org/386943007/diff/1/tools/telemetry/telemetry/pa...
File tools/telemetry/telemetry/page/page_runner.py (right):

https://codereview.chromium.org/386943007/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:342: results.page_test_name =
test.Name() + '.' + page_set.Name()
only test.name btw.... there's subtlety here

https://codereview.chromium.org/386943007/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/page_measurement_results.py (right):

https://codereview.chromium.org/386943007/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_measurement_results.py:29:
self.page_test_name = self._test.Name() + '.' + self._page_set.Name()
no... dont do this. page test name doesn't have to be computed except by a
results object that needs it. do it only your json results object.

and btw, this isn't the page test name you formed by catenation. its the test
name. the page test name is the measurement. please study this in a lot more
detail. the name recorded as **test name** must match the name passed into run
benchmark.

do not call this the page test name. page test is a different class and not what
is being named. this is the test name.

[eakuefner, 2014-07-11 21:25:59]

https://codereview.chromium.org/386943007/diff/1/tools/telemetry/telemetry/pa...
File tools/telemetry/telemetry/page/page_runner.py (right):

https://codereview.chromium.org/386943007/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:342: results.page_test_name =
test.Name() + '.' + page_set.Name()
On 2014/07/11 20:39:21, nduca wrote:
> only test.name btw.... there's subtlety here

I looked into this and it doesn't appear that tests have a name field. I ran
several benchmarks using pdb to look for it and investigated and there doesn't
appear to be a name field anywhere in the hierarchy. The Name() call seems to
come from command_line.Command, and it seems to perform a bit of mangling on the
name of the class and return that.

Am I missing something?

https://codereview.chromium.org/386943007/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/page_measurement_results.py (right):

https://codereview.chromium.org/386943007/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_measurement_results.py:29:
self.page_test_name = self._test.Name() + '.' + self._page_set.Name()
On 2014/07/11 20:39:21, nduca wrote:
> no... dont do this. page test name doesn't have to be computed except by a
> results object that needs it. do it only your json results object.

Removed the code from here, and also the code to pass test and page_set to the
existing results objects on construction.

> and btw, this isn't the page test name you formed by catenation. its the test
> name. the page test name is the measurement. please study this in a lot more
> detail. the name recorded as **test name** must match the name passed into run
> benchmark.

I'm sorry I've been slow to understand this. Thanks for letting me know about
the confusion. I've looked into this and I hope I understand better now. Here is
my best effort at explaining what I think the picture looks like:

A page measurement is a particular kind of page test. A page set, as the name
suggests, is a collection of pages to be tested. A benchmark or test (as best I
understand these are synonymous) is a page test (or page measurement in many
cases, e.g. smoothness) together with a page set. The name of the
test/benchmark, then, is page_test_name.page_set_name, where page_test_name and
page_set_name stand in for the names of the page test and page set that comprise
the test/benchmark respectively.

Is my understanding accurate?

[eakuefner, 2014-07-15 22:37:36]

PTAL. New approach that lifts logic for creating results objects and makes
results constructors take a benchmark object. Tests don't pass but I need
feedback on the approach before continuing.

One particular issue I want feedback on: is it appropriate/reasonable to have
results/dummy_benchmark_for_test.py as I do? There are two problems I want to
solve for testing but am not sure how: 1. having a dummy benchmark available for
unittests and 2. having a dummy results object available that refers to this
dummy benchmark, since results objects must as of this patch mandatorily refer
to a benchmark. The way I've done that now is my first approach, but I'm sure it
could be improved.

[nduca, 2014-07-15 23:10:36]

https://codereview.chromium.org/386943007/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/386943007/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/benchmark.py:65: def Run(self, args):
rename args finder_options

https://codereview.chromium.org/386943007/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/benchmark.py:69: test = self.PageTestClass()()
test->page_test

https://codereview.chromium.org/386943007/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/benchmark.py:82: results =
results_options.PrepareResults(test, args, self)
Please rename to CreateResults()

https://codereview.chromium.org/386943007/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/benchmark.py:165: 
put it in the right place, alphabetically

@property
def metadata(self):
  return BenchmarkMetadata(self.name)

in this file, above benchmark, put
  class BenchmarkMetadata():
    only has name, but we can add description, how to run etc here

https://codereview.chromium.org/386943007/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_runner.py (right):

https://codereview.chromium.org/386943007/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:338: def Run(test, page_set,
expectations, finder_options, results):
test->page_test


argument order should be (page_test, page_set, finder_options, expectations,
results)

https://codereview.chromium.org/386943007/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/record_wpr.py (right):

https://codereview.chromium.org/386943007/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/record_wpr.py:81: dummy_results =
page_measurement_results.PageMeasurementResults(
results.benchmark

https://codereview.chromium.org/386943007/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/record_wpr.py:100: recorder =
RecordPage(measurements)
RecordPage->RecordPageTest. recorder->record_page_test

make a record benchmark here, use it below. But, put a comment on this line
 # Note: this code should not be manually instantiate RecordPageTest. Ideally,
 # it would create the RecordBenchmark and ask the benchmark to create the test.
However,
 # the way benchmark creation is structured, you cannot communicate runtime
information to the
 # benchmarks' page_test. Hence, this hack.

https://codereview.chromium.org/386943007/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/record_wpr.py:135: results =
page_runner.Run(recorder, ps, expectations, options)
this call site needs to be updated.

the problem here is that we dont actually make a benchmark...

https://codereview.chromium.org/386943007/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/results_options.py (right):

https://codereview.chromium.org/386943007/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/results_options.py:46: def
PrepareResults(test, options, benchmark):
rename method to CreateResults

rename to finder options

(test, options, benchmark) -> (benchmark, page_test, options)

https://codereview.chromium.org/386943007/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/results_options.py:46: def
PrepareResults(test, options, benchmark):
benchmark, page_test, options

[eakuefner, 2014-08-04 19:40:11]

PTAL: hopefully we are finally on the right track thanks to the page_runner
refactor by Chris.

[nduca, 2014-08-04 19:47:17]

https://codereview.chromium.org/386943007/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/record_wpr.py (right):

https://codereview.chromium.org/386943007/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/record_wpr.py:147: benchmark_metadata =
benchmark.BenchmarkMetadata('')
record_wpr and ditch the todo?

https://codereview.chromium.org/386943007/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/results_options.py (right):

https://codereview.chromium.org/386943007/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/results_options.py:90: output_stream,
benchmark_name, options.reset_results,
i'd suggest passing the metadata all the way through
and might as well push to json too

[eakuefner, 2014-08-04 20:03:57]

https://codereview.chromium.org/386943007/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/record_wpr.py (right):

https://codereview.chromium.org/386943007/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/record_wpr.py:147: benchmark_metadata =
benchmark.BenchmarkMetadata('')
On 2014/08/04 19:47:17, nduca wrote:
> record_wpr and ditch the todo?

Done.

https://codereview.chromium.org/386943007/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/results_options.py (right):

https://codereview.chromium.org/386943007/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/results_options.py:90: output_stream,
benchmark_name, options.reset_results,
On 2014/08/04 19:47:17, nduca wrote:
> i'd suggest passing the metadata all the way through
> and might as well push to json too

Done.

[nduca, 2014-08-04 20:05:52]

lgtm but no fake plz

https://codereview.chromium.org/386943007/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/results/json_output_formatter_unittest.py
(right):

https://codereview.chromium.org/386943007/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/results/json_output_formatter_unittest.py:36:
self._formatter = json_output_formatter.JsonOutputFormatter(self._output,
do you need to fake? i think you can just do BenchmarkMetadata()... faking is
only if you need a different behavior

[eakuefner, 2014-08-04 20:13:07]

https://codereview.chromium.org/386943007/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/results/json_output_formatter_unittest.py
(right):

https://codereview.chromium.org/386943007/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/results/json_output_formatter_unittest.py:36:
self._formatter = json_output_formatter.JsonOutputFormatter(self._output,
On 2014/08/04 20:05:52, nduca wrote:
> do you need to fake? i think you can just do BenchmarkMetadata()... faking is
> only if you need a different behavior

Done.

[eakuefner, 2014-08-04 20:13:13]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2014-08-04 20:13:18]

The CQ bit was unchecked by eakuefner@chromium.org

[eakuefner, 2014-08-04 23:52:44]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2014-08-04 23:54:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eakuefner@chromium.org/386943007/120001

[commit-bot: I haz the power, 2014-08-05 03:27:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-05 07:50:58]

Change committed as 287483