Refactor stages 1, 2 and test_api overhaul.

scripts/slave/recipe_modules/auto_bisect/test_api.py

+# Copyright 2015 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 math
+
+from collections import defaultdict
+
+from recipe_engine import recipe_test_api
+
+from auto_bisect import revision_state
+
+
+class AutoBisectTestApi(recipe_test_api.RecipeTestApi):
+  def buildbot_job_status_mock(self, bb_data_list):
+    if bb_data_list:
+      return bb_data_list.pop()
+    return self.m.json.output_stream(
+        {'build': {
+            'result': 'SUCCESS',
+            'status': 'COMPLETED'}})
+
+  def compare_samples_data(self, data, rev_a, rev_b):

[dtu, 2016/09/13 23:57:28]

This logic feels too complex for a test, and is in danger of just repeating the
code being tested.

Is there a way to mock out the results of this function? You don't have to
handle arbitrary values, just the values given by the tests. And the tests don't
have to cover arbitrary values either, just representative values of each
scenario.

[RobertoCN, 2016/09/21 22:22:48]

Simplified it a lot, PTAL. Now any revision that has 1 as one of its values is
considered pre-regression and any that has 7 is considered post-regression. With
this we can return all three cases and also cover retest scenarios (by not
having either 1 or 7 until later in the list)

+    values_a = data[rev_a.commit_hash].get('parsed_values', [])[
+        :rev_a.test_run_count]
+    values_b = data[rev_b.commit_hash].get('parsed_values', [])[
+        :rev_b.test_run_count]
+    while len(values_a) < rev_a.test_run_count:
+      if values_a:
+        values_a.append(values_a[-1])
+      else:
+        values_a.append(0)
+
+    while len(values_b) < rev_b.test_run_count:
+      if values_b:
+        values_b.append(values_b[-1])
+      else:
+        values_b.append(0)
+
+    avg = lambda x: sum(x)/float((len(x) or 1))
+    mean_a = avg(values_a)
+    mean_b = avg(values_b)
+    var_a = map (lambda x: (x - mean_a) ** 2, values_a)
+    var_b = map (lambda x: (x - mean_b) ** 2, values_b)
+    std_dev_a = math.sqrt(avg(var_a))
+    std_dev_b = math.sqrt(avg(var_b))
+    result = revision_state.NEED_MORE_DATA
+    if len(values_a) >= 5 and len(values_b) >= 5:
+      if mean_a != mean_b:
+        result = revision_state.SIGNIFICANTLY_DIFFERENT
+      # TODO(robertocn): Add a test that uses this clause.
+      if len(values_a) >= 18 and len(values_b) >= 18:  # pragma: no cover
+        if mean_a == mean_b:
+          result = revision_state.NOT_SIGNIFICANTLY_DIFFERENT
+    return self.m.json.output_stream(
+        {
+            'sample_a': {
+                'debug_values': values_a,
+                'mean': mean_a,
+                'std_dev': std_dev_a
+            },
+            'sample_b': {
+                'debug_values': values_b,
+                'mean': mean_b,
+                'std_dev': std_dev_b,
+            },
+            'result': result
+    })
+
+  @recipe_test_api.mod_test_data
+  def hash_cp_map(self, items):
+    result = {}
+    for item in items:
+      if 'hash' in item and 'commit_pos' in item:

[dtu, 2016/09/13 23:57:28]

Is there ever a case where 'hash' (and/or 'commit_pos') is not specified? If
not, I'd remove the checks to simplify the code.

[RobertoCN, 2016/09/21 22:22:49]

'hash' will always be there, 'commit_pos' may not. Simplifying the check.

+        result[item['commit_pos']] = self.m.json.output_stream(
+            {'git_sha': item['hash']})
+    return result
+
+  @recipe_test_api.mod_test_data
+  @staticmethod
+  def revision_data(items):

[dtu, 2016/09/13 23:57:28]

As far as I can tell, this is only used in one place, to pass 'parsed_values' to
compare_samples_data(). Maybe a function with a narrower scope would be one that
only returns 'parsed_values'? Another bold possibility: make
compare_samples_data() return results by looking up the commit hashes, and not
have values at all.

[RobertoCN, 2016/09/21 22:22:49]

compare_sample_data cannot look up data on the config items directly. I will on
a later patch replace all mod_test_data with functions that operate directly on
list of dicts sent by the testcase via __call__, which will be the only contents
of _test_data.

+    result = {}
+    for item in items:
+      if 'hash' in item:
+        result[item['hash']] = item
+    return result
+
+  @recipe_test_api.mod_test_data
+  def revision_list(self, items):
+    result = {}
+    for item in items:
+      if 'hash' in item:
+        depot = item.get('depot', 'chromium')
+        result.setdefault(depot, [])
+        result[depot].append([item['hash'], item.get('commit_pos')])
+    # Exclude the start of the revision range.
+    result['chromium'] = result['chromium'][1:]

[dtu, 2016/09/13 23:57:28]

Special-casing chromium is weird. Should this apply to all depots?

[RobertoCN, 2016/09/21 22:22:48]

We special case the topmost repo because the last revision in the roll is
equivalent to the roll itself.

+    for depot in result:
+      result[depot] = self.m.json.output_stream(result[depot])
+    return result
+
+  @recipe_test_api.mod_test_data
+  def deps_change(self, items):
+    # If the revision has the key DEPS_change, we mock the result of git show to
+    # appear as if DEPS was among the files changed by the CL.
+    result = {}
+    for item in items:
+      if 'hash' in item:
+        git_output = ''
+        if 'DEPS_change' in item:
+          git_output = 'DEPS'
+      result[item['hash']] = self.m.raw_io.stream_output(git_output)

