Swarming: conditionally run tests on swarming in chromium_trybot recipe.

Swarming: conditionally run tests on swarming in chromium_trybot recipe.

Test spec now can contain optional 'swarming' section in test definition that
describes conditions for running the test on Swarming. If this section is not
present, swarming support is completely disabled (as indicated by unchanged
expectation files).

R=maruel@chromium.org,phajdan.jr@chromium.org
BUG=354057
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=277077

[Vadim Sh., 2014-06-04 00:03:56]

PTAL

Expectations for non swarming tests do not change -> when committed should be
NOOP unless explicitly enables by modifying test spec. 

Once committed I'll verify it works by uploading a CL that changes
src/testing/buildbot/chromium_try.json and triggering try job with that CL.

I also had to add support for always_run, can_fail_build, ... to some Swarming
and Isolate steps. All modifications in recipe_modules/(isolate|swarming)/* are
about that.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipes/chromi...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium_trybot.py:489: args.append('--gtest_filter=%s' %
':'.join(failed_tests))
Local tests are using a temp file with gtest filters, passed to the test via
--test-launcher-filter-file. 

I'm not sure why it is done so, but it's a giant PITA to replicate exact same
behavior on swarming. So reverting to --gtest_filter= for now. I think it should
work.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium_trybot.py:570: """Returns a list of tests to run
and additional targets to compile.
This is refactoring of spec parsing code into a separate function +
should_use_test callback. This callback is also used to decide when to run the
test on swarming.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium_trybot.py:706: if 'platforms' in test:
No new logic here.

[M-A Ruel, 2014-06-04 00:52:53]

Some comments but mostly questions, nothing huge.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/isolate/api.py (right):

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/isolate/api.py:41: def find_isolated_tests(self,
build_dir, targets=None, **kwargs):
Side note: it's interesting because in the last CL you specified all the child
flags manually but here you gave up and used kwargs. I don't mind either way, I
just find this interesting.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/isolate/api.py:56: if 'abort_on_failure' not in
kwargs:
What you want is:
kwargs.setdefault('abort_on_failure', True)

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/swarming/api.py:228: kwargs: passed to recipe step
constructor as is, may contain
as-is

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/swarming/api.py:286: kwargs: passed to recipe step
constructor as is, may contain
as-is

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/swarming/api.py:304: if 'always_run' not in kwargs:
setdefault, everywhere

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/test_utils/api.py (right):

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/test_utils/api.py:43: # If True, 'trigger' and
'collect' should be used instead of 'run'.
If I were another reader that didn't know about swarming, I'd be confused by
this comment. Either make the comment clearer or the variable name clearer of
the specificity about swarming.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/test_utils/api.py:103: """Runs synchronous and
asynchronous tests."""
Add a note about how test can run magically in parallel.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipes/chromi...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium_trybot.py:568: 
remove one empty line

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium_trybot.py:584: # TODO (nodir): Remove this after
TODO(nodir):

We never put a space in between.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium_trybot.py:621: 'is ambiguous. Use false or true.'
% test_name)
True or False ?

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium_trybot.py:627: if isinstance(test_args,
basestring):
I'm not familiar with this code but the isinstance() dances done in this
function is a bit sad. I hope it'll get away eventually.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium_trybot.py:1069: yield (
I think a comment about why it's done this way would be useful for future
readers. No need to state what is being done but more about the greater context.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium_trybot.py:1077: 'swarming': True,
I don't understand why you do not use a dict with the supported platform as done
below. I prefer when the variables do not mutate to different type, especially
in different items in the same list.

[Vadim Sh., 2014-06-04 02:58:27]

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/isolate/api.py (right):

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/isolate/api.py:41: def find_isolated_tests(self,
build_dir, targets=None, **kwargs):
On 2014/06/04 00:52:53, M-A Ruel wrote:
> Side note: it's interesting because in the last CL you specified all the child
> flags manually but here you gave up and used kwargs. I don't mind either way,
I
> just find this interesting.

I've realized I need to pass crapload of additional flags to support control
flow needed in chromium_trybot recipe (basically, running after failures, when
retrying without a patch). And I gave up. Tuple (can_fail_build,
abort_on_failure, always_run) is a ridiculous way to structure a sequence of
steps.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/isolate/api.py:56: if 'abort_on_failure' not in
kwargs:
On 2014/06/04 00:52:53, M-A Ruel wrote:
> What you want is:
> kwargs.setdefault('abort_on_failure', True)

Done.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/swarming/api.py:228: kwargs: passed to recipe step
constructor as is, may contain
On 2014/06/04 00:52:53, M-A Ruel wrote:
> as-is

Done.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/swarming/api.py:286: kwargs: passed to recipe step
constructor as is, may contain
On 2014/06/04 00:52:53, M-A Ruel wrote:
> as-is

Done.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/swarming/api.py:304: if 'always_run' not in kwargs:
On 2014/06/04 00:52:53, M-A Ruel wrote:
> setdefault, everywhere

Done.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/test_utils/api.py (right):

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/test_utils/api.py:43: # If True, 'trigger' and
'collect' should be used instead of 'run'.
On 2014/06/04 00:52:53, M-A Ruel wrote:
> If I were another reader that didn't know about swarming, I'd be confused by
> this comment. Either make the comment clearer or the variable name clearer of
> the specificity about swarming.

Added more explanation, mentioned swarming.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/test_utils/api.py:103: """Runs synchronous and
asynchronous tests."""
On 2014/06/04 00:52:53, M-A Ruel wrote:
> Add a note about how test can run magically in parallel.

Done.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipes/chromi...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium_trybot.py:568: 
On 2014/06/04 00:52:53, M-A Ruel wrote:
> remove one empty line

Hmm.. I'm not sure. It marks the end of local classes definitions block and
start of local functions definitions.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium_trybot.py:584: # TODO (nodir): Remove this after
On 2014/06/04 00:52:53, M-A Ruel wrote:
> TODO(nodir):
> 
> We never put a space in between.

Done. (I've just copy-pasted it from where it was before).

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium_trybot.py:621: 'is ambiguous. Use false or true.'
% test_name)
On 2014/06/04 00:52:53, M-A Ruel wrote:
> True or False ?

It's originally in *.json file. So boolean literals are lowercase.

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium_trybot.py:627: if isinstance(test_args,
basestring):
On 2014/06/04 00:52:53, M-A Ruel wrote:
> I'm not familiar with this code but the isinstance() dances done in this
> function is a bit sad. I hope it'll get away eventually.

I didn't change this logic, it was here previously. Test spec format is a bit
loose. Pawel can comment, since he wrote it (I think).

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium_trybot.py:1069: yield (
On 2014/06/04 00:52:53, M-A Ruel wrote:
> I think a comment about why it's done this way would be useful for future
> readers. No need to state what is being done but more about the greater
context.

Added comments describing what code flow paths are being tested. It's difficult
to explain how this stuff works for readers that are not familiar with recipes
magic :(

https://codereview.chromium.org/313693003/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium_trybot.py:1077: 'swarming': True,
On 2014/06/04 00:52:53, M-A Ruel wrote:
> I don't understand why you do not use a dict with the supported platform as
done
> below. I prefer when the variables do not mutate to different type, especially
> in different items in the same list.

It's for code coverage of 'if isinstance(swarming_spec, bool):' branch in
parse_test_spec :) Added explanation to comment. Made all other tests use
'swarming: True'.

[Paweł Hajdan Jr., 2014-06-04 10:50:31]

Nice work, glad to see this!

Please wait for my final review, and don't commit before. Ping on IM/email as
needed - you can also catch me on daily CQ standups.

The most important comment is the testing strategy. Everything else is pretty
much fine (possibly details that are likely to change with the switch I'm
suggesting), but I'd really like to compare swarming and non-swarming
side-by-side as opposed to switching single test targets to swarming globally,
without being able to compare the reliability.

https://codereview.chromium.org/313693003/diff/20001/scripts/slave/recipes/ch...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/313693003/diff/20001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:507: return []
Can we issue a failing step just to make sure the build is in fact red?

I'm mostly concerned about some assumptions being no longer true in the future
by accident, and failing here will help catch this.

https://codereview.chromium.org/313693003/diff/20001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:523: can_fail_build=False,
Could you explain why this is non-fatal? It seems it could result in silent
failures, but maybe I just need to understand better.

https://codereview.chromium.org/313693003/diff/20001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:526: def has_valid_results(self,
suffix):
Can we also see if self._tasks / self._results look as expected here, i.e. we
have successfully triggered and/or collected these results from swarming?

My intent is to catch various failure modes, and avoid e.g. empty results being
treated as valid/success because collection failed.

Even if that failure mode is not possible now (or we think it's not possible)
I'd like to add defense-in-depth here for the future.

https://codereview.chromium.org/313693003/diff/20001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:534: assert suffix in self._results, (
This is an example of a check that I'd recommend to move to has_valid_results.

You could then assert has_valid_results here instead.

https://codereview.chromium.org/313693003/diff/20001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:614: swarming_spec =
test_dict.get('swarming', False)
I strongly recommend different testing strategy.

Could you please introduce a different set of bots to test swarming on?

Then we can compare the pass/fail rate on non-swarming and swarming bots, and
send a percentage of CQ traffic to swarming ones as an experiment (without
blocking CQ on them, we did that already for recipe bots and GPU bots).

[M-A Ruel, 2014-06-05 15:05:10]

Personally lgtm, assuming granular switchability comes one day.

[Vadim Sh., 2014-06-09 20:27:16]

Pawel, please take another look.

The plan:
1. Get this submitted.
2. Add (linux|mac|win)_chromium_rel_swarming builders on tryserver.chromium,
using exact same pool of slaves as *_chromium_rel builders.
3. Make CQ send portion of jobs to these new builders.

Is that what you wanted?

https://codereview.chromium.org/313693003/diff/20001/scripts/slave/recipes/ch...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/313693003/diff/20001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:507: return []
On 2014/06/04 10:50:31, Paweł Hajdan Jr. wrote:
> Can we issue a failing step just to make sure the build is in fact red?
> 
> I'm mostly concerned about some assumptions being no longer true in the future
> by accident, and failing here will help catch this.

Done.

https://codereview.chromium.org/313693003/diff/20001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:523: can_fail_build=False,
On 2014/06/04 10:50:31, Paweł Hajdan Jr. wrote:
> Could you explain why this is non-fatal? It seems it could result in silent
> failures, but maybe I just need to understand better.

For same reason api.chromium.runtest on line 420 (GTestTest.run) is non-fatal.
If I understand correctly, it is related to patch deapply logic. (i.e. red test
doesn't necessary mean red build because trunk may be red).

https://codereview.chromium.org/313693003/diff/20001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:526: def has_valid_results(self,
suffix):
On 2014/06/04 10:50:31, Paweł Hajdan Jr. wrote:
> Can we also see if self._tasks / self._results look as expected here, i.e. we
> have successfully triggered and/or collected these results from swarming?
> 
> My intent is to catch various failure modes, and avoid e.g. empty results
being
> treated as valid/success because collection failed.
> 
> Even if that failure mode is not possible now (or we think it's not possible)
> I'd like to add defense-in-depth here for the future.

Done.

https://codereview.chromium.org/313693003/diff/20001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:534: assert suffix in self._results, (
On 2014/06/04 10:50:31, Paweł Hajdan Jr. wrote:
> This is an example of a check that I'd recommend to move to has_valid_results.
> 
> You could then assert has_valid_results here instead.

Done.

https://codereview.chromium.org/313693003/diff/20001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:614: swarming_spec =
test_dict.get('swarming', False)
On 2014/06/04 10:50:31, Paweł Hajdan Jr. wrote:
> I strongly recommend different testing strategy.
> 
> Could you please introduce a different set of bots to test swarming on?
> 
> Then we can compare the pass/fail rate on non-swarming and swarming bots, and
> send a percentage of CQ traffic to swarming ones as an experiment (without
> blocking CQ on them, we did that already for recipe bots and GPU bots).

Added configs for (linux|mac|win)_chromium_rel_swarming builders, that are
identtical to *_chromium_rel, but with enable_swarming: True. 

If enable_swarming is False (default), swarming is not used (regardless of
what's in test spec). If enable_swarming is True, test spec may specify what
tests to run on swarming.

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

I consider the remaining comments minor - I'd still like to take another look
before this gets committed if possible, and of course feel free to ping me on
IM, especially before or during or slightly after daily CQ standup meetings to
decrease review latency.

https://codereview.chromium.org/313693003/diff/20001/scripts/slave/recipes/ch...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/313693003/diff/20001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:523: can_fail_build=False,
On 2014/06/09 20:27:16, Vadim Sh. wrote:
> On 2014/06/04 10:50:31, Paweł Hajdan Jr. wrote:
> > Could you explain why this is non-fatal? It seems it could result in silent
> > failures, but maybe I just need to understand better.
> 
> For same reason api.chromium.runtest on line 420 (GTestTest.run) is non-fatal.
> If I understand correctly, it is related to patch deapply logic. (i.e. red
test
> doesn't necessary mean red build because trunk may be red).

Indeed, the with/without patch steps are non-fatal, and if there is a failure
caused by the patch we emit a failing summary step.

Now that was about running tests locally. With swarming there are many
additional failure modes.

We should indeed ignore the test failures at this point. However, I'd like to
make sure swarming system failures are not ignored, but propagated (similar to
how local test run can yield invalid test results).

Could you add comments here summarizing why test failures are non-fatal in
with/without patch steps (and instead a fatal summary step is issued as needed),
and how are swarming system/infrastructure failures handled?

Note it's not so much about swarming itself - anything that goes over network
can fail, AE might be down, etc.

https://codereview.chromium.org/313693003/diff/20001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:614: swarming_spec =
test_dict.get('swarming', False)
On 2014/06/09 20:27:16, Vadim Sh. wrote:
> On 2014/06/04 10:50:31, Paweł Hajdan Jr. wrote:
> > I strongly recommend different testing strategy.
> > 
> > Could you please introduce a different set of bots to test swarming on?
> > 
> > Then we can compare the pass/fail rate on non-swarming and swarming bots,
and
> > send a percentage of CQ traffic to swarming ones as an experiment (without
> > blocking CQ on them, we did that already for recipe bots and GPU bots).
> 
> Added configs for (linux|mac|win)_chromium_rel_swarming builders, that are
> identtical to *_chromium_rel, but with enable_swarming: True. 
> 
> If enable_swarming is False (default), swarming is not used (regardless of
> what's in test spec). If enable_swarming is True, test spec may specify what
> tests to run on swarming.

Sounds good to me. Thank you for making this change.

https://codereview.chromium.org/313693003/diff/40001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/test_utils/api.py (right):

https://codereview.chromium.org/313693003/diff/40001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/test_utils/api.py:116: # Trigger all async tests
first, so that they are running in parallel
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.

https://codereview.chromium.org/313693003/diff/40001/scripts/slave/recipes/ch...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/313693003/diff/40001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:485: # *.isolated may be missing if
*_run target is misconfigured. It a error
nit: "It a error" -> "It's an error"

https://codereview.chromium.org/313693003/diff/40001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:640: swarming_spec =
test_dict.get('swarming', False)
nit: Could you rename this to "can_use_swarming_if_enabled"?

This is a bit verbose, but hopefully makes it more clear in the test spec, which
is edited by developers which are not infrastructure experts.

I agree it's still not perfectly clear, but just a step I'd like to take.

Feel free to come up with different naming ideas, although I'd really like this
to be clear even if more verbose.

https://codereview.chromium.org/313693003/diff/40001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:644: elif isinstance(swarming_spec,
dict):
We have isinstance for other parts of the spec for backwards compatibility.
Swarming is new, so can we standardize on one format for now? We'll likely have
to extend it anyway, so let's keep the initial version as simple as possible
(less moving parts).

[Vadim Sh., 2014-06-12 01:00:09]

https://codereview.chromium.org/313693003/diff/20001/scripts/slave/recipes/ch...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/313693003/diff/20001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:523: can_fail_build=False,
On 2014/06/10 08:36:55, Paweł Hajdan Jr. wrote:
> On 2014/06/09 20:27:16, Vadim Sh. wrote:
> > On 2014/06/04 10:50:31, Paweł Hajdan Jr. wrote:
> > > Could you explain why this is non-fatal? It seems it could result in
silent
> > > failures, but maybe I just need to understand better.
> > 
> > For same reason api.chromium.runtest on line 420 (GTestTest.run) is
non-fatal.
> > If I understand correctly, it is related to patch deapply logic. (i.e. red
> test
> > doesn't necessary mean red build because trunk may be red).
> 
> Indeed, the with/without patch steps are non-fatal, and if there is a failure
> caused by the patch we emit a failing summary step.
> 
> Now that was about running tests locally. With swarming there are many
> additional failure modes.
> 
> We should indeed ignore the test failures at this point. However, I'd like to
> make sure swarming system failures are not ignored, but propagated (similar to
> how local test run can yield invalid test results).
If swarming itself fails completely, has_valid_results() returns False, because
swarming doesn't produce *.json with test summary in that case.

If only one shard fails... I think right now 'collect' step would be marked as
failed, but it would not fail the build. I'll add 'UNRELIABLE_RESULTS' to
global_tags in that case (in separate a CL). I think it's a good use case for
this.

> Could you add comments here summarizing why test failures are non-fatal in
> with/without patch steps (and instead a fatal summary step is issued as
needed),

I don't think it's a right place for that comment, since it is not swarming
related thing. There should be file-level comment (doc string perhaps)
documenting the entire process. I invite you to write it, since you're the
author of that logic (afaik).
 
> and how are swarming system/infrastructure failures handled?
> 
> Note it's not so much about swarming itself - anything that goes over network
> can fail, AE might be down, etc.

If swarming\isolate fails, test doesn't produce *.json, and has_valid_results()
returns False. 

But sometimes test run, and fail because a bot is in a stupid state. And such
case is indistinguishable from test failure from swarming standpoint. And it
happens for non-swarming tests as well. So I don't think that such possibility
should block this CL.

https://codereview.chromium.org/313693003/diff/40001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/test_utils/api.py (right):

https://codereview.chromium.org/313693003/diff/40001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/test_utils/api.py:116: # Trigger all async tests
first, so that they are running in parallel
On 2014/06/10 08:36:56, Paweł Hajdan Jr. wrote:
> 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.

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.

https://codereview.chromium.org/313693003/diff/40001/scripts/slave/recipes/ch...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/313693003/diff/40001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:485: # *.isolated may be missing if
*_run target is misconfigured. It a error
On 2014/06/10 08:36:56, Paweł Hajdan Jr. wrote:
> nit: "It a error" -> "It's an error"

Done.

https://codereview.chromium.org/313693003/diff/40001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:640: swarming_spec =
test_dict.get('swarming', False)
On 2014/06/10 08:36:56, Paweł Hajdan Jr. wrote:
> nit: Could you rename this to "can_use_swarming_if_enabled"?
> 
No, because in general it's not a boolean field, but a dict.

I remade it to be a dict with 'can_use_on_swarming_builders' key:

{
  'test': 'browser_tests',
  'swarming': {
    'can_use_on_swarming_builders': true,
    ...
  }
}

Is it clear enough?

> This is a bit verbose, but hopefully makes it more clear in the test spec,
which
> is edited by developers which are not infrastructure experts.
> 
I think using JSON for test specs is sub-optimal. JSON doesn't support comments.
I think it should be converted to python dicts loaded with ast.literal_eval.
Then the structure of test spec can be described in the same file as a comment,
so that any developer can understand it with perfect clearness... And since it's
literal_eval, no crazy buildbot-config style shit is possible.

> I agree it's still not perfectly clear, but just a step I'd like to take.
> 
> Feel free to come up with different naming ideas, although I'd really like
this
> to be clear even if more verbose.

https://codereview.chromium.org/313693003/diff/40001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:644: elif isinstance(swarming_spec,
dict):
On 2014/06/10 08:36:56, Paweł Hajdan Jr. wrote:
> We have isinstance for other parts of the spec for backwards compatibility.
> Swarming is new, so can we standardize on one format for now? We'll likely
have
> to extend it anyway, so let's keep the initial version as simple as possible
> (less moving parts).
Always dict now.

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

https://codereview.chromium.org/313693003/diff/40001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/test_utils/api.py (right):

https://codereview.chromium.org/313693003/diff/40001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/test_utils/api.py:116: # Trigger all async tests
first, so that they are running in parallel
On 2014/06/12 01:00:08, Vadim Sh. wrote:
> On 2014/06/10 08:36:56, Paweł Hajdan Jr. wrote:
> > 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.
> 
> Can you provide any valid use case for pre_run or post_run other than async
> running?

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.

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

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?

> Refactored into two classes.

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).

[M-A Ruel, 2014-06-12 12:19:23]

On 2014/06/12 11:16:10, Paweł Hajdan Jr. wrote:
> 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).

This CL has been for review for more than a week and it's a blocker to get our
OKR. As such I hope it'll be finished before next week.

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

I give up, you won.

https://codereview.chromium.org/313693003/diff/40001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/test_utils/api.py (right):

https://codereview.chromium.org/313693003/diff/40001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/test_utils/api.py:116: # Trigger all async tests
first, so that they are running in parallel
On 2014/06/12 11:16:10, Paweł Hajdan Jr. wrote:
> On 2014/06/12 01:00:08, Vadim Sh. wrote:
> > On 2014/06/10 08:36:56, Paweł Hajdan Jr. wrote:
> > > 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.
> > 
> > Can you provide any valid use case for pre_run or post_run other than async
> > running?
> 
> Given the three-phase run it seems it would always have something to do with
> async.
> 
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.
 
> However, the trigger and collect names seem too specific for an *API* . IMHO
Do we have any other API that does async step execution? I'd rather not
introduce new verbs in the code. 

> 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.
> 
> > Replacing 'async' word with pre_run and post_run completely obfuscates the
> real
> > reason this stuff is being added. 
> 
> 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.

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.

> 
> 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?
> 
> > Refactored into two classes.
> 
> 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.

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.  

> 
> 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.
> 

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.

> 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).

[Paweł Hajdan Jr., 2014-06-13 13:41:38]

LGTM, thanks!

[Vadim Sh., 2014-06-13 20:51:10]

The CQ bit was checked by vadimsh@chromium.org

[commit-bot: I haz the power, 2014-06-13 20:51:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vadimsh@chromium.org/313693003/100001

[commit-bot: I haz the power, 2014-06-13 20:53:13]

Change committed as 277077