[Findit] Adding support for triaging suspected builds from flake analysis

[Findit] Adding support for triaging suspected builds from flake analysis

BUG=656206
Committed: https://chromium.googlesource.com/infra/infra/+/f1b2dc0e2046619c348d18a0a94fbe2bf61de9af

[lijeffrey, 2016-10-14 19:45:35]

Description was changed from

==========
Do not commit yet - establishing baseline working code in case of breakages
[Findit] Adding support for triaging suspected builds from flake analysis

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

to

==========
[Findit] Adding support for triaging suspected builds from flake analysis

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

[lijeffrey, 2016-10-14 19:45:35]

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

[lijeffrey, 2016-10-14 19:45:50]

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

[lijeffrey, 2016-10-14 19:46:15]

ptal

[chanli, 2016-10-14 21:25:09]

Question: do you plan to update WfAnalysis to use base_triage_model in the
future?

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/handle...
File appengine/findit/handlers/flake/triage_flake_analysis.py (right):

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/flake/triage_flake_analysis.py:5: """This module is to
handle manual triage of a suspected flake analysis.
flake result?

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/flake/triage_flake_analysis.py:44: test_name and
version_number and suspected_build_number and
version_number can be get from analysis.

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/model/...
File appengine/findit/model/base_triaged_model.py (right):

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/model/...
appengine/findit/model/base_triaged_model.py:28: version =
ndb.StringProperty(indexed=False)
This is very confusing. I don't think you should mix them together.  For
WfAnalysis, we only have Findit version, and I agree that for
master_flake_analysis, we should also have the version of the analysis, so maybe
have 2 different parameters?

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/model/...
appengine/findit/model/base_triaged_model.py:32: suspect_info =
ndb.StringProperty(default=None, indexed=False)
Based on the comments, this value should not be a string?

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/model/...
appengine/findit/model/base_triaged_model.py:49: triage_history =
ndb.LocalStructuredProperty(
Nit: Do you ming move this to the end of the class? Just not get it surrounded
by functions.

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/model/...
appengine/findit/model/base_triaged_model.py:54: """Gets the triage history of a
WfAnalysis as a list.
This is not necessarily a WfAnalysis, right? Actually only master_flake_analysis
inherits TriagedModel. And why make it static? Is there anything wrong to make
it as below?

def GetTriageHistory(self):
 # proess and return self.triage_history

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

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/templa...
appengine/findit/templates/flake/flake.html:3: ASDFASDFASDFASDF
Is this line needed?

[lijeffrey, 2016-10-14 23:11:31]

Comments addressed, ptal

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/handle...
File appengine/findit/handlers/flake/triage_flake_analysis.py (right):

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/flake/triage_flake_analysis.py:5: """This module is to
handle manual triage of a suspected flake analysis.
On 2016/10/14 21:25:09, chanli wrote:
> flake result?

oops good catch :)

Done.

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/flake/triage_flake_analysis.py:44: test_name and
version_number and suspected_build_number and
On 2016/10/14 21:25:09, chanli wrote:
> version_number can be get from analysis.

Not quite, if I'm understanding your comment correctly. We want to pass the
version number explicitly to tell the datastore exactly which version of mfa to
update. I imagine in the future we would want to be able to do side by side
comparisons of each mfa's version (i.e. make the version number in the triage
history clickable to view different flakiness graphs with different input
parameters for reruns) and it's possible to retroactively triage a result as
correct or incorrect if we change our minds or got it wrong the first time.

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/model/...
File appengine/findit/model/base_triaged_model.py (right):

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/model/...
appengine/findit/model/base_triaged_model.py:28: version =
ndb.StringProperty(indexed=False)
On 2016/10/14 21:25:09, chanli wrote:
> This is very confusing. I don't think you should mix them together.  For
> WfAnalysis, we only have Findit version, and I agree that for
> master_flake_analysis, we should also have the version of the analysis, so
maybe
> have 2 different parameters?

Done.

Created findit_version and version_number.

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/model/...
appengine/findit/model/base_triaged_model.py:32: suspect_info =
ndb.StringProperty(default=None, indexed=False)
On 2016/10/14 21:25:09, chanli wrote:
> Based on the comments, this value should not be a string?

Changed to Json property

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/model/...
appengine/findit/model/base_triaged_model.py:49: triage_history =
ndb.LocalStructuredProperty(
On 2016/10/14 21:25:09, chanli wrote:
> Nit: Do you ming move this to the end of the class? Just not get it surrounded
> by functions.

Done.

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/model/...
appengine/findit/model/base_triaged_model.py:54: """Gets the triage history of a
WfAnalysis as a list.
On 2016/10/14 21:25:09, chanli wrote:
> This is not necessarily a WfAnalysis, right? Actually only
master_flake_analysis
> inherits TriagedModel. And why make it static? Is there anything wrong to make
> it as below?
> 
> def GetTriageHistory(self):
>  # proess and return self.triage_history

Oh yes sorry I got ahead of myself. That is the eventual outcome, WfAnalysis
isn't hooked up to use this new model yet

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

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/templa...
appengine/findit/templates/flake/flake.html:3: ASDFASDFASDFASDF
On 2016/10/14 21:25:09, chanli wrote:
> Is this line needed?

lol

[lijeffrey, 2016-10-14 23:47:25]

Description was changed from

==========
[Findit] Adding support for triaging suspected builds from flake analysis

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

to

==========
[Findit] Adding support for triaging suspected builds from flake analysis

BUG=656206
==========

[chanli, 2016-10-15 01:47:21]

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/model/...
File appengine/findit/model/base_triaged_model.py (right):

