Introduce OutputFormatter interface and wire it through PageTestResults.

Make --output-format=csv uses the new OutputFormatter instead of
subclassing PageMeasurementResults.

BUG=346956

Review URL: https://codereview.chromium.org/416993008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@285855 0039d316-1c4b-4281-b951-d872f2087c98

tools/telemetry/telemetry/results/csv_page_measurement_results.py → tools/telemetry/telemetry/results/csv_output_formatter.py

@@ -4,30 +4,27 @@
 
 import csv
 
-from telemetry.results import page_measurement_results
+from telemetry.results import output_formatter
 from telemetry.value import merge_values
 
 
-class CsvPageMeasurementResults(
-    page_measurement_results.PageMeasurementResults):
+class CsvOutputFormatter(output_formatter.OutputFormatter):
   def __init__(self, output_stream):
-    super(CsvPageMeasurementResults, self).__init__(output_stream)
-    self._results_writer = csv.writer(self._output_stream)
-    self._representative_value_names = None
+    super(CsvOutputFormatter, self).__init__(output_stream)
 
-  def PrintSummary(self):
-    try:
-      values = merge_values.MergeLikeValuesFromSamePage(
-          self.all_page_specific_values)
-      self._OutputHeader(values)
-      value_groups_by_page = merge_values.GroupStably(
-          values, lambda value: value.page.url)
-      for values_for_page in value_groups_by_page:
-        self._OutputValuesForPage(values_for_page[0].page, values_for_page)
-    finally:
-      super(CsvPageMeasurementResults, self).PrintSummary()
+  def Format(self, page_test_results):
+    values = merge_values.MergeLikeValuesFromSamePage(
+        page_test_results.all_page_specific_values)
+    writer = csv.writer(self.output_stream)
+    header_value_names = self._OutputHeader(values, writer)
+    value_groups_by_page = merge_values.GroupStably(
+        values, lambda value: value.page.url)
+    for values_for_page in value_groups_by_page:
+      self._OutputValuesForPage(
+          header_value_names, values_for_page[0].page, values_for_page,
+          writer)
 
-  def _OutputHeader(self, values):
+  def _OutputHeader(self, values, csv_writer):
     """Output the header rows.
 
     This will retrieve the header string from the given values. As a
@@ -39,33 +36,39 @@ class CsvPageMeasurementResults(
     Args:
       values: A set of values from which to extract the header string,
           which is the value name and the units.
+      writer: A csv.writer instance.
+
+    Returns:
+      The value names being output on the header, in the order of
+      output.
     """
     representative_values = {}
     for value in values:
       if value.name not in representative_values:
         representative_values[value.name] = value
-    self._representative_value_names = list(
-        representative_values.keys())
-    self._representative_value_names.sort()
+    header_value_names = list(representative_values.keys())
+    header_value_names.sort()
 
     row = ['page_name']
-    for value_name in self._representative_value_names:
+    for value_name in header_value_names:
       units = representative_values[value_name].units
       row.append('%s (%s)' % (value_name, units))
-    self._results_writer.writerow(row)
-    self._output_stream.flush()
+    csv_writer.writerow(row)
+    self.output_stream.flush()
+    return header_value_names
 
-  def _OutputValuesForPage(self, page, page_values):
+  def _OutputValuesForPage(self, header_value_names, page, page_values,
+                           csv_writer):
     row = [page.display_name]
     values_by_value_name = {}
     for value in page_values:
       values_by_value_name[value.name] = value
 
-    for value_name in self._representative_value_names:
+    for value_name in header_value_names:
       value = values_by_value_name.get(value_name, None)
       if value and value.GetRepresentativeNumber():
         row.append('%s' % value.GetRepresentativeNumber())
       else:
         row.append('-')
-    self._results_writer.writerow(row)
-    self._output_stream.flush()
+    csv_writer.writerow(row)
+    self.output_stream.flush()

tools/telemetry/telemetry/results/csv_page_measurement_results_unittest.py → tools/telemetry/telemetry/results/csv_output_formatter_unittest.py

@@ -6,7 +6,8 @@ import csv
 import os
 import unittest
 
-from telemetry.results import csv_page_measurement_results
+from telemetry.results import csv_output_formatter
+from telemetry.results import page_test_results
 from telemetry.page import page_set
 from telemetry.value import histogram
 from telemetry.value import scalar
