[Findit] Modify result page to display tryjob result for test failure.

[Findit] Modify result page to display tryjob result for test failure.
BUG=583806
Committed: https://chromium.googlesource.com/infra/infra/+/be1759165bf708024d8ae21f91d7e471e4896c57

[chanli, 2016-01-30 04:02:49]

Hi,

This is the CL that modify result page to display tryjob result for test
failure. please take a look. Thank you.

[chanli, 2016-02-01 19:03:49]

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

[chanli, 2016-02-02 00:37:01]

Hi,

I have included the change on the template. Please take a look.

[stgao, 2016-02-02 21:08:33]

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/handle...
File appengine/findit/handlers/test/try_job_result_test.py (right):

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/handle...
appengine/findit/handlers/test/try_job_result_test.py:232: 'compile':{
nit: space ": {".

I guess pylint or gpylint will help on formatting.

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

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:19: point_build_keys =
try_job_key.split('/')
what does 'point build' mean? Referred build?

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:53: culprit_info['commit_position']
= culprit['commit_position']
IIRC, in the other CL, we might just provide the revision(git hash) only.
Thus, commit_position and review_url might not be available and the code here
will cause runtime exception.

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

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:266: tableString =
'<table><tr><th>Step</th><th>Test</th><th>Try
Job</th><th>Status</th><th>Culprit</th></tr>';
Define local var instead of global one?

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:270: tableString += '<td>' +
value.test + '</td>';
Just to double check, the value.test will be 'N/A' if the failure is step-level
like compile, right?

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:281: status_class = 'running';
Besides these, there is one more status -- scheduled, right?

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:285: tableString += '<td><a
href=">' + value.review_url + '">' + value.commit_position || value.revision +
'</a></td>';
note: review url might not be available.

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:368: <div id='try_job'>
I guess this should be hidden by default.

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

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:184: 'compile':
culprits[failed_revisions[0]]
use failed_revision directly instead?

[chanli, 2016-02-02 22:18:13]

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/handle...
File appengine/findit/handlers/test/try_job_result_test.py (right):

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/handle...
appengine/findit/handlers/test/try_job_result_test.py:232: 'compile':{
On 2016/02/02 21:08:32, stgao wrote:
> nit: space ": {".
> 
> I guess pylint or gpylint will help on formatting.

Done.

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

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:19: point_build_keys =
try_job_key.split('/')
On 2016/02/02 21:08:32, stgao wrote:
> what does 'point build' mean? Referred build?

Right.. Done.

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:53: culprit_info['commit_position']
= culprit['commit_position']
On 2016/02/02 21:08:32, stgao wrote:
> IIRC, in the other CL, we might just provide the revision(git hash) only.
> Thus, commit_position and review_url might not be available and the code here
> will cause runtime exception.

Done.

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

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:266: tableString =
'<table><tr><th>Step</th><th>Test</th><th>Try
Job</th><th>Status</th><th>Culprit</th></tr>';
On 2016/02/02 21:08:32, stgao wrote:
> Define local var instead of global one?

Done.

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:270: tableString += '<td>' +
value.test + '</td>';
On 2016/02/02 21:08:32, stgao wrote:
> Just to double check, the value.test will be 'N/A' if the failure is
step-level
> like compile, right?

Yes.

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:281: status_class = 'running';
On 2016/02/02 21:08:32, stgao wrote:
> Besides these, there is one more status -- scheduled, right?

Right. But scheduled and running will share the same CSS class.

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:285: tableString += '<td><a
href=">' + value.review_url + '">' + value.commit_position || value.revision +
'</a></td>';
On 2016/02/02 21:08:33, stgao wrote:
> note: review url might not be available.

Done.

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:368: <div id='try_job'>
On 2016/02/02 21:08:32, stgao wrote:
> I guess this should be hidden by default.

Done.

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

https://codereview.chromium.org/1652603003/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:184: 'compile':
culprits[failed_revisions[0]]
On 2016/02/02 21:08:33, stgao wrote:
> use failed_revision directly instead?

Done.

[chanli, 2016-02-02 22:30:40]

Found a small bug... fixed it.

[stgao, 2016-02-02 22:51:28]

lgtm with nits.

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

https://codereview.chromium.org/1652603003/diff/60001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:68: if
isinstance(failure_result_map[failed_step], dict):
nit: add a comment to explain why dict is tested here. And what does it mean for
a dict and a raw string.

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

https://codereview.chromium.org/1652603003/diff/60001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:281: status_class = 'running';
nit: Maybe add a comment for the CSS class sharing.

[chanli, 2016-02-02 23:38:08]

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

https://codereview.chromium.org/1652603003/diff/60001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:68: if
isinstance(failure_result_map[failed_step], dict):
On 2016/02/02 22:51:27, stgao wrote:
> nit: add a comment to explain why dict is tested here. And what does it mean
for
> a dict and a raw string.

Done.

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

https://codereview.chromium.org/1652603003/diff/60001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:281: status_class = 'running';
On 2016/02/02 22:51:27, stgao wrote:
> nit: Maybe add a comment for the CSS class sharing.

Done.

[lijeffrey, 2016-02-03 06:08:37]

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
File appengine/findit/handlers/try_job_result.py (right):

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:22: if try_job:
nit: maybe to make this slightly more readable do something like

if not try_job:
  return

if try_job.compile_results:
  ...

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:46: if step in try_job_culprits:  #
pragma: no cover
is it possible to make a test that covers this?

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:47: if not
try_job_culprits[step].get('tests',{}):  # Only step level
nit: whitespace 'tests', {}

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:52: culprit_info['revision'] =
culprit['revision']
nit: since the others are using .get, why not use .get('rivision') here as well?

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:62: if not analysis:
nit: why not keep this the same, then declare culprits_info and try_job_keys
after the if statement?

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:67: # failure_result_map uses
step_names as keys and save referred_try_job_keys.
what's referred_try_job_key?

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:67: # failure_result_map uses
step_names as keys and save referred_try_job_keys.
nit: saves

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:68: # If non-swarming, step_name and
referred_try_job_key matches directly as:
nit: match

[qyearsley, 2016-02-03 18:38:28]

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
File appengine/findit/handlers/try_job_result.py (right):

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:21: referred_build_keys[0],
referred_build_keys[1], referred_build_keys[2])
Nit: indent should be 4 spaces

Also, if referred_build_keys should always have a length of 3, you could also
do:
try_job = WfTryJob.Get(*referred_build_keys)

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:52: culprit_info['revision'] =
culprit['revision']
On 2016/02/03 at 06:08:37, lijeffrey wrote:
> nit: since the others are using .get, why not use .get('rivision') here as
well?

Is "revision" required (cannot be None) and the others are optional (can be
None)? If so, then using bracket notation seems OK to me since it implies 'I
want a KeyError to be raised if "revision" is not in culprit_info'.

[chanli, 2016-02-03 19:20:28]

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
File appengine/findit/handlers/try_job_result.py (right):

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:22: if try_job:
On 2016/02/03 06:08:37, lijeffrey wrote:
> nit: maybe to make this slightly more readable do something like
> 
> if not try_job:
>   return
> 
> if try_job.compile_results:
>   ...

Done.

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:46: if step in try_job_culprits:  #
pragma: no cover
On 2016/02/03 06:08:37, lijeffrey wrote:
> is it possible to make a test that covers this?

This line has been removed since it'll never be reached.

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:47: if not
try_job_culprits[step].get('tests',{}):  # Only step level
On 2016/02/03 06:08:37, lijeffrey wrote:
> nit: whitespace 'tests', {}

Done.

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:52: culprit_info['revision'] =
culprit['revision']
On 2016/02/03 06:08:37, lijeffrey wrote:
> nit: since the others are using .get, why not use .get('rivision') here as
well?

This change is actually optional because revision is guaranteed but not the
other 2.

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:62: if not analysis:
On 2016/02/03 06:08:37, lijeffrey wrote:
> nit: why not keep this the same, then declare culprits_info and try_job_keys
> after the if statement?

To be consistent with below, I'm returning culprits_info here.

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:67: # failure_result_map uses
step_names as keys and save referred_try_job_keys.
On 2016/02/03 06:08:37, lijeffrey wrote:
> nit: saves

Done.

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:67: # failure_result_map uses
step_names as keys and save referred_try_job_keys.
On 2016/02/03 06:08:37, lijeffrey wrote:
> what's referred_try_job_key?

Is 'referred try_job_key' better? Basically it is the key point to try job
result of first failed build.

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:68: # If non-swarming, step_name and
referred_try_job_key matches directly as:
On 2016/02/03 06:08:37, lijeffrey wrote:
> nit: match

Done.

[chanli, 2016-02-03 19:24:42]

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
File appengine/findit/handlers/try_job_result.py (right):

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:21: referred_build_keys[0],
referred_build_keys[1], referred_build_keys[2])
On 2016/02/03 18:38:28, qyearsley wrote:
> Nit: indent should be 4 spaces
> 
> Also, if referred_build_keys should always have a length of 3, you could also
> do:
> try_job = WfTryJob.Get(*referred_build_keys)

