Make bisect to abort early when the return codes for known good and known bad revisions are same.

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 276cdebca7383fef54c8fc9131e2b1292346bb76..cf454c8a6b828d86f2b6f1428587f60d3bc2c42a 100644
--- a/tools/auto_bisect/bisect_perf_regression_test.py
+++ b/tools/auto_bisect/bisect_perf_regression_test.py
@@ -114,13 +114,28 @@ DEFAULT_OPTIONS = {
 # that use _MockRunTests.

[qyearsley, 2015/06/23 20:47:45]

This comment should be updated.

[prasadv, 2015/06/23 21:37:48]

Now used by only _MockRunTests

 _MockResultsGenerator = (x for x in [])
 
-
-def _MockRunTests(*args, **kwargs):  # pylint: disable=unused-argument
-  return _FakeTestResult(_MockResultsGenerator.next())
-
-
-def _FakeTestResult(values):
-  result_dict = {'mean': 0.0, 'std_err': 0.0, 'std_dev': 0.0, 'values': values}
+def _MakeMockRunTests(is_return_code=False):

[qyearsley, 2015/06/23 20:47:45]

Optional: Maybe it would be clearer if this argument was called
bisect_mode_is_return_code, although this is a bit long (so I'm not completely
sure whether it's better).

[prasadv, 2015/06/23 21:37:48]

Done.

+  """Return MockRunTest based on the bisect mode."""
+  # pylint: disable=unused-argument
+  def _MockRunTests(*args, **kwargs):

[RobertoCN, 2015/06/23 21:03:03]

I think we only need one of these two functions.

[prasadv, 2015/06/23 21:37:48]

Done.

+    return _FakeTestResult(_MockResultsGenerator.next(), is_return_code)
+
+  def _MockRunTestsReturnCode(*args, **kwargs):
+    return _FakeTestResult(_MockResultsGenerator.next(), is_return_code)
+  # pylint: enable=unused-argument
+
+  if is_return_code:
+    return _MockRunTestsReturnCode
+  else:
+    return _MockRunTests
+
+
+def _FakeTestResult(values, is_return_code):
+  mean = 0.0
+  if is_return_code:
+    mean = 0 if (
+        all(current_value == 0 for current_value in values)) else 1

[qyearsley, 2015/06/23 20:47:45]

This would fit on one line if you shortened the variable name:

mean = 0 if all(v == 0 for v in values) else 1

Normally shorter variable names make it less readable, but in this case since
the variable scope is limited to only one line, I think it's OK.

[prasadv, 2015/06/23 21:37:48]

Done.

+  result_dict = {'mean': mean, 'std_err': 0.0, 'std_dev': 0.0, 'values': values}
   success_code = 0
   return (result_dict, success_code)
 
@@ -375,7 +390,7 @@ class BisectPerfRegressionTest(unittest.TestCase):
     _MockResultsGenerator = (r for r in results)
     bisect_class = bisect_perf_regression.BisectPerformanceMetrics
     original_run_tests = bisect_class.RunPerformanceTestAndParseResults
-    bisect_class.RunPerformanceTestAndParseResults = _MockRunTests
+    bisect_class.RunPerformanceTestAndParseResults = _MakeMockRunTests()
 
     try:
       dry_run_results = _GenericDryRun(_GetExtendedOptions(0, 0, False))
@@ -406,6 +421,36 @@ class BisectPerfRegressionTest(unittest.TestCase):
   def testBisectNotAborted_MultipleValues(self):
     self.assertFalse(self._CheckAbortsEarly(MULTIPLE_VALUES))
 
+  def _CheckAbortsEarlyForReturnCode(self, results):
+    """Returns True if the bisect job would abort early in return code mode."""
+    global _MockResultsGenerator
+    _MockResultsGenerator = (r for r in results)
+    bisect_class = bisect_perf_regression.BisectPerformanceMetrics
+    original_run_tests = bisect_class.RunPerformanceTestAndParseResults
+    bisect_class.RunPerformanceTestAndParseResults = _MakeMockRunTests(True)
+    options = dict(DEFAULT_OPTIONS)
+    options.update({'bisect_mode': 'return_code'})
+    try:
+      dry_run_results = _GenericDryRun(options)
+    except StopIteration:
+      # If StopIteration was raised, that means that the next value after
+      # the first two values was requested, so the job was not aborted.
+      return False
+    finally:
+      bisect_class.RunPerformanceTestAndParseResults = original_run_tests
+
+    # If the job was aborted, there should be a warning about it.
+    if ('known good and known bad revisions returned same' in
+        dry_run_results.abort_reason):
+      return True
+    return False
+
+  def testBisectAbortOn_SameReturnCode(self):
+    self.assertTrue (self._CheckAbortsEarlyForReturnCode([[0,0,0], [0,0,0]]))

[qyearsley, 2015/06/23 20:47:45]

Nit: space after assertTrue

[prasadv, 2015/06/23 21:37:48]

Done.

+
+  def testBisectNotAbortedOn_DifferentReturnCode(self):
+    self.assertFalse(self._CheckAbortsEarlyForReturnCode([[1,1,1], [0,0,0]]))
+
   def testGetCommitPosition(self):
     cp_git_rev = '7017a81991de983e12ab50dfc071c70e06979531'
     self.assertEqual(291765, source_control.GetCommitPosition(cp_git_rev))