@@ -18,18 +19,12 @@ def _MakePageSet():
   ps.AddPageWithDefaultRunNavigate('http://www.bar.com/')
   return ps
 
-class NonPrintingCsvPageMeasurementResults(
-    csv_page_measurement_results.CsvPageMeasurementResults):
-  def __init__(self, *args):
-    super(NonPrintingCsvPageMeasurementResults, self).__init__(*args)
 
-  def _PrintPerfResult(self, *args):
-    pass
-
-class CsvPageMeasurementResultsTest(unittest.TestCase):
+class CsvOutputFormatterTest(unittest.TestCase):
   def setUp(self):
     self._output = StringIO.StringIO()
     self._page_set = _MakePageSet()
+    self._formatter = csv_output_formatter.CsvOutputFormatter(self._output)
 
   @property
   def lines(self):
@@ -47,7 +42,7 @@ class CsvPageMeasurementResultsTest(unittest.TestCase):
     return rows[1:]
 
   def test_with_no_results_on_second_run(self):
-    results = NonPrintingCsvPageMeasurementResults(self._output)
+    results = page_test_results.PageTestResults()
     results.StartTest(self._page_set[0])
     results.AddValue(scalar.ScalarValue(self._page_set[0], 'foo', 'seconds', 3))
     results.StopTest(self._page_set[0])
@@ -55,7 +50,8 @@ class CsvPageMeasurementResultsTest(unittest.TestCase):
     results.StartTest(self._page_set[1])
     results.StopTest(self._page_set[1])
 
-    results.PrintSummary()
+    self._formatter.Format(results)
+
     self.assertEqual(['page_name', 'foo (seconds)'], self.output_header_row)
     # TODO(chrishenry): Is this really the right behavior? Should this
     # not output a second row with '-' as its results?
@@ -63,7 +59,7 @@ class CsvPageMeasurementResultsTest(unittest.TestCase):
     self.assertEqual(expected, self.output_data_rows)
 
   def test_fewer_results_on_second_run(self):
-    results = NonPrintingCsvPageMeasurementResults(self._output)
+    results = page_test_results.PageTestResults()
     results.StartTest(self._page_set[0])
     results.AddValue(scalar.ScalarValue(self._page_set[0], 'foo', 'seconds', 3))
     results.AddValue(scalar.ScalarValue(self._page_set[0], 'bar', 'seconds', 4))
@@ -73,7 +69,7 @@ class CsvPageMeasurementResultsTest(unittest.TestCase):
     results.AddValue(scalar.ScalarValue(self._page_set[1], 'bar', 'seconds', 5))
     results.StopTest(self._page_set[1])
 
