Implementing direction_of_improvement

tools/auto_bisect/bisect_perf_regression_test.py

Index: tools/auto_bisect/bisect_perf_regression_test.py
diff --git a/tools/auto_bisect/bisect_perf_regression_test.py b/tools/auto_bisect/bisect_perf_regression_test.py
index 0e56e443e1afe033effa13f89faf2d72cf4a64f1..7bbb738361a6bdd0f841d9aa9105c40e364d66bc 100644
--- a/tools/auto_bisect/bisect_perf_regression_test.py
+++ b/tools/auto_bisect/bisect_perf_regression_test.py
@@ -17,9 +17,8 @@ import mock
 import source_control as source_control_module
 
 
-def _GetBisectPerformanceMetricsInstance():
-  """Returns an instance of the BisectPerformanceMetrics class."""
-  options_dict = {
+# Default options for the dry run
+DEFAULT_OPTIONS = {
     'debug_ignore_build': True,
     'debug_ignore_sync': True,
     'debug_ignore_perf_test': True,
@@ -28,6 +27,10 @@ def _GetBisectPerformanceMetricsInstance():
     'good_revision': 280000,
     'bad_revision': 280005,
   }
+
+
+def _GetBisectPerformanceMetricsInstance(options_dict):
+  """Returns an instance of the BisectPerformanceMetrics class."""
   bisect_options = bisect_perf_regression.BisectOptions.FromDict(options_dict)
   source_control = source_control_module.DetermineAndCreateSourceControl(
       bisect_options)
@@ -36,6 +39,43 @@ def _GetBisectPerformanceMetricsInstance():
   return bisect_instance
 
 
+def _GetExtendedOptions(d, f):
+  """Returns the a copy of the default options dict plus some options."""
+  result = dict(DEFAULT_OPTIONS)
+  result.update({
+      'direction_of_improvement': d,
+      'debug_fake_first_test_mean': f})
+  return result
+
+
+def _GenericDryRun(options, print_results=False):
+  """Performs a dry run of the bisector.
+
+  Args:
+    options: Dictionary containing the options for the bisect instance.
+    print_results: Boolean telling whether to call FormatAndPrintResults.
+
+  Returns:
+    The results dictionary as returned by the bisect Run method.
+  """
+  # Disable rmtree to avoid deleting local trees.
+  old_rmtree = shutil.rmtree
+  try:
+    shutil.rmtree = lambda path, onerror: None
+    bisect_instance = _GetBisectPerformanceMetricsInstance(options)
+    results = bisect_instance.Run(bisect_instance.opts.command,
+                                  bisect_instance.opts.bad_revision,
+                                  bisect_instance.opts.good_revision,
+                                  bisect_instance.opts.metric)
+
+    if print_results:
+      bisect_instance.FormatAndPrintResults(results)
+
+    return results
+  finally:
+    shutil.rmtree = old_rmtree
+
+
 class BisectPerfRegressionTest(unittest.TestCase):
   """Test case for other functions and classes in bisect-perf-regression.py."""
 
@@ -261,21 +301,34 @@ class BisectPerfRegressionTest(unittest.TestCase):
     This serves as a smoke test to catch errors in the basic execution of the
     script.
     """
-    # Disable rmtree to avoid deleting local trees.
-    old_rmtree = shutil.rmtree
-    try:
-      shutil.rmtree = lambda path, onerror: None
-      bisect_instance = _GetBisectPerformanceMetricsInstance()
-      results = bisect_instance.Run(bisect_instance.opts.command,
-                                    bisect_instance.opts.bad_revision,
-                                    bisect_instance.opts.good_revision,
-                                    bisect_instance.opts.metric)
-      bisect_instance.FormatAndPrintResults(results)
-    finally:
-      shutil.rmtree = old_rmtree
+    _GenericDryRun(DEFAULT_OPTIONS, True)
+
+  def testBisectImprovementDirectionFails(self):
+    """Dry run of a bisect with an improvement instead of regression.
+    """

[qyearsley, 2014/10/12 03:37:11]

Style nit: In general we've been putting the closing """ on the first line if
the docstring is only one line.

[RobertoCN, 2014/10/15 18:44:44]

Done.

+
+    # Test result goes from 0 to 100 where higher is better
+    results = _GenericDryRun(_GetExtendedOptions(1, 100))
+    self.assertIsNotNone(results.error)
+    self.assertIn('does not represent a regression', results.error)
+    # Test result goes from 0 to -100 where lower is better
+    results = _GenericDryRun(_GetExtendedOptions(-1, -100))
+    self.assertIsNotNone(results.error)
+    self.assertIn('does not represent a regression', results.error)
+
+  def testBisectImprovementDirectionSucceeds(self):
+    """Bisects with improvement direction matching regression range.
+    """
+    # Test result goes from 0 to 100 where lower is better
+    results = _GenericDryRun(_GetExtendedOptions(-1, 100))
+    self.assertIsNone(results.error)
+    # Test result goes from 0 to -100 where higher is better
+    results = _GenericDryRun(_GetExtendedOptions(1, -100))
+    self.assertIsNone(results.error)
+
 
   def testGetCommitPosition(self):
-    bisect_instance = _GetBisectPerformanceMetricsInstance()
+    bisect_instance = _GetBisectPerformanceMetricsInstance(DEFAULT_OPTIONS)
     cp_git_rev = '7017a81991de983e12ab50dfc071c70e06979531'
     self.assertEqual(
         291765, bisect_instance.source_control.GetCommitPosition(cp_git_rev))
@@ -285,7 +338,7 @@ class BisectPerfRegressionTest(unittest.TestCase):
         291467, bisect_instance.source_control.GetCommitPosition(svn_git_rev))
 
   def testGetCommitPositionForV8(self):
-    bisect_instance = _GetBisectPerformanceMetricsInstance()
+    bisect_instance = _GetBisectPerformanceMetricsInstance(DEFAULT_OPTIONS)
     v8_rev = '21d700eedcdd6570eff22ece724b63a5eefe78cb'
     depot_path = os.path.join(bisect_instance.src_cwd, 'v8')
     self.assertEqual(
@@ -293,7 +346,7 @@ class BisectPerfRegressionTest(unittest.TestCase):
         bisect_instance.source_control.GetCommitPosition(v8_rev, depot_path))
 
   def testGetCommitPositionForWebKit(self):
-    bisect_instance = _GetBisectPerformanceMetricsInstance()
+    bisect_instance = _GetBisectPerformanceMetricsInstance(DEFAULT_OPTIONS)
     wk_rev = 'a94d028e0f2c77f159b3dac95eb90c3b4cf48c61'
     depot_path = os.path.join(bisect_instance.src_cwd, 'third_party', 'WebKit')
     self.assertEqual(
@@ -301,7 +354,7 @@ class BisectPerfRegressionTest(unittest.TestCase):
         bisect_instance.source_control.GetCommitPosition(wk_rev, depot_path))
 
   def testUpdateDepsContent(self):
-    bisect_instance = _GetBisectPerformanceMetricsInstance()
+    bisect_instance = _GetBisectPerformanceMetricsInstance(DEFAULT_OPTIONS)
     deps_file = 'DEPS'
     # We are intentionally reading DEPS file contents instead of string literal
     # with few lines from DEPS because to check if the format we are expecting