De-duplicate steps between chromium and chromium_trybot recipes

scripts/slave/recipe_modules/test_utils/api.py

Index: scripts/slave/recipe_modules/test_utils/api.py
diff --git a/scripts/slave/recipe_modules/test_utils/api.py b/scripts/slave/recipe_modules/test_utils/api.py
index a2d49a126359fc6378c16d5850e7db911b72ba03..bdd1b50a59ebe0e2f9f5046f1534e2c618e469eb 100644
--- a/scripts/slave/recipe_modules/test_utils/api.py
+++ b/scripts/slave/recipe_modules/test_utils/api.py
@@ -35,54 +35,14 @@ class TestUtilsApi(recipe_api.RecipeApi):
             '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
@@ -98,15 +58,15 @@ class TestUtilsApi(recipe_api.RecipeApi):
       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"""
@@ -115,7 +75,7 @@ class TestUtilsApi(recipe_api.RecipeApi):
           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
@@ -123,10 +83,10 @@ class TestUtilsApi(recipe_api.RecipeApi):
     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"""
@@ -136,8 +96,8 @@ class TestUtilsApi(recipe_api.RecipeApi):
         """,
         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