[Findit] UI change and triage change for cl level trige.

[Findit] UI change and triage change for cl level trige.

When triage the suspected CL(s) of an analysis, update WfSuspectedCL as well. And use the info of WfSuspectedCL on result page.

Also create a script to calculate the accuracy rate for each type of failures:
- compile failure
  - only have heuristic result
  - only have try job result
  - have result from both approach

- test failure
  - only have heuristic result
  - only have try job result
  - have result from both approach

BUG=635181
Committed: https://chromium.googlesource.com/infra/infra/+/be3b619d914654751bff887fc75c15dd8aa3782c

[chanli, 2016-09-21 20:50:03]

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

[chanli, 2016-09-21 20:50:03]

PTAL

[chanli, 2016-09-21 23:23:16]

Hi,

Please refer to following pages:
compile and try_job:
https://chanli-dot-findit-for-me.appspot.com/waterfall/build-failure?url=http...

Test and heuristic:
https://chanli-dot-findit-for-me.appspot.com/waterfall/build-failure?url=http...

Not first time failure(Noted I actually hide the triaging part in this kind of
builds, not sure if it's the right way to handle it):
https://chanli-dot-findit-for-me.appspot.com/waterfall/build-failure?url=http...

[lijeffrey, 2016-09-23 05:09:09]

just some initial comments, i will review more in detial tomorrow

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

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/build_failure.py:40: return None
nit: how about instead of returning None, return {} since this function says it
returns a dict?

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/build_failure.py:325: build_key = '%s/%s/%d' %
(master_name, builder_name, build_number)
nit: how about import base_build_model and use CreateBuildId here instead?

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/build_failure.py:340: print json.dumps(build, indent
=2)
nit: remove print before committing

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/build_failure.py:462: 
nit: too many blank lines

https://codereview.chromium.org/2361583002/diff/40001/appengine/findit/handle...
File appengine/findit/handlers/triage_suspected_cl.py (right):

https://codereview.chromium.org/2361583002/diff/40001/appengine/findit/handle...
appengine/findit/handlers/triage_suspected_cl.py:26: def _UpdateSuspectedCL(
nit: looks like this fits on 1 line?

[stgao, 2016-09-23 19:35:41]

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

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/build_failure.py:325: build_key = '%s/%s/%d' %
(master_name, builder_name, build_number)
On 2016/09/23 05:09:09, lijeffrey wrote:
> nit: how about import base_build_model and use CreateBuildId here instead?

This encoding and decoding should be abstracted in one place as I commented in
the other CL.

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/build_failure.py:326: suspected_cls =
analysis.suspected_cls
Should we do a deep copy first before modifying?

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/build_failure.py:332: cl_object =
WfSuspectedCL.Get(cl['repo_name'], cl['revision'])
Can we map var name to its class_name for easier reading?

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

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/triage_suspected_cl.py:27: repo_name, revision,
build_key, cl_status):
Mind adding comments to explain how we decide the status of a suspected CL and
analysis below too?

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/triage_suspected_cl.py:114: cl_keys =
cl_info.split('/')
Ditto.

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/triage_suspected_cl.py:117: build_key = '%s/%s/%d' %
(master_name, builder_name, build_number)
Ditto.

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

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:83: for (var i=0;
i<findit.suspectedCLs.length; i++) {
nit: space " < ".

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:95: if (status == "0") {
use single quote for consistency?

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/templa...
File appengine/findit/templates/waterfall/compile_failure.html (right):

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/templa...
appengine/findit/templates/waterfall/compile_failure.html:64: <b>Heuristic
result:</b>
Can we have a <br> between try-job result and heuristic result?

[chanli, 2016-09-24 01:09:40]

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

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/build_failure.py:40: return None
On 2016/09/23 05:09:09, lijeffrey wrote:
> nit: how about instead of returning None, return {} since this function says
it
> returns a dict?

Done.

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/build_failure.py:325: build_key = '%s/%s/%d' %
(master_name, builder_name, build_number)
On 2016/09/23 19:35:41, stgao (slow) wrote:
> On 2016/09/23 05:09:09, lijeffrey wrote:
> > nit: how about import base_build_model and use CreateBuildId here instead?
> 
> This encoding and decoding should be abstracted in one place as I commented in
> the other CL.

Done.

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/build_failure.py:325: build_key = '%s/%s/%d' %
(master_name, builder_name, build_number)
On 2016/09/23 05:09:09, lijeffrey wrote:
> nit: how about import base_build_model and use CreateBuildId here instead?

Done.

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/build_failure.py:326: suspected_cls =
analysis.suspected_cls
On 2016/09/23 19:35:41, stgao (slow) wrote:
> Should we do a deep copy first before modifying?

Oops, Done.

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/build_failure.py:332: cl_object =
WfSuspectedCL.Get(cl['repo_name'], cl['revision'])
On 2016/09/23 19:35:41, stgao (slow) wrote:
> Can we map var name to its class_name for easier reading?

Done.

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/build_failure.py:340: print json.dumps(build, indent
=2)
On 2016/09/23 05:09:09, lijeffrey wrote:
> nit: remove print before committing

Done.

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/build_failure.py:462: 
On 2016/09/23 05:09:09, lijeffrey wrote:
> nit: too many blank lines

Done.

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

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/triage_suspected_cl.py:27: repo_name, revision,
build_key, cl_status):
On 2016/09/23 19:35:41, stgao (slow) wrote:
> Mind adding comments to explain how we decide the status of a suspected CL and
> analysis below too?

Done.

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/triage_suspected_cl.py:114: cl_keys =
cl_info.split('/')
On 2016/09/23 19:35:41, stgao (slow) wrote:
> Ditto.

Done.

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/triage_suspected_cl.py:117: build_key = '%s/%s/%d' %
(master_name, builder_name, build_number)
On 2016/09/23 19:35:41, stgao (slow) wrote:
> Ditto.

Done.

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

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:83: for (var i=0;
i<findit.suspectedCLs.length; i++) {
On 2016/09/23 19:35:41, stgao (slow) wrote:
> nit: space " < ".

Done.

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:95: if (status == "0") {
On 2016/09/23 19:35:41, stgao (slow) wrote:
> use single quote for consistency?

Done.

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/templa...
File appengine/findit/templates/waterfall/compile_failure.html (right):

https://codereview.chromium.org/2361583002/diff/20001/appengine/findit/templa...
appengine/findit/templates/waterfall/compile_failure.html:64: <b>Heuristic
result:</b>
On 2016/09/23 19:35:41, stgao (slow) wrote:
> Can we have a <br> between try-job result and heuristic result?

Done.

https://codereview.chromium.org/2361583002/diff/40001/appengine/findit/handle...
File appengine/findit/handlers/triage_suspected_cl.py (right):

https://codereview.chromium.org/2361583002/diff/40001/appengine/findit/handle...
appengine/findit/handlers/triage_suspected_cl.py:26: def _UpdateSuspectedCL(
On 2016/09/23 05:09:09, lijeffrey wrote:
> nit: looks like this fits on 1 line?

Done.

[stgao, 2016-09-28 00:21:27]

https://codereview.chromium.org/2361583002/diff/60001/appengine/findit/handle...
File appengine/findit/handlers/triage_suspected_cl.py (right):

https://codereview.chromium.org/2361583002/diff/60001/appengine/findit/handle...
appengine/findit/handlers/triage_suspected_cl.py:39: # on each build.
This comment is basically how to code does, but it is still not clear in what
cases which statuses will be.

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

https://codereview.chromium.org/2361583002/diff/60001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:83: for (var i=0; i <
findit.suspectedCLs.length; i++) {
As this is a simple table, can we just use NINJA template instead of pure
javascript.

We should not use javascript if possible.

[stgao, 2016-09-28 00:41:52]

One more thing: on
https://chanli-dot-findit-for-me.appspot.com/waterfall/build-failure?url=http...,
there is no list of suspected CLs. As discussed in person, we should fix it
later.

[stgao, 2016-09-28 00:42:50]

And one bug, if I clicked correct or incorrect multiple times, each will be
recorded in the history. But only the first time should be recorded.

[chanli, 2016-10-03 00:00:28]

https://codereview.chromium.org/2361583002/diff/60001/appengine/findit/handle...
File appengine/findit/handlers/triage_suspected_cl.py (right):

https://codereview.chromium.org/2361583002/diff/60001/appengine/findit/handle...
appengine/findit/handlers/triage_suspected_cl.py:39: # on each build.
On 2016/09/28 00:21:27, stgao (slow) wrote:
> This comment is basically how to code does, but it is still not clear in what
> cases which statuses will be.

Done.

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

https://codereview.chromium.org/2361583002/diff/60001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:83: for (var i=0; i <
findit.suspectedCLs.length; i++) {
On 2016/09/28 00:21:27, stgao (slow) wrote:
> As this is a simple table, can we just use NINJA template instead of pure
> javascript.
> 
> We should not use javascript if possible.

Done.

[chanli, 2016-10-05 00:13:03]

ptal

[stgao, 2016-10-06 00:29:52]

lgtm after nits and the bug I mentioned are fixed

https://codereview.chromium.org/2361583002/diff/140001/appengine/findit/handl...
File appengine/findit/handlers/test/triage_suspected_cl_test.py (right):

https://codereview.chromium.org/2361583002/diff/140001/appengine/findit/handl...
appengine/findit/handlers/test/triage_suspected_cl_test.py:296: time_delta =
calendar.timegm(datetime(2016, 7, 1, 00, 00).timetuple())
nit: "time_delta" seems misleading

https://codereview.chromium.org/2361583002/diff/140001/appengine/findit/handl...
appengine/findit/handlers/test/triage_suspected_cl_test.py:346: time_delta =
calendar.timegm(datetime(2016, 7, 1, 00, 00).timetuple())
same here.

https://codereview.chromium.org/2361583002/diff/140001/appengine/findit/templ...
File appengine/findit/templates/build_failure.html (right):

https://codereview.chromium.org/2361583002/diff/140001/appengine/findit/templ...
appengine/findit/templates/build_failure.html:80:
$.getJSON('/waterfall/triage-suspected-cl?cl_info=' + e.name +'&status=' +
e.value + '&url=' + findit.buildUrl, function(data) {
Have we fixed the bug that triaging the same CL as correct or incorrect multiple
times as I showed you?

[chanli, 2016-10-07 22:40:51]

The CQ bit was checked by chanli@chromium.org

[chanli, 2016-10-07 22:40:51]

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/2361583002/#ps160001
(title: ".")

[commit-bot: I haz the power, 2016-10-07 22:40: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-10-07 23:24:11]

Description was changed from

==========
[Findit] UI change and triage change for cl level trige.

BUG=635181
==========

to

==========
[Findit] UI change and triage change for cl level trige.

BUG=635181

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

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

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/infra/infra/+/be3b619d914654751bff887fc75c1...

[chanli1, 2017-04-07 20:03:12]

Description was changed from

==========
[Findit] UI change and triage change for cl level trige.

BUG=635181

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

to

==========
[Findit] UI change and triage change for cl level trige.

When triage the suspected CL(s) of an analysis, update WfSuspectedCL as well.
And use the info of WfSuspectedCL on result page.

Also create a script to calculate the accuracy rate for each type of failures:
- compile failure
  - only have heuristic result
  - only have try job result
  - have result from both approach

- test failure
  - only have heuristic result
  - only have try job result
  - have result from both approach
And use the accuracy rates as confidence score on result page.

BUG=635181

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

[chanli1, 2017-04-07 20:07:28]

Description was changed from

==========
[Findit] UI change and triage change for cl level trige.

When triage the suspected CL(s) of an analysis, update WfSuspectedCL as well.
And use the info of WfSuspectedCL on result page.

Also create a script to calculate the accuracy rate for each type of failures:
- compile failure
  - only have heuristic result
  - only have try job result
  - have result from both approach

- test failure
  - only have heuristic result
  - only have try job result
  - have result from both approach
And use the accuracy rates as confidence score on result page.

BUG=635181

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

to

==========
[Findit] UI change and triage change for cl level trige.

When triage the suspected CL(s) of an analysis, update WfSuspectedCL as well.
And use the info of WfSuspectedCL on result page.

Also create a script to calculate the accuracy rate for each type of failures:
- compile failure
  - only have heuristic result
  - only have try job result
  - have result from both approach

- test failure
  - only have heuristic result
  - only have try job result
  - have result from both approach

BUG=635181

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