https://codereview.chromium.org/2416303002/diff/20001/appengine/findit/model/...
appengine/findit/model/base_triaged_model.py:54: """Gets the triage history of a
WfAnalysis as a list.
On 2016/10/14 23:11:31, lijeffrey wrote:
> On 2016/10/14 21:25:09, chanli wrote:
> > This is not necessarily a WfAnalysis, right? Actually only
> master_flake_analysis
> > inherits TriagedModel. And why make it static? Is there anything wrong to
make
> > it as below?
> > 
> > def GetTriageHistory(self):
> >  # proess and return self.triage_history
> 
> Oh yes sorry I got ahead of myself. That is the eventual outcome, WfAnalysis
> isn't hooked up to use this new model yet

I actually have a second question here: why make the function static?

https://codereview.chromium.org/2416303002/diff/60001/appengine/findit/common...
File appengine/findit/common/base_handler.py (right):

https://codereview.chromium.org/2416303002/diff/60001/appengine/findit/common...
appengine/findit/common/base_handler.py:63: def _ShowDebugInfo(self):  # pragma:
no cover
Do you mind adding a test for this function? Although it should be straight
forward...

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

https://codereview.chromium.org/2416303002/diff/60001/appengine/findit/model/...
appengine/findit/model/base_triaged_model.py:34: suspect_info =
ndb.JsonProperty(default={}, indexed=False)
I remembered default to {} is problematic. One way to avoid it is to use
default=None here, and In UpdateTriageResult make it default to {}

https://codereview.chromium.org/2416303002/diff/60001/appengine/findit/model/...
appengine/findit/model/base_triaged_model.py:63: not analysis.triage_history): 
# pragma: no cover
Not sure if we should check if user is admin here... 

Shouldn't we do this at handler's side and pass the check result to template and
then decide if we display triage_history or not?

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

https://codereview.chromium.org/2416303002/diff/60001/appengine/findit/templa...
appengine/findit/templates/build_failure.html:255: <input type="radio" name="{{
cl_info }}" value="1" onchange="triageSuspectedCL(this)">Incorrect
Thank you for this fix..

[lijeffrey, 2016-10-15 02:16:41]

https://codereview.chromium.org/2416303002/diff/60001/appengine/findit/common...
File appengine/findit/common/base_handler.py (right):

https://codereview.chromium.org/2416303002/diff/60001/appengine/findit/common...
appengine/findit/common/base_handler.py:63: def _ShowDebugInfo(self):  # pragma:
no cover
On 2016/10/15 01:47:21, chanli wrote:
> Do you mind adding a test for this function? Although it should be straight
> forward...

Done.

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

https://codereview.chromium.org/2416303002/diff/60001/appengine/findit/model/...
appengine/findit/model/base_triaged_model.py:34: suspect_info =
ndb.JsonProperty(default={}, indexed=False)
On 2016/10/15 01:47:21, chanli wrote:
> I remembered default to {} is problematic. One way to avoid it is to use
> default=None here, and In UpdateTriageResult make it default to {}

Done.

https://codereview.chromium.org/2416303002/diff/60001/appengine/findit/model/...
appengine/findit/model/base_triaged_model.py:63: not analysis.triage_history): 
# pragma: no cover
On 2016/10/15 01:47:21, chanli wrote:
> Not sure if we should check if user is admin here... 
> 
> Shouldn't we do this at handler's side and pass the check result to template
and
> then decide if we display triage_history or not? 

This function was originally only on the handler side, but is general enough
that other classes can use it too so I moved it to this parent class. I agree
though, the handler side should do this checking and this function should just
return the history

[chanli, 2016-10-15 02:27:31]

On 2016/10/15 02:16:41, lijeffrey wrote:
>
https://codereview.chromium.org/2416303002/diff/60001/appengine/findit/common...
> File appengine/findit/common/base_handler.py (right):
> 
>
https://codereview.chromium.org/2416303002/diff/60001/appengine/findit/common...
> appengine/findit/common/base_handler.py:63: def _ShowDebugInfo(self):  #
pragma:
> no cover
> On 2016/10/15 01:47:21, chanli wrote:
> > Do you mind adding a test for this function? Although it should be straight
> > forward...
> 
> Done.
> 
>
https://codereview.chromium.org/2416303002/diff/60001/appengine/findit/model/...
> File appengine/findit/model/base_triaged_model.py (right):
> 
>
https://codereview.chromium.org/2416303002/diff/60001/appengine/findit/model/...
> appengine/findit/model/base_triaged_model.py:34: suspect_info =
> ndb.JsonProperty(default={}, indexed=False)
> On 2016/10/15 01:47:21, chanli wrote:
> > I remembered default to {} is problematic. One way to avoid it is to use
> > default=None here, and In UpdateTriageResult make it default to {}
> 
> Done.
> 
>
https://codereview.chromium.org/2416303002/diff/60001/appengine/findit/model/...
> appengine/findit/model/base_triaged_model.py:63: not analysis.triage_history):

> # pragma: no cover
> On 2016/10/15 01:47:21, chanli wrote:
> > Not sure if we should check if user is admin here... 
> > 
> > Shouldn't we do this at handler's side and pass the check result to template
> and
> > then decide if we display triage_history or not? 
> 
> This function was originally only on the handler side, but is general enough
> that other classes can use it too so I moved it to this parent class. I agree
> though, the handler side should do this checking and this function should just
> return the history

lgtm

[lijeffrey, 2016-10-15 02:40:51]

The CQ bit was checked by lijeffrey@chromium.org

[lijeffrey, 2016-10-15 02:40:51]

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/2416303002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-15 02:41:01]

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-15 02:55:39]

Description was changed from

==========
[Findit] Adding support for triaging suspected builds from flake analysis

BUG=656206
==========

to

==========
[Findit] Adding support for triaging suspected builds from flake analysis

BUG=656206

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

[commit-bot: I haz the power, 2016-10-15 02:55:40]

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