De-duplicate steps between chromium and chromium_trybot recipes

scripts/slave/recipe_modules/test_utils/api.py

 # Copyright 2013 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.
 
 from slave import recipe_api
 
 class TestUtilsApi(recipe_api.RecipeApi):
   @staticmethod
   def format_step_text(data):
     """
@@ skipping 17 matching lines @@
         # Only displaying the section (even the header) when it's non-empty
         # simplifies caller code.
         if section[1]:
           step_text.append('<br/>%s<br/>' % section[0])
           step_text.extend(('%s<br/>' % line for line in section[1]))
       else:  # pragma: no cover
         raise ValueError(
             'Expected a one or two-element list, got %r instead.' % section)
     return ''.join(step_text)
 
-  class Test(object):
-    """
-    Base class for tests that can be retried after deapplying a previously
-    applied patch.
-    """
-
-    @property
-    def name(self):  # pragma: no cover
-      """Name of the test."""
-      raise NotImplementedError()
-
-    def pre_run(self, suffix):  # pragma: no cover
-      """Steps to execute before running the test."""
-      return []
-
-    def run(self, suffix):  # pragma: no cover
-      """Run the test. suffix is 'with patch' or 'without patch'."""
-      raise NotImplementedError()
-
-    def post_run(self, suffix):  # pragma: no cover
-      """Steps to execute after running the test."""
-      return []
-
-    def has_valid_results(self, suffix):  # pragma: no cover
-      """
-      Returns True if results (failures) are valid.
-
-      This makes it possible to distinguish between the case of no failures
-      and the test failing to even report its results in machine-readable
-      format.
-      """
-      raise NotImplementedError()
-
-    def failures(self, suffix):  # pragma: no cover
-      """Return list of failures (list of strings)."""
-      raise NotImplementedError()
-
-    def _step_name(self, suffix):
-      """Helper to uniformly combine tests's name with a suffix."""
-      return '%s (%s)' % (self.name, suffix)
-
-  def determine_new_failures(self, tests, deapply_patch_fn):
+  def determine_new_failures(self, api, tests, deapply_patch_fn):

[agable, 2014/06/27 18:34:42]

I really don't like all of these functions taking an API as input. Self already
is an api! All the other modules are on self.m! Having two APIs to choose from
is a Bad Idea.

[Paweł Hajdan Jr., 2014/06/27 18:58:22]

Suggestions what to replace that are welcome.

I could use self (or self.m) instead, however then test_utils' DEPS would need
to include things required by steps.py in the chromium recipe module.

It's a tradeoff, either way works for me although the solution from this patch
seems to make more sense to me. Maybe there is a third way I'm just not seeing -
recommendations are welcome.

[agable, 2014/06/27 21:04:22]

If you do the reorganization I describe in the comment above, then passing api
won't be necessary here.

     """
     Utility function for running steps with a patch applied, and retrying
     failing steps without the patch. Failures from the run without the patch are
     ignored.
 
     Args:
+      api - recipe API
       tests - iterable of objects implementing the Test interface above
       deapply_patch_fn - function that takes a list of failing tests
                          and undoes any effect of the previously applied patch
     """
     # Convert iterable to list, since it is enumerated multiple times.
     tests = list(tests)
 
     if self.m.step_history.failed:
       yield self.m.python.inline(
         'Aborting due to failed build state',
         "import sys; sys.exit(1)",
         always_run=True, abort_on_failure=True)
       return  # won't actually hit this, but be explicit
 
     def run(prefix, tests):
-      yield (t.pre_run(prefix) for t in tests)
-      yield (t.run(prefix) for t in tests)
-      yield (t.post_run(prefix) for t in tests)
+      yield (t.pre_run(api, prefix) for t in tests)
+      yield (t.run(api, prefix) for t in tests)
+      yield (t.post_run(api, prefix) for t in tests)
 
     yield run('with patch', tests)
 
     failing_tests = []
     for t in tests:
-      if not t.has_valid_results('with patch'):
+      if not t.has_valid_results(api, 'with patch'):
         yield self.m.python.inline(
           t.name,
           r"""
           import sys
           print 'TEST RESULTS WERE INVALID'
           sys.exit(1)
           """,
           always_run=True)
-      elif t.failures('with patch'):
+      elif t.failures(api, 'with patch'):
         failing_tests.append(t)
     if not failing_tests:
       return
 
     yield deapply_patch_fn(failing_tests)
 
     yield run('without patch', failing_tests)
-    yield (self._summarize_retried_test(t) for t in failing_tests)
+    yield (self._summarize_retried_test(api, t) for t in failing_tests)
 
-  def _summarize_retried_test(self, test):
-    if not test.has_valid_results('without patch'):
+  def _summarize_retried_test(self, api, test):
+    if not test.has_valid_results(api, 'without patch'):
       return self.m.python.inline(
         test.name,
         r"""
         import sys
         print 'TEST RESULTS WERE INVALID'
         sys.exit(1)
         """,
         always_run=True)
 
-    ignored_failures = set(test.failures('without patch'))
-    new_failures = set(test.failures('with patch')) - ignored_failures
+    ignored_failures = set(test.failures(api, 'without patch'))
+    new_failures = set(test.failures(api, 'with patch')) - ignored_failures
 
     def followup_fn(step_result):
       p = step_result.presentation
 
       p.step_text += self.format_step_text([
           ['failures:', new_failures],
           ['ignored:', ignored_failures]
       ])
 
       if new_failures:
@@ skipping 24 matching lines @@
       """,
       args=[
         self.m.json.input({
           'new': list(new_failures),
           'ignored': list(ignored_failures),
         })
       ],
       followup_fn=followup_fn,
       always_run=True,
     )