[Findit] Trigger swarming tasks after detech_first_faliure_pipeline

[Findit] Trigger swarming tasks after detech_first_faliure_pipeline

BUG=627298
Committed: https://chromium.googlesource.com/infra/infra/+/668b1e3f1018f5e9493a36dc12686dfe67e1d4c0

[chanli, 2016-07-12 00:32:34]

chanli@chromium.org changed reviewers:
+ stgao@chromium.org

[chanli, 2016-07-12 00:32:34]

ptal

[stgao, 2016-07-12 17:47:50]

https://codereview.chromium.org/2139093002/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/analyze_build_failure_pipeline.py (right):

https://codereview.chromium.org/2139093002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/analyze_build_failure_pipeline.py:80: yield
StartTryJobOnDemandPipeline(
As discussed, TriggerSwarmingTasksPipeline and StartTryJobOnDemandPipeline
should be run in order to make sure Swarming are triggered before we start
waiting.

We could use InOrder or After. InOrder is preferred, as we might add another
subpipeline in this ordered processing.

https://codereview.chromium.org/2139093002/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/swarming_tasks_to_try_job_pipeline.py (right):

https://codereview.chromium.org/2139093002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/swarming_tasks_to_try_job_pipeline.py:34: # Gets the
swarming tasks' results.
nit: Also wait, right?

https://codereview.chromium.org/2139093002/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/test/detect_first_failure_pipeline_test.py
(right):

https://codereview.chromium.org/2139093002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/test/detect_first_failure_pipeline_test.py:743:
expected_base_tests = ['test1', 'PRE_t', 'TestSuite1.Test1',
How about a map from original test to base test?

https://codereview.chromium.org/2139093002/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/trigger_swarming_tasks_pipeline.py (right):

https://codereview.chromium.org/2139093002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/trigger_swarming_tasks_pipeline.py:26:
failure_result_map = analysis.failure_result_map
failure_result_map not updated nor saved.

https://codereview.chromium.org/2139093002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/trigger_swarming_tasks_pipeline.py:29: # have not
trigger a swarming task yet.
nit: triggered

[chanli, 2016-07-12 20:04:51]

chanli@chromium.org changed reviewers:
+ lijeffrey@chromium.org

[chanli, 2016-07-12 20:04:52]

ptal

https://codereview.chromium.org/2139093002/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/analyze_build_failure_pipeline.py (right):

https://codereview.chromium.org/2139093002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/analyze_build_failure_pipeline.py:80: yield
StartTryJobOnDemandPipeline(
On 2016/07/12 17:47:49, stgao wrote:
> As discussed, TriggerSwarmingTasksPipeline and StartTryJobOnDemandPipeline
> should be run in order to make sure Swarming are triggered before we start
> waiting.
> 
> We could use InOrder or After. InOrder is preferred, as we might add another
> subpipeline in this ordered processing.

Done.

https://codereview.chromium.org/2139093002/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/swarming_tasks_to_try_job_pipeline.py (right):

https://codereview.chromium.org/2139093002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/swarming_tasks_to_try_job_pipeline.py:34: # Gets the
swarming tasks' results.
On 2016/07/12 17:47:49, stgao wrote:
> nit: Also wait, right?

Done.

https://codereview.chromium.org/2139093002/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/test/detect_first_failure_pipeline_test.py
(right):

https://codereview.chromium.org/2139093002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/test/detect_first_failure_pipeline_test.py:743:
expected_base_tests = ['test1', 'PRE_t', 'TestSuite1.Test1',
On 2016/07/12 17:47:50, stgao wrote:
> How about a map from original test to base test?

Done.

https://codereview.chromium.org/2139093002/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/trigger_swarming_tasks_pipeline.py (right):

https://codereview.chromium.org/2139093002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/trigger_swarming_tasks_pipeline.py:26:
failure_result_map = analysis.failure_result_map
On 2016/07/12 17:47:50, stgao wrote:
> failure_result_map not updated nor saved.

Done.

https://codereview.chromium.org/2139093002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/trigger_swarming_tasks_pipeline.py:29: # have not
trigger a swarming task yet.
On 2016/07/12 17:47:50, stgao wrote:
> nit: triggered

Done.

[stgao, 2016-07-12 20:26:52]

lgtm after comments are addressed.

Since this change is not a small one, it might worth testing it out with a real
test failure on the live app.

https://codereview.chromium.org/2139093002/diff/40001/appengine/findit/model/...
File appengine/findit/model/wf_analysis.py (right):

https://codereview.chromium.org/2139093002/diff/40001/appengine/findit/model/...
appengine/findit/model/wf_analysis.py:149: default={})
This possible pitfall might cause unexpected behavior.

It is similar to [] or {} as default parameters in a function.
http://stackoverflow.com/questions/5587582/numpy-array-subclass-unexpedly-sha...


Alternative is to the set default value in function ``Create`` above.

https://codereview.chromium.org/2139093002/diff/40001/appengine/findit/waterf...
File appengine/findit/waterfall/analyze_build_failure_pipeline.py (right):

https://codereview.chromium.org/2139093002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/analyze_build_failure_pipeline.py:83: yield
StartTryJobOnDemandPipeline(
nit: maybe add a comment here too.

[chanli, 2016-07-12 20:41:19]

https://codereview.chromium.org/2139093002/diff/40001/appengine/findit/model/...
File appengine/findit/model/wf_analysis.py (right):

https://codereview.chromium.org/2139093002/diff/40001/appengine/findit/model/...
appengine/findit/model/wf_analysis.py:149: default={})
On 2016/07/12 20:26:52, stgao wrote:
> This possible pitfall might cause unexpected behavior.
> 
> It is similar to [] or {} as default parameters in a function.
>
http://stackoverflow.com/questions/5587582/numpy-array-subclass-unexpedly-sha...
> 
> 
> Alternative is to the set default value in function ``Create`` above.

Done.

https://codereview.chromium.org/2139093002/diff/40001/appengine/findit/waterf...
File appengine/findit/waterfall/analyze_build_failure_pipeline.py (right):

https://codereview.chromium.org/2139093002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/analyze_build_failure_pipeline.py:83: yield
StartTryJobOnDemandPipeline(
On 2016/07/12 20:26:52, stgao wrote:
> nit: maybe add a comment here too.

Done.

[lijeffrey, 2016-07-12 22:36:28]

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/model/...
File appengine/findit/model/wf_analysis.py (right):

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/model/...
appengine/findit/model/wf_analysis.py:30: analysis.failure_result_map = {}
I think this can be simplified slightly with

analysis.failure_result_map = analysis.failure_result_map or {}

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/waterf...
File appengine/findit/waterfall/detect_first_failure_pipeline.py (right):

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/detect_first_failure_pipeline.py:25: def
_RemoveAnyPrefixes(test):
Nit: should this be renamed to _RemoveAllPrefixes, since the while loop in line
40 seems to remove all of them?

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/detect_first_failure_pipeline.py:34: test_name_start
= test.find('.') if test.find('.') > -1 else 0
I think we can do this with just 1 call to test.find() by doing something like

test_name_start = max(test.find('.'), 0)

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/detect_first_failure_pipeline.py:36: return test
nit: 1 empty line after return test

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/detect_first_failure_pipeline.py:37: test_suite =
test[ : test_name_start]
nit: is the extra space in 'test[ :' necessary? Same with line 38

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/detect_first_failure_pipeline.py:43: base_test =
test_suite + '.' + test_name
nit: How about

base_test = '%s.%s' % (test_suite, test_name)?

I think using %s should be more robust than the + operator which breaks if
anything is None

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/waterf...
File appengine/findit/waterfall/trigger_swarming_tasks_pipeline.py (right):

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/trigger_swarming_tasks_pipeline.py:16: def
_GetsStepsNeedToTriggerSwarmingTasks(
Nit: how about rename this function to

_GetStepsThatNeedToTriggerSwarmingTasks or something similar?

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/trigger_swarming_tasks_pipeline.py:49: 
nit: Shouldn't there be 2 empty lines here?

[chanli, 2016-07-12 22:54:56]

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/model/...
File appengine/findit/model/wf_analysis.py (right):

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/model/...
appengine/findit/model/wf_analysis.py:30: analysis.failure_result_map = {}
On 2016/07/12 22:36:27, lijeffrey wrote:
> I think this can be simplified slightly with
> 
> analysis.failure_result_map = analysis.failure_result_map or {}

Done.

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/waterf...
File appengine/findit/waterfall/detect_first_failure_pipeline.py (right):

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/detect_first_failure_pipeline.py:25: def
_RemoveAnyPrefixes(test):
On 2016/07/12 22:36:27, lijeffrey wrote:
> Nit: should this be renamed to _RemoveAllPrefixes, since the while loop in
line
> 40 seems to remove all of them?

Done.

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/detect_first_failure_pipeline.py:34: test_name_start
= test.find('.') if test.find('.') > -1 else 0
On 2016/07/12 22:36:27, lijeffrey wrote:
> I think we can do this with just 1 call to test.find() by doing something like
> 
> test_name_start = max(test.find('.'), 0)

Good idea. Done

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/detect_first_failure_pipeline.py:36: return test
On 2016/07/12 22:36:27, lijeffrey wrote:
> nit: 1 empty line after return test

Done.

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/detect_first_failure_pipeline.py:37: test_suite =
test[ : test_name_start]
On 2016/07/12 22:36:28, lijeffrey wrote:
> nit: is the extra space in 'test[ :' necessary? Same with line 38

I honestly don't know... I'll remove them for now.

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/detect_first_failure_pipeline.py:43: base_test =
test_suite + '.' + test_name
On 2016/07/12 22:36:28, lijeffrey wrote:
> nit: How about
> 
> base_test = '%s.%s' % (test_suite, test_name)?
> 
> I think using %s should be more robust than the + operator which breaks if
> anything is None

Done.

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/waterf...
File appengine/findit/waterfall/trigger_swarming_tasks_pipeline.py (right):

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/trigger_swarming_tasks_pipeline.py:16: def
_GetsStepsNeedToTriggerSwarmingTasks(
On 2016/07/12 22:36:28, lijeffrey wrote:
> Nit: how about rename this function to
> 
> _GetStepsThatNeedToTriggerSwarmingTasks or something similar?

Done.

https://codereview.chromium.org/2139093002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/trigger_swarming_tasks_pipeline.py:49: 
On 2016/07/12 22:36:28, lijeffrey wrote:
> nit: Shouldn't there be 2 empty lines here?

Done.

[lijeffrey, 2016-07-12 23:25:14]

lgtm

[chanli, 2016-07-12 23:27:29]

The CQ bit was checked by chanli@chromium.org

[chanli, 2016-07-12 23:27:29]

The patchset sent to the CQ was uploaded after l-g-t-m from stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/2139093002/#ps80001
(title: "Address Jeff's comments")

[commit-bot: I haz the power, 2016-07-12 23:27:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 23:41:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-12 23:41:41]

Try jobs failed on following builders:
  Infra Linux Trusty 64 Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Linux%20Trusty%...)

[chanli, 2016-07-13 00:08:55]

The CQ bit was checked by chanli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 00:09:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 00:23:13]

The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 00:23:14]

Dry run: Try jobs failed on following builders:
  Infra Linux Trusty 64 Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Linux%20Trusty%...)

[chanli, 2016-07-13 00:34:30]

The CQ bit was checked by chanli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 00:34:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 00:49:17]

The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 00:49:18]

Dry run: This issue passed the CQ dry run.

[chanli, 2016-07-13 00:50:50]

The CQ bit was checked by chanli@chromium.org

[chanli, 2016-07-13 00:50:50]

The patchset sent to the CQ was uploaded after l-g-t-m from stgao@chromium.org,
lijeffrey@chromium.org
Link to the patchset: https://codereview.chromium.org/2139093002/#ps100001
(title: ".")

[commit-bot: I haz the power, 2016-07-13 00:50:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 00:54:00]

Description was changed from

==========
[Findit] Trigger swarming tasks after detech_first_faliure_pipeline

BUG=627298
==========

to

==========
[Findit] Trigger swarming tasks after detech_first_faliure_pipeline

BUG=627298

Committed:
https://chromium.googlesource.com/infra/infra/+/668b1e3f1018f5e9493a36dc12686...
==========

[commit-bot: I haz the power, 2016-07-13 00:54:03]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/infra/infra/+/668b1e3f1018f5e9493a36dc12686...