[Findit] Refactoring monitor swarming task pipelines

[Findit] Refactoring monitor swarming task pipelines

BUG=663315
Committed: https://chromium.googlesource.com/infra/infra/+/fbe7208fd7f6feb9caefd206f205160f039b5015

[lijeffrey, 2016-11-08 11:25:14]

Description was changed from

==========
Fixing pylint issues


[Findit] Refactoring monitor swarming task pipelines

BUG=
==========

to

==========
[Findit] Refactoring monitor swarming task pipelines

BUG=663315
==========

[lijeffrey, 2016-11-08 11:25:14]

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

[lijeffrey, 2016-11-08 11:26:50]

ptal

[chanli, 2016-11-08 19:26:00]

https://codereview.chromium.org/2477343003/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2477343003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:37: def
_GetArgs(self):
This function is not necessary in the base class

https://codereview.chromium.org/2477343003/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2477343003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:112:
def run(self, master_name, builder_name, build_number, step_name, task_id,
I think there is no need to change this class...

https://codereview.chromium.org/2477343003/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/process_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2477343003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/process_swarming_task_result_pipeline.py:8: import
time
datatime, logging and time can be removed

https://codereview.chromium.org/2477343003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/process_swarming_task_result_pipeline.py:25: """
Nit: an empty line here?

[lijeffrey, 2016-11-08 21:45:00]

comments addressed, ptal

https://codereview.chromium.org/2477343003/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2477343003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:37: def
_GetArgs(self):
On 2016/11/08 19:26:00, chanli wrote:
> This function is not necessary in the base class

After moving the run logic from process_flake_swarming_task_result_pipeline this
is still needed so I'll keep it here

https://codereview.chromium.org/2477343003/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2477343003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:112:
def run(self, master_name, builder_name, build_number, step_name, task_id,
On 2016/11/08 19:26:00, chanli wrote:
> I think there is no need to change this class...

Done.

https://codereview.chromium.org/2477343003/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/process_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2477343003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/process_swarming_task_result_pipeline.py:8: import
time
On 2016/11/08 19:26:00, chanli wrote:
> datatime, logging and time can be removed

Done.

https://codereview.chromium.org/2477343003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/process_swarming_task_result_pipeline.py:25: """
On 2016/11/08 19:26:00, chanli wrote:
> Nit: an empty line here?

Done.

[chanli, 2016-11-09 00:55:24]

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

https://codereview.chromium.org/2477343003/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:74:
step_name_no_platform = swarming_util.GetTagValue(
We can modify this to:
step_name_no_platform = step_name_no_platform or
swarming_util.GetTagValue(data.get('tags', {}), 'ref_name') 

to reduce function calls

[lijeffrey, 2016-11-09 01:03:21]

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

https://codereview.chromium.org/2477343003/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:74:
step_name_no_platform = swarming_util.GetTagValue(
On 2016/11/09 00:55:24, chanli wrote:
> We can modify this to:
> step_name_no_platform = step_name_no_platform or
> swarming_util.GetTagValue(data.get('tags', {}), 'ref_name') 
> 
> to reduce function calls

Done.

[chanli, 2016-11-09 04:28:28]

lgtm except one comment.

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

https://codereview.chromium.org/2477343003/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:56:
tests_statuses[name][test_run['status']] += 1
Until here the function for flake-swarms and waterfall-swarms are the same,
maybe we can move this part to the base class and use inheritance here?

[lijeffrey, 2016-11-09 10:18:10]

comments addressed, as well as cleaned up unit tests further. mind 1 last round
of review?

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

https://codereview.chromium.org/2477343003/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:56:
tests_statuses[name][test_run['status']] += 1
On 2016/11/09 04:28:28, chanli wrote:
> Until here the function for flake-swarms and waterfall-swarms are the same,
> maybe we can move this part to the base class and use inheritance here?
> 

Done.

[chanli, 2016-11-10 18:42:02]

still LGTM. With one nit.

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

https://codereview.chromium.org/2477343003/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:45: #
are also run, like TEST and PRE_TEST in browser_tests.
Nit: move this comment before ln49?

[lijeffrey, 2016-11-11 17:10:42]

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

https://codereview.chromium.org/2477343003/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:45: #
are also run, like TEST and PRE_TEST in browser_tests.
On 2016/11/10 18:42:01, chanli wrote:
> Nit: move this comment before ln49?

Done.

[lijeffrey, 2016-11-11 17:10:47]

The CQ bit was checked by lijeffrey@chromium.org

[lijeffrey, 2016-11-11 17:10:48]

The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org
Link to the patchset: https://codereview.chromium.org/2477343003/#ps80001
(title: "Addressing nit")

[commit-bot: I haz the power, 2016-11-11 17:10:55]

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-11-11 17:25:33]

Description was changed from

==========
[Findit] Refactoring monitor swarming task pipelines

BUG=663315
==========

to

==========
[Findit] Refactoring monitor swarming task pipelines

BUG=663315

Committed:
https://chromium.googlesource.com/infra/infra/+/fbe7208fd7f6feb9caefd206f2051...
==========

[commit-bot: I haz the power, 2016-11-11 17:25:34]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/infra/infra/+/fbe7208fd7f6feb9caefd206f2051...