TimelineBasedMeasurement(object) instead of TimelineBasedMeasurement(page_test.PageTest)

tools/telemetry/telemetry/benchmark.py

Index: tools/telemetry/telemetry/benchmark.py
diff --git a/tools/telemetry/telemetry/benchmark.py b/tools/telemetry/telemetry/benchmark.py
index 46fabc1c299e68724b8e6f813e53204e6e927b32..215c65ccb92fbad26eade37f07547094dc639af3 100644
--- a/tools/telemetry/telemetry/benchmark.py
+++ b/tools/telemetry/telemetry/benchmark.py
@@ -20,6 +20,8 @@ from telemetry.page import test_expectations
 from telemetry.results import results_options
 from telemetry.util import cloud_storage
 from telemetry.util import exception_formatter
+from telemetry.web_perf import timeline_based_measurement
+
 
 Disabled = decorators.Disabled
 Enabled = decorators.Enabled
@@ -30,6 +32,16 @@ class InvalidOptionsError(Exception):
   pass
 
 
+class InvalidOverrideError(Exception):
+  """Raised when overrides do not make sense.
+
+  PageTests should only override CreatePageTest.
+  TimelineBasedMeasurements should only override
+  TimelineBasedMeasurementOptions.
+  """
+  pass
+
+
 class BenchmarkMetadata(object):
   def __init__(self, name):
     self._name = name
@@ -41,9 +53,18 @@ class BenchmarkMetadata(object):
 class Benchmark(command_line.Command):
   """Base class for a Telemetry benchmark.
 
-  A test packages a PageTest and a PageSet together.
+  A benchmark creates a TimelineBasedMeasurement with
+  a TimelineBasedMeasurementOptions.
+  Alternatively, a benchmark packages a PageTest and a PageSet together.
+
+  Benchmark creators either override CreateTimelineBasedMeasurementOptions or
+  CreatePageTest. Only one of those methods can be overridden.
+
+  A TimelineBasedMeasurement will be used if Benchmark.test is None, or if
+  CreatePageTest is not overridden.
   """
   options = {}
+  test = None

[slamm, 2014/12/19 00:45:48]

Since we check if the test is None, should I add this.
Or, should the code check hasattr(self, 'test')?

[nednguyen, 2014/12/19 01:16:21]

I prefer None checking over these attributes magic.

[chrishenry, 2014/12/19 01:25:03]

Why not test = TimelineBasedMeasurement?

[slamm, 2015/01/02 23:47:20]

Done.

 
   def __init__(self, max_failures=None):
     """Creates a new Benchmark.
@@ -53,6 +74,16 @@ class Benchmark(command_line.Command):
           from executing subsequent page runs. If None, we never bail.
     """
     self._max_failures = max_failures
+    has_custom_tbm_options = (
+        self.CreateTimelineBasedMeasurementOptions.__func__ !=
+        Benchmark.CreateTimelineBasedMeasurementOptions.__func__)
+    has_custom_create_page_test = (
+        self.CreatePageTest.__func__ != Benchmark.CreatePageTest.__func__)
+    if has_custom_tbm_options and has_custom_create_page_test:
+      raise InvalidOverrideError(
+        'Both CreatePageTest and '
+        'CreateTimelineBasedMeasurementOptions are overridden. '
+        'It only one makes sense to override one.')
 
   @classmethod
   def Name(cls):
@@ -195,19 +226,41 @@ class Benchmark(command_line.Command):
         extracted_profile_dir_path)
     options.browser_options.profile_dir = extracted_profile_dir_path
 
-  def CreatePageTest(self, options):  # pylint: disable=W0613
-    """Get the PageTest for this Benchmark.
+  def CreateTimelineBasedMeasurementOptions(self):
+    """Return the TimelineBasedMeasurementOptions for this Benchmark.
 
-    By default, it will create a page test from the test's test attribute.
-    Override to generate a custom page test.
+    Override this method to configure TimelineBasedMeasurement tests.
+    Otherwise, override CreatePageTest for PageTest tests. Do not override
+    both methods.
     """
-    if not hasattr(self, 'test'):
-      raise NotImplementedError('This test has no "test" attribute.')
-    if not issubclass(self.test, page_test.PageTest):
-      raise TypeError('"%s" is not a PageTest.' % self.test.__name__)
-    return self.test()
+    return timeline_based_measurement.Options()
+
+  def CreatePageTest(self, options):  # pylint: disable=unused-argument
+    """Return the PageTest for this Benchmark.
 
-  def CreatePageSet(self, options):  # pylint: disable=W0613
+    Override this method for PageTest tests.
+    Override, override CreateTimelineBasedMeasurementOptions to configure
+    TimelineBasedMeasurement tests. Do not override both methods.
+
+    If neither method is overridden, then the default is to create the
+    benchmark based on the Benchmark.test attribute which has the test class.
+    If |test| is None or a TimelineBasedMeasurement, a TimelineBasedMeasurement
+    is returned. If |test| is a PageTest class, then |test()| is returned.
+    """
+    tbm_class = timeline_based_measurement.TimelineBasedMeasurement
+    if self.test not in (None, page_test.PageTest, tbm_class):
+      raise TypeError('"%s" is not a PageTest or TimelineBasedMeasurement.' %
+                      self.test.__name__)

[nednguyen, 2014/12/19 01:24:43]

Hmh, how does this work if test is subclass for page_test?

[slamm, 2015/01/02 23:47:20]

Fixed with issubclass.

+    if issubclass(self.test, page_test.PageTest):
+      if (self.CreateTimelineBasedMeasurementOptions.__func__ !=
+          Benchmark.CreateTimelineBasedMeasurementOptions.__func__):
+        raise InvalidOverrideError(
+            'self.test is a PageTest, however, '
+            'CreateTimelineBasedMeasurementOptions is also overridden.')
+      return self.test()
+    return tbm_class(self.CreateTimelineBasedMeasurementOptions())
+
+  def CreatePageSet(self, options):  # pylint: disable=unused-argument
     """Get the page set this test will run on.
 
     By default, it will create a page set from the this test's page_set