Done.

https://codereview.chromium.org/1652603003/diff/80001/appengine/findit/handle...
appengine/findit/handlers/try_job_result.py:52: culprit_info['revision'] =
culprit['revision']
On 2016/02/03 18:38:28, qyearsley wrote:
> On 2016/02/03 at 06:08:37, lijeffrey wrote:
> > nit: since the others are using .get, why not use .get('rivision') here as
> well?
> 
> Is "revision" required (cannot be None) and the others are optional (can be
> None)? If so, then using bracket notation seems OK to me since it implies 'I
> want a KeyError to be raised if "revision" is not in culprit_info'.

It's actually more of culprit is created only when there is a revision.

[lijeffrey, 2016-02-03 19:44:42]

lgtm

[chanli, 2016-02-04 00:32:50]

The CQ bit was checked by chanli@chromium.org

[chanli, 2016-02-04 00:32: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/1652603003/#ps130001
(title: ".")

[commit-bot: I haz the power, 2016-02-04 00:32:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1652603003/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1652603003/130001

[chanli, 2016-02-04 00:33:07]

Description was changed from

==========
[Findit] Modify result page to display tryjob result for test failure.
==========

to

==========
[Findit] Modify result page to display tryjob result for test failure.
BUG=583806
==========

[commit-bot: I haz the power, 2016-02-04 00:35:33]

Description was changed from

==========
[Findit] Modify result page to display tryjob result for test failure.
BUG=583806
==========

to

==========
[Findit] Modify result page to display tryjob result for test failure.
BUG=583806

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

[commit-bot: I haz the power, 2016-02-04 00:35:34]

Committed patchset #8 (id:130001) as
https://chromium.googlesource.com/infra/infra/+/be1759165bf708024d8ae21f91d7e...