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

 # 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.
 
 import os
 import re
 import shutil
 import sys
 import unittest
 
@@ skipping 93 matching lines @@
     'debug_ignore_sync': True,
     'debug_ignore_perf_test': True,
     'debug_ignore_regression_confidence': True,
     'command': 'fake_command',
     'metric': 'fake/metric',
     'good_revision': 280000,
     'bad_revision': 280005,
 }
 
 # This global is a placeholder for a generator to be defined by the test cases
 # 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 _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 _MockRunTests(*args, **kwargs):  # pylint: disable=unused-argument
-  return _FakeTestResult(_MockResultsGenerator.next())
+  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):
-  result_dict = {'mean': 0.0, 'std_err': 0.0, 'std_dev': 0.0, 'values': values}
+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)
 
 
 def _GetBisectPerformanceMetricsInstance(options_dict):
   """Returns an instance of the BisectPerformanceMetrics class."""
   opts = bisect_perf_regression.BisectOptions.FromDict(options_dict)
   return bisect_perf_regression.BisectPerformanceMetrics(opts, os.getcwd())
 
 
@@ skipping 234 matching lines @@
     # Test result goes from 0 to -100 where higher is better
     results = _GenericDryRun(_GetExtendedOptions(1, -100))
     self.assertIsNone(results.error)
 
   def _CheckAbortsEarly(self, results):
     """Returns True if the bisect job would abort early."""
     global _MockResultsGenerator
     _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))
     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
 
@@ skipping 10 matching lines @@
 
   def testBisectNotAborted_ClearRegression(self):
     self.assertFalse(self._CheckAbortsEarly(CLEAR_REGRESSION))
 
   def testBisectNotAborted_BarelyRegression(self):
     self.assertFalse(self._CheckAbortsEarly(BARELY_REGRESSION))
 
   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))
 
     svn_git_rev = 'e6db23a037cad47299a94b155b95eebd1ee61a58'
     self.assertEqual(291467, source_control.GetCommitPosition(svn_git_rev))
 
   def testGetCommitPositionForV8(self):
     bisect_instance = _GetBisectPerformanceMetricsInstance(DEFAULT_OPTIONS)
     v8_rev = '21d700eedcdd6570eff22ece724b63a5eefe78cb'
@@ skipping 288 matching lines @@
          (None, 0)),
     ]
     self._SetupRunGitMock(try_cmd)
     bisect_perf_regression._StartBuilderTryJob(
         fetch_build.PERF_BUILDER, git_revision, bot_name, bisect_job_name,
         patch)
 
 
 if __name__ == '__main__':
   unittest.main()