-    results.PrintSummary()
+    self._formatter.Format(results)
     self.assertEqual(['page_name', 'bar (seconds)', 'foo (seconds)'],
                      self.output_header_row)
     expected = [[self._page_set[0].url, '4.0', '3.0'],
@@ -81,7 +77,7 @@ class CsvPageMeasurementResultsTest(unittest.TestCase):
     self.assertEqual(expected, self.output_data_rows)
 
   def test_with_output_at_print_summary_time(self):
-    results = NonPrintingCsvPageMeasurementResults(self._output)
+    results = page_test_results.PageTestResults()
     results.StartTest(self._page_set[0])
     results.AddValue(scalar.ScalarValue(self._page_set[0], 'foo', 'seconds', 3))
     results.StopTest(self._page_set[0])
@@ -90,7 +86,7 @@ class CsvPageMeasurementResultsTest(unittest.TestCase):
     results.AddValue(scalar.ScalarValue(self._page_set[1], 'bar', 'seconds', 4))
     results.StopTest(self._page_set[1])
 
-    results.PrintSummary()
+    self._formatter.Format(results)
 
     self.assertEqual(
       self.output_header_row,
@@ -101,7 +97,7 @@ class CsvPageMeasurementResultsTest(unittest.TestCase):
     self.assertEqual(expected, self.output_data_rows)
 
   def test_histogram(self):
-    results = NonPrintingCsvPageMeasurementResults(self._output)
+    results = page_test_results.PageTestResults()
     results.StartTest(self._page_set[0])
     results.AddValue(histogram.HistogramValue(
         self._page_set[0], 'a', '',
@@ -114,7 +110,7 @@ class CsvPageMeasurementResultsTest(unittest.TestCase):
         raw_value_json='{"buckets": [{"low": 2, "high": 3, "count": 1}]}'))
     results.StopTest(self._page_set[1])
 
-    results.PrintSummary()
+    self._formatter.Format(results)
 
     self.assertEqual(
         self.output_header_row,

tools/telemetry/telemetry/results/output_formatter.py

+# Copyright 2014 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+
+class OutputFormatter(object):
+  """A formatter for PageTestResults.
+
+  An OutputFormatter takes PageTestResults, formats the results
+  (telemetry.value.Value instances), and output the formatted results
+  in the given output stream.
+
+  Examples of output formatter: CsvOutputFormatter produces results in
+  CSV format."""
+
+  def __init__(self, output_stream):
+    """Constructs a new formatter that writes to the output_stream.
+
+    Args:
+      output_stream: The stream to write the formatted output to.
+    """
+    self.output_stream = output_stream
+
+  def Format(self, page_test_results):
+    """Formats the given PageTestResults into the output stream.
+
+    This will be called once at the end of a benchmark.
+
+    Args:
+      page_test_results: A PageTestResults object containing all results
+         from the current benchmark run.
+    """
+    raise NotImplementedError()

tools/telemetry/telemetry/results/page_test_results.py

@@ -11,9 +11,22 @@ from telemetry import value as value_module
 from telemetry.value import failure
 
 class PageTestResults(object):
-  def __init__(self, output_stream=None, trace_tag=''):
+  def __init__(self, output_stream=None, output_formatters=None, trace_tag=''):
+    """
+    Args:
+      output_stream: The output stream to use to write test results.
+      output_formatters: A list of output formatters. The output
+          formatters are typically used to format the test results, such
+          as CsvOutputFormatter, which output the test results as CSV.
+      trace_tag: A string to append to the buildbot trace
+      name. Currently only used for buildbot.
+    """
+    # TODO(chrishenry): Figure out if trace_tag is still necessary.
+
     super(PageTestResults, self).__init__()
     self._output_stream = output_stream
+    self._output_formatters = (
+        output_formatters if output_formatters is not None else [])
     self._trace_tag = trace_tag
     self._current_page = None
 
@@ -97,6 +110,9 @@ class PageTestResults(object):
     self.successes.append(page)
 
   def PrintSummary(self):
+    for output_formatter in self._output_formatters:
+      output_formatter.Format(self)
+
     if self.failures:
       logging.error('Failed pages:\n%s', '\n'.join(
           p.display_name for p in self.pages_that_had_failures))

tools/telemetry/telemetry/results/results_options.py

@@ -9,10 +9,11 @@ import sys
 from telemetry.core import util
 from telemetry.page import page_measurement
 from telemetry.results import buildbot_page_measurement_results
-from telemetry.results import csv_page_measurement_results
+from telemetry.results import csv_output_formatter
 from telemetry.results import gtest_test_results
 from telemetry.results import html_page_measurement_results
 from telemetry.results import page_measurement_results
+from telemetry.results import page_test_results
 
 
 # Allowed output formats. The default is the first item in the list.
@@ -62,11 +63,13 @@ def PrepareResults(test, options):
   if not hasattr(options, 'output_trace_tag'):
     options.output_trace_tag = ''
 
+  output_formatters = []
   if options.output_format == 'none':
     return page_measurement_results.PageMeasurementResults(
         output_stream, trace_tag=options.output_trace_tag)
   elif options.output_format == 'csv':
-    return csv_page_measurement_results.CsvPageMeasurementResults(output_stream)
+    output_formatters.append(
+        csv_output_formatter.CsvOutputFormatter(output_stream))
   elif options.output_format == 'buildbot':
     return buildbot_page_measurement_results.BuildbotPageMeasurementResults(
         output_stream, trace_tag=options.output_trace_tag)
@@ -77,6 +80,10 @@ def PrepareResults(test, options):
         output_stream, test.__class__.__name__, options.reset_results,
         options.upload_results, options.browser_type,
         options.results_label, trace_tag=options.output_trace_tag)
+
+  if len(output_formatters) > 0:
+    return page_test_results.PageTestResults(
+        output_formatters=output_formatters)
   else:
     # Should never be reached. The parser enforces the choices.
     raise Exception('Invalid --output-format "%s". Valid choices are: %s'