[Findit] Adding logic to force try jobs regardless of bailout or previous results

[Findit] Adding logic to force try jobs regardless of bailout

BUG=616526
Committed: https://chromium.googlesource.com/infra/infra/+/8c8e25309120e4e69ac183fe4a1a6c3d35a0546d

[lijeffrey, 2016-06-01 17:34:38]

Description was changed from

==========
Fixing presubmit check error


[Findit] Adding logic to force try jobs regardless of bailout

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

to

==========
[Findit] Adding logic to force try jobs regardless of bailout

BUG=616526
==========

[lijeffrey, 2016-06-01 17:34:38]

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

[lijeffrey, 2016-06-01 17:34:47]

ptal

[chanli, 2016-06-01 21:01:59]

https://codereview.chromium.org/2026283002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/test/try_job_util_test.py (right):

https://codereview.chromium.org/2026283002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/test/try_job_util_test.py:144: def
testBailOutForTryJobWithOutdatedTimestampForce(self):
Naming nit: because of 'force' the try job doesn't bail out.

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

https://codereview.chromium.org/2026283002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/try_job_util.py:172: if
_ShouldBailOutForOutdatedBuild(build):
nit: extra space after if.

[stgao, 2016-06-02 01:25:13]

I only reviewed the change on the pipeline part, and didn't find anything
obvious.

I didn't look close to the change on build_failure.html and the handler, as I'm
not that familiar with the code there. Chan wrote those code.

https://codereview.chromium.org/2026283002/diff/1/appengine/findit/handlers/b...
File appengine/findit/handlers/build_failure.py (right):

https://codereview.chromium.org/2026283002/diff/1/appengine/findit/handlers/b...
appengine/findit/handlers/build_failure.py:394:
self.request.get('force_try_job_rerun') == '1')
nit: just "force_try_job"?

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

https://codereview.chromium.org/2026283002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/try_job_util.py:111: need_new_try_job =
force_try_job_rerun
Here, force_try_job_rerun should be False, right?
But it is not clear enough it is False. I'd suggest using False directly.

[lijeffrey, 2016-06-02 05:36:06]

https://codereview.chromium.org/2026283002/diff/1/appengine/findit/handlers/b...
File appengine/findit/handlers/build_failure.py (right):

https://codereview.chromium.org/2026283002/diff/1/appengine/findit/handlers/b...
appengine/findit/handlers/build_failure.py:394:
self.request.get('force_try_job_rerun') == '1')
On 2016/06/02 01:25:13, stgao wrote:
> nit: just "force_try_job"?

Done.

https://codereview.chromium.org/2026283002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/test/try_job_util_test.py (right):

https://codereview.chromium.org/2026283002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/test/try_job_util_test.py:144: def
testBailOutForTryJobWithOutdatedTimestampForce(self):
On 2016/06/01 21:01:59, chanli wrote:
> Naming nit: because of 'force' the try job doesn't bail out.

Done.

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

https://codereview.chromium.org/2026283002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/try_job_util.py:111: need_new_try_job =
force_try_job_rerun
On 2016/06/02 01:25:13, stgao wrote:
> Here, force_try_job_rerun should be False, right?
> But it is not clear enough it is False. I'd suggest using False directly.

Done.

https://codereview.chromium.org/2026283002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/try_job_util.py:172: if
_ShouldBailOutForOutdatedBuild(build):
On 2016/06/01 21:01:59, chanli wrote:
> nit: extra space after if.

what?

[stgao, 2016-06-02 21:30:05]

lgtm for the change on pipelines.

[chanli, 2016-06-02 23:32:52]

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

https://codereview.chromium.org/2026283002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/try_job_util.py:172: if
_ShouldBailOutForOutdatedBuild(build):
On 2016/06/02 05:36:06, lijeffrey wrote:
> On 2016/06/01 21:01:59, chanli wrote:
> > nit: extra space after if.
> 
> what?

Never mind... the underscore disappeared before...

https://codereview.chromium.org/2026283002/diff/20001/appengine/findit/handle...
File appengine/findit/handlers/build_failure.py (right):

https://codereview.chromium.org/2026283002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/build_failure.py:256: (tests == [NON_SWARMING] and
show_debug_info)):
I don't understand here: I guess this is to display try job info for
unclassified_test in debug mode? But that would be covered by
try_job_result['status'] in NO_TRY_JOB_REASON_MAP.values(), right?

https://codereview.chromium.org/2026283002/diff/20001/appengine/findit/handle...
File appengine/findit/handlers/handlers_util.py (right):

https://codereview.chromium.org/2026283002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/handlers_util.py:317: try_job['ref_name'] =
task.get('ref_name')
If this is a force-try-job for a non-swarming task, there will not be a  task,
thus no task.get('ref_name'). I think you need to process the step name(strip
the 'on platform'. e.g. unit_tests on Ubuntu-12.04 --> unit_tests)

