Swarming: conditionally run tests on swarming in chromium_trybot recipe.

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 a91b801feb820c1bbfeac33cb411ff90788e3e52..de876ff8bfcbedfd401b947bcd67e4bbf8cb5c71 100644
--- a/scripts/slave/recipe_modules/test_utils/api.py
+++ b/scripts/slave/recipe_modules/test_utils/api.py
@@ -40,6 +40,13 @@ class TestUtilsApi(recipe_api.RecipeApi):
     Base class for tests that can be retried after deapplying a previously
     applied patch.
     """
+    # If True, the test supports asynchronous execution. In that case 'trigger'
+    # and 'collect' will be used instead of 'run'. 'trigger' produces a step
+    # that asynchronously launches the test (just starts the test and
+    # immediately returns control back to the recipe), and 'collect' produces
+    # a step that blocks until the test is finished. The recipe can execute any
+    # other steps in between. This mode is used by tests running on Swarming.
+    async = False
 
     @property
     def name(self):  # pragma: no cover
@@ -50,6 +57,14 @@ class TestUtilsApi(recipe_api.RecipeApi):
       """Run the test. suffix is 'with patch' or 'without patch'."""
       raise NotImplementedError()
 
+    def trigger(self, suffix):  # pragma: no cover
+      """Launch the test asynchronously, used if self.async == True."""
+      raise NotImplementedError()
+
+    def collect(self, suffix):  # pragma: no cover
+      """Wait for triggered test to finish, used if self.async == True."""
+      raise NotImplementedError()
+
     def has_valid_results(self, suffix):  # pragma: no cover
       """
       Returns True if results (failures) are valid.
@@ -79,6 +94,9 @@ class TestUtilsApi(recipe_api.RecipeApi):
       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.',
@@ -86,7 +104,24 @@ class TestUtilsApi(recipe_api.RecipeApi):
         always_run=True, abort_on_failure=True)
       return  # won't actually hit this, but be explicit
 
-    yield (t.run('with patch') for t in tests)
+    def run(prefix, tests):
+      """Runs synchronous and asynchronous tests (at the same time).
+
+      Asynchronous tests are launched first (just launched, not being blocked
+      on). While they are running, the recipe blocks on synchronous tests
+      (sequentially, one by one). And then finally waits for all asynchronous
+      tests to finish. Effectively asynchronous tests are running in parallel
+      with each other and with synchronous tests.
+      """
+      # Trigger all async tests first, so that they are running in parallel

[Paweł Hajdan Jr., 2014/06/10 08:36:56]

Could you make one design change here?

Instead of the async property, how about having three methods that always get
run?

t.pre_run(prefix) # previously trigger
t.run(prefix)
t.post_run(prefix) # previously collect

I'm not really attached to names, feel free to use slightly different ones. Even
trigger and collect can stay if you prefer.

The main point is getting rid of test.async as part of the interface between
test_utils and the recipe. Internally classes inheriting from Test obviously can
keep track whether given instance should be async or not - although it may be
cleaner to have two classes instead, one async and one sync, and decide at
instantiation time which one to use.

[Vadim Sh., 2014/06/12 01:00:08]

Can you provide any valid use case for pre_run or post_run other than async
running?

Replacing 'async' word with pre_run and post_run completely obfuscates the real
reason this stuff is being added. 

Refactored into two classes.

[Paweł Hajdan Jr., 2014/06/12 11:16:10]

Given the three-phase run it seems it would always have something to do with
async.

However, the trigger and collect names seem too specific for an *API* . IMHO
this is enough reason to make this simple change, even if we don't know for now
how else someone may use it. Note that the API design also guides users of the
API, and I'd prefer it to be generic.
Please note this reasoning can be applied to most cases where something more
generic is used instead of something more specific - in other words, an
abstraction.

Is there some reason for a strong opposition against this suggestion? IMHO the
intent of the code would still be clear, especially the caller code. It'd just
remove specific references to swarming use case from the API intended to be
generic.

Note that this seems to fit well with how the API was designed from the
beginning - we have an abstract Test class with the implementation of run,
has_valid_results, and failures fully up to its caller. It's generic enough to
accommodate quite different steps like gtest tests but also checkdeps,
checkperms, checklicenses, even deps2git. If it had assumptions that are
specific to gtest, it would be more difficult or more ugly to use it for all
these cases.

Does this example of an API make its intent obscure?
Sorry, isinstance check on the class doesn't seem much different from checking a
class variable. Actually, to me it's an even bigger concern, because it forces
usage of a specific base class.

Could you clearly explain advantages of that approach over what I'm
recommending? IMHO it's not a matter of style that I'd be fine to accept a
different way of doing things, but design of an important API, and that has a
tendency to have more far-reaching consequences so it's important to me.

This is the last concern I have, after it's fixed I'll be happy to finish the
review. Feel free to ping me on IM for faster review turnaround across timezones
(also will be in MTV 6/16-6/27).

[Vadim Sh., 2014/06/12 18:34:10]

A test can't be synchronous and asynchronous at the same time. trigger\collect
(or launch\wait, or future=run()\future.wait(), whatever) and 'run' are mutually
exclusive. That's the main reason for dichotomy I had with async = True\False or
Test\AsyncTest classes. Now there's possible a new beast, that have all three
methods. It's actually increases overall abstraction complexity imho.
 
Do we have any other API that does async step execution? I'd rather not
introduce new verbs in the code. 
Premature abstractions are evil. That sort of reasoning leads to
AbstractBuilderFactoryFactoryBean classes. I personally prefer method of
incremental refactoring, when existing interfaces doesn't work, rather then
trying to invent everything upfront.
Again, test can't be synchronous and asynchronous at the same time. At least
that's the assumption I had. By choosing a base class you chooses what sort of
test you have.  
It's bikeshedding. You prefer one thing, I prefer another. Looks like it's more
important to you, than to me though, so I did it your way, even though I'm not
convinced it any better.

+      # with synchronous tests.
+      yield (t.trigger(prefix) for t in tests if t.async)
+      # Now block on all synchronous tests.
+      yield (t.run(prefix) for t in tests if not t.async)
+      # And finally wait for all pending asynchronous tests to complete.
+      yield (t.collect(prefix) for t in tests if t.async)
+
+    yield run('with patch', tests)
 
     failing_tests = []
     for t in tests:
@@ -106,7 +141,7 @@ class TestUtilsApi(recipe_api.RecipeApi):
 
     yield deapply_patch_fn(failing_tests)
 
-    yield (t.run('without patch') for t in failing_tests)
+    yield run('without patch', failing_tests)
     yield (self._summarize_retried_test(t) for t in failing_tests)
 
   def _summarize_retried_test(self, test):