[dtu, 2016/09/13 23:57:28]

This line assumes 'hash' is in item, whereas the previous line 107 does not.

[RobertoCN, 2016/09/21 22:22:48]

Done.

+    return result
+
+  @recipe_test_api.mod_test_data
+  def diff_patch(self):
+    return self.m.raw_io.stream_output("""
+diff --git a/DEPS b/DEPS
+index 029be3b..2b3ea0a 100644
+--- a/DEPS
++++ b/DEPS
+@@ -13,7 +13,7 @@ deps = {
+     '@98fc59a5896f4ea990a4d527548204fed8f06c64',
+   'build/third_party/infra_libs':
+     'https://chromium.googlesource.com/infra/infra/packages/infra_libs.git'
+-    '@a13e6745a4edd01fee683e4157ea0195872e64eb',
++    '@15ea0920b5f83d0aff4bd042e95bc388d069d51c',
+   'build/third_party/lighttpd':
+     'https://chromium.googlesource.com/chromium/deps/lighttpd.git'
+     '@9dfa55d15937a688a92cbf2b7a8621b0927d06eb',
+    """)
+
+  @recipe_test_api.mod_test_data
+  def deps(self, items):
+    result = {}
+    for item in items:
+      if 'hash' in item:
+        deps_content = ''
+        if 'DEPS' in item:
+          deps_content = item['DEPS']
+      result[item['hash']] = self.m.raw_io.stream_output(deps_content)
+    return result
+
+  @recipe_test_api.placeholder_step_data
+  @staticmethod
+  def exists_result(exists=True):

[dtu, 2016/09/13 23:57:28]

Make protected or private.

style guide: Don't use @staticmethod.

[RobertoCN, 2016/09/21 22:22:49]

https://cs.chromium.org/search/?q=@staticmethod+file:%5Ebuild/scripts/slave/r...

This seems like an accepted pattern throughout. Does it make more sense to
receive self and do nothing with it?

+    if exists:
+      return 'GS location exists', 0, ''
+    return 'GS location does not exist', 1, ''

[dtu, 2016/09/13 23:57:28]

Sorry, I can't figure out why this is the return value here. This is returned by
m.gsutil(['ls']) ?

[RobertoCN, 2016/09/21 22:22:49]

the placeholder_step_data decorator converts this into step_test_data. I just
rewrote it to be more clear.

+
+  @recipe_test_api.mod_test_data
+  def gsutil_exists(self, items):
+    result = {}
+    for item in items:
+      if 'hash' in item and 'gsutil_exists' in item:
+        result[item['hash']] = [self.exists_result(i)
+                                for i in item['gsutil_exists']]

[dtu, 2016/09/13 23:57:28]

Does it need to be a list? I don't see any case where it changes within a
revision.

[RobertoCN, 2016/09/21 22:22:49]

We need to be able to return false for a number of times and then true to test
certain code paths.  It currently does change within a revision, because when
you pop out all Falses it defaults to True, see the code in
revision_state._is_build_archived(), it would be nice if that defaulting logic
was defined here rather than there, maybe when we change these to be functions.
I tried several things but the mod_test_data thing required that the results be
serializable, so I couldn't pass a function.

I thought having this as a list was more readable.

+    return result
+
+  @recipe_test_api.mod_test_data
+  def run_results(self, items):
+    def single_result(v):
+      return self.m.raw_io.stream_output(
+          data=v.get('stdout', 'text from actual benchmark, (ignored)'),
+          retcode=v.get('retcode', 0)) + self.m.raw_io.output(
+          data=v.get('stdout', 'text from actual benchmark, (ignored)'))

[dtu, 2016/09/13 23:57:28]

nit: Can you assign v.get(...) to a variable and reuse it in both places? Or
does that not work?

[RobertoCN, 2016/09/21 22:22:49]

Done.

+
+    result = {'default': self.m.raw_io.stream_output('mock output', retcode=0)}
+    for item in items:
+      if 'hash' in item and 'test_results' in item:
+        result[item['hash']] = [single_result(v) for v in item['test_results']]
+    return result
+
+  @recipe_test_api.mod_test_data
+  def cl_info(self, items):
+    result = {}
+    for item in items:
+      if 'hash' in item:
+        if 'cl_info' in item:

[dtu, 2016/09/13 23:57:28]

info = item.get('cl_info', {})

[RobertoCN, 2016/09/21 22:22:48]

Done.

+          info = item['cl_info']
+        else:
+          info = {}
+        result[item['hash']] = self.m.json.output_stream(info)
+    return result
+
+  @recipe_test_api.mod_test_data
+  def build_status(self, items):
+    result = {}
+    for item in items:
+      if 'hash' in item and 'build_status' in item:
+        result[item['hash']] = []
+        for entry in item['build_status']:
+          result[item['hash']].append(self.m.json.output_stream(entry))
+    return result
+
+  def __call__(self, config_items):
+    return (
+        self.revision_data(config_items)
+        + self.hash_cp_map(config_items)
+        + self.revision_list(config_items)
+        + self.run_results(config_items)
+        + self.deps_change(config_items)
+        + self.deps(config_items)
+        + self.cl_info (config_items)
+        + self.diff_patch()
+        + self.gsutil_exists(config_items)
+        + self.build_status(config_items)
+    )
+
+#    """Takes massive dictionary to populate test_data for all steps."""
+#    get commit hash
+#    get test results(gsutil) ?
+#    fetch deps
+#    generating patch
+#    reading culprit information
+#    expanding revision range
+#    hashing modified deps
+#    fetch builder state
+#    fetch build details
+#    (check image) gsutil
+#