https://codereview.chromium.org/2026283002/diff/20001/appengine/findit/templa...
File appengine/findit/templates/build_failure.html (right):

https://codereview.chromium.org/2026283002/diff/20001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:348: function
displayUnclassifiedFailuresToAdmins(step_name, results) {
Just a thought: could we re-use some code from displayUnclassifiedFailures?

[lijeffrey, 2016-06-27 22:15:43]

ptal

https://codereview.chromium.org/2026283002/diff/20001/appengine/findit/handle...
File appengine/findit/handlers/build_failure.py (right):

https://codereview.chromium.org/2026283002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/build_failure.py:256: (tests == [NON_SWARMING] and
show_debug_info)):
On 2016/06/02 23:32:52, chanli wrote:
> I don't understand here: I guess this is to display try job info for
> unclassified_test in debug mode? But that would be covered by
> try_job_result['status'] in NO_TRY_JOB_REASON_MAP.values(), right?

Done. You're right, this isn't needed here.

https://codereview.chromium.org/2026283002/diff/20001/appengine/findit/handle...
File appengine/findit/handlers/handlers_util.py (right):

https://codereview.chromium.org/2026283002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/handlers_util.py:317: try_job['ref_name'] =
task.get('ref_name')
On 2016/06/02 23:32:52, chanli wrote:
> If this is a force-try-job for a non-swarming task, there will not be a  task,
> thus no task.get('ref_name'). I think you need to process the step name(strip
> the 'on platform'. e.g. unit_tests on Ubuntu-12.04 --> unit_tests)

Done.

https://codereview.chromium.org/2026283002/diff/20001/appengine/findit/templa...
File appengine/findit/templates/build_failure.html (right):

https://codereview.chromium.org/2026283002/diff/20001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:348: function
displayUnclassifiedFailuresToAdmins(step_name, results) {
On 2016/06/02 23:32:52, chanli wrote:
> Just a thought: could we re-use some code from displayUnclassifiedFailures?

This whole file should probably be refactored , we can do it in a separate
change :)

[chanli, 2016-06-27 22:18:40]

On 2016/06/27 22:15:43, lijeffrey wrote:
> ptal
> 
>
https://codereview.chromium.org/2026283002/diff/20001/appengine/findit/handle...
> File appengine/findit/handlers/build_failure.py (right):
> 
>
https://codereview.chromium.org/2026283002/diff/20001/appengine/findit/handle...
> appengine/findit/handlers/build_failure.py:256: (tests == [NON_SWARMING] and
> show_debug_info)):
> On 2016/06/02 23:32:52, chanli wrote:
> > I don't understand here: I guess this is to display try job info for
> > unclassified_test in debug mode? But that would be covered by
> > try_job_result['status'] in NO_TRY_JOB_REASON_MAP.values(), right?
> 
> Done. You're right, this isn't needed here.
> 
>
https://codereview.chromium.org/2026283002/diff/20001/appengine/findit/handle...
> File appengine/findit/handlers/handlers_util.py (right):
> 
>
https://codereview.chromium.org/2026283002/diff/20001/appengine/findit/handle...
> appengine/findit/handlers/handlers_util.py:317: try_job['ref_name'] =
> task.get('ref_name')
> On 2016/06/02 23:32:52, chanli wrote:
> > If this is a force-try-job for a non-swarming task, there will not be a 
task,
> > thus no task.get('ref_name'). I think you need to process the step
name(strip
> > the 'on platform'. e.g. unit_tests on Ubuntu-12.04 --> unit_tests)
> 
> Done.
> 
>
https://codereview.chromium.org/2026283002/diff/20001/appengine/findit/templa...
> File appengine/findit/templates/build_failure.html (right):
> 
>
https://codereview.chromium.org/2026283002/diff/20001/appengine/findit/templa...
> appengine/findit/templates/build_failure.html:348: function
> displayUnclassifiedFailuresToAdmins(step_name, results) {
> On 2016/06/02 23:32:52, chanli wrote:
> > Just a thought: could we re-use some code from displayUnclassifiedFailures?
> 
> This whole file should probably be refactored , we can do it in a separate
> change :)

lgtm

[lijeffrey, 2016-06-27 22:37:41]

The CQ bit was checked by lijeffrey@chromium.org

[lijeffrey, 2016-06-27 22:37:42]

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/2026283002/#ps40001
(title: "Addressing comments  and rebase")

[commit-bot: I haz the power, 2016-06-27 22:37:51]

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-06-27 22:59:49]

Description was changed from

==========
[Findit] Adding logic to force try jobs regardless of bailout

BUG=616526
==========

to

==========
[Findit] Adding logic to force try jobs regardless of bailout

BUG=616526

Committed:
https://chromium.googlesource.com/infra/infra/+/8c8e25309120e4e69ac183fe4a1a6...
==========

[commit-bot: I haz the power, 2016-06-27 22:59:49]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/infra/infra/+/8c8e25309120e4e69ac183fe4a1a6...