[tools/perf] Add unittests coverage for system health benchmarks

tools/perf/benchmarks/benchmark_unittest.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.
 
 """For all the benchmarks that set options, test that the options are valid."""
 
 from collections import defaultdict
 import os
 import unittest
+import sys

[petrcermak, 2016/08/03 08:49:27]

nit: sort order ('s' < 'u')

[nednguyen, 2016/08/03 15:41:36]

Done.

 
+from benchmarks import system_health

[petrcermak, 2016/08/03 08:49:27]

nit: I would probably do `from benchmarks import system_health as
system_health_benchmark` so that the name is unambiguous throughout the file.
WDYT?

[nednguyen, 2016/08/03 15:41:36]

Done.

+from page_sets.system_health import system_health_stories

[petrcermak, 2016/08/03 08:49:26]

nit: sort order ('b' < 'c' < 'p'). If you want to keep the SH imports together,
then I suggest you separate them using a blank line.

[nednguyen, 2016/08/03 15:41:36]

Done.

 from core import perf_benchmark
 
 from telemetry import benchmark as benchmark_module
 from telemetry.core import discover
 from telemetry.internal.browser import browser_options
 from telemetry.testing import progress_reporter
 
 
 def _GetPerfDir(*subdirs):
   perf_dir = os.path.dirname(os.path.dirname(__file__))
   return os.path.join(perf_dir, *subdirs)
 
 
 def _GetAllPerfBenchmarks():
   return discover.DiscoverClasses(
       _GetPerfDir('benchmarks'), _GetPerfDir(), benchmark_module.Benchmark,
       index_by_class_name=True).values()
 
 
+def _GetAllSystemHealthBenchmarks():
+  return [b for b in _GetAllPerfBenchmarks() if
+          sys.modules[b.__module__] == system_health]
+
+
 def _BenchmarkOptionsTestGenerator(benchmark):
   def testBenchmarkOptions(self):  # pylint: disable=unused-argument
     """Invalid options will raise benchmark.InvalidOptionsError."""
     options = browser_options.BrowserFinderOptions()
     parser = options.CreateParser()
     benchmark.AddCommandLineArgs(parser)
     benchmark_module.AddCommandLineArgs(parser)
     benchmark.SetArgumentDefaults(parser)
   return testBenchmarkOptions
 
 
 class TestNoBenchmarkNamesDuplication(unittest.TestCase):
 
   def runTest(self):
     all_benchmarks = _GetAllPerfBenchmarks()
     names_to_benchmarks = defaultdict(list)
     for b in all_benchmarks:
       names_to_benchmarks[b.Name()].append(b)
     for n in names_to_benchmarks:
       self.assertEquals(1, len(names_to_benchmarks[n]),
                         'Multiple benchmarks with the same name %s are '
                         'found: %s' % (n, str(names_to_benchmarks[n])))
 
 
+class TestSystemHealthBenchmarks(unittest.TestCase):
+
+  def testNaming(self):
+    for s in _GetAllSystemHealthBenchmarks():

[petrcermak, 2016/08/03 08:49:27]

nit: could you change the variable to |b| as well so that it's consistent with
the rest of the test?

[nednguyen, 2016/08/03 15:41:36]

Done.

+      self.assertTrue(
+          s.Name().startswith('system_health.'),
+          '%s must have name started with "system_health." prefix')

[petrcermak, 2016/08/03 08:49:26]

you forgot to add `% s`

[petrcermak, 2016/08/03 08:49:27]

also, I wonder whether '%r' wouldn't be more applicable:

%s -> 'module.ClassName'
%r -> '<class module.ClassName at 0x7f83d1757870>'

Alternatively, how about we use '%s' with `b.Name()`?

[nednguyen, 2016/08/03 15:41:36]

Done.

+
+  def testShouldTearDownStateAfterEachStoryRunIsTrue(self):
+    for b in _GetAllSystemHealthBenchmarks():
+      self.assertTrue(
+          b.ShouldTearDownStateAfterEachStoryRun(),
+          '%s has ShouldTearDownStateAfterEachStoryRun set to False' % b)
+
+  def testSystemHealthStorySetAreUsed(self):
+    for b in _GetAllSystemHealthBenchmarks():
+      if b is system_health.WebviewStartupSystemHealthBenchmark:
+          continue

[petrcermak, 2016/08/03 08:49:27]

nit: this should be indented only 2 spaces (-2)

[petrcermak, 2016/08/03 08:49:27]

Does this mean that the WebView benchmark shouldn't be a part of System Health
or it should use the SH story set?

[nednguyen, 2016/08/03 15:41:36]

Probably it should use the SH story set. We probably should add blank case to sh
story et.

+      self.assertIsInstance(
+          b().CreateStorySet(None),
+          system_health_stories.SystemHealthStorySet,
+          '%s does not use SystemHealthStorySet' % b)
+
+
 class TestNoOverrideCustomizeBrowserOptions(unittest.TestCase):
 
   def runTest(self):
     all_benchmarks = _GetAllPerfBenchmarks()
     for benchmark in all_benchmarks:
       self.assertEquals(True, issubclass(benchmark,
                                          perf_benchmark.PerfBenchmark),
                         'Benchmark %s needs to subclass from PerfBenchmark'
                         % benchmark.Name())
       self.assertEquals(
@@ skipping 10 matching lines @@
   for benchmark in all_benchmarks:
     if not benchmark.options:
       # No need to test benchmarks that have not defined options.
       continue
 
     class BenchmarkOptionsTest(unittest.TestCase):
       pass
     setattr(BenchmarkOptionsTest, benchmark.Name(),
             _BenchmarkOptionsTestGenerator(benchmark))
     suite.addTest(BenchmarkOptionsTest(benchmark.Name()))
-  suite.addTest(TestNoBenchmarkNamesDuplication())
-  suite.addTest(TestNoOverrideCustomizeBrowserOptions())
 
 
 def load_tests(loader, standard_tests, pattern):
-  del loader, standard_tests, pattern  # unused
+  del loader, pattern  # unused
   suite = progress_reporter.TestSuite()
+  for t in standard_tests:
+    suite.addTests(t)
   _AddBenchmarkOptionsTests(suite)
   return suite