[Findit] Using master flake analysis key for updating triage results

[Findit] Using master flake analysis key for updating triage results

Previous internal error was due to sending the information needed to associate the result with the master flake analysis being encoded as a string 'master_name/builder_name/build_number/step_name/test_name', then splitting by '/'. However, many test names contain '/'s, leading to incorrect extraction of key components. This change encodes step and test names on the template side when passing to the triage handler, and explicitly specifies each component.

BUG=670512
Committed: https://chromium.googlesource.com/infra/infra/+/20521525601d389a3b799bb2b6a5a9479fc94c13

[lijeffrey, 2016-12-07 19:59:47]

Description was changed from

==========
[Findit] Encoding step and test name to avoid special characters causing issues

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

to

==========
[Findit] Encoding step and test name in flake checker when triaging

Previous internal error was due to sending the information needed to associate
the result with the master flake analysis being encoded as a string
'master_name/builder_name/build_number/step_name/test_name', then splitting by
'/'. However, many test names contain '/'s, leading to incorrect extraction of
key components. This change encodes step and test names on the template side
when passing to the triage handler, and explicitly specifies each component.

BUG=670512
==========

[lijeffrey, 2016-12-07 19:59:47]

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

[lijeffrey, 2016-12-07 20:00:48]

Currently my "git cl upload" is configured to use my @google account which
gerrit doesn't like. In order to get this change reviewed quicker I'm using
rietveld for this CL until I get gerrit configured correctly.

PTAL

[stgao, 2016-12-07 20:12:50]

https://codereview.chromium.org/2554143003/diff/1/appengine/findit/handlers/f...
File appengine/findit/handlers/flake/triage_flake_analysis.py (right):

https://codereview.chromium.org/2554143003/diff/1/appengine/findit/handlers/f...
appengine/findit/handlers/flake/triage_flake_analysis.py:48: version_number =
self.request.get('version_number').strip()
Would it be easier if we use the key.urlsafe() just like Chan's CL yesterday?

[chanli, 2016-12-07 20:56:12]

https://codereview.chromium.org/2554143003/diff/1/appengine/findit/handlers/f...
File appengine/findit/handlers/flake/triage_flake_analysis.py (right):

https://codereview.chromium.org/2554143003/diff/1/appengine/findit/handlers/f...
appengine/findit/handlers/flake/triage_flake_analysis.py:48: version_number =
self.request.get('version_number').strip()
On 2016/12/07 20:12:50, stgao (slow on Monday) wrote:
> Would it be easier if we use the key.urlsafe() just like Chan's CL yesterday?

Agreed.

[lijeffrey, 2016-12-07 22:02:03]

https://codereview.chromium.org/2554143003/diff/1/appengine/findit/handlers/f...
File appengine/findit/handlers/flake/triage_flake_analysis.py (right):

https://codereview.chromium.org/2554143003/diff/1/appengine/findit/handlers/f...
appengine/findit/handlers/flake/triage_flake_analysis.py:48: version_number =
self.request.get('version_number').strip()
On 2016/12/07 20:12:50, stgao (slow on Monday) wrote:
> Would it be easier if we use the key.urlsafe() just like Chan's CL yesterday?

That's much cleaner! Done.

[chanli, 2016-12-07 22:22:52]

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

https://codereview.chromium.org/2554143003/diff/20001/appengine/findit/handle...
appengine/findit/handlers/flake/triage_flake_analysis.py:43:
suspected_build_number = self.request.get('suspected_build_number').strip()
I think suspected_build_number is actually
master_flake_analysis.suspected_flake_build_number? Do we need to have this
parameter?

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

https://codereview.chromium.org/2554143003/diff/20001/appengine/findit/templa...
appengine/findit/templates/flake/result.html:61:
parameters['suspected_build_number'] = {{ suspected_flake.build_number }};
Same question as above.

[stgao, 2016-12-08 00:14:12]

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

https://codereview.chromium.org/2554143003/diff/20001/appengine/findit/handle...
appengine/findit/handlers/flake/triage_flake_analysis.py:29:
master_flake_analysis.UpdateTriageResult(
Do we assert that the analysis is completed and have a finding?

https://codereview.chromium.org/2554143003/diff/20001/appengine/findit/handle...
appengine/findit/handlers/flake/triage_flake_analysis.py:43:
suspected_build_number = self.request.get('suspected_build_number').strip()
On 2016/12/07 22:22:52, chanli wrote:
> I think suspected_build_number is actually
> master_flake_analysis.suspected_flake_build_number? Do we need to have this
> parameter?

+1

https://codereview.chromium.org/2554143003/diff/20001/appengine/findit/handle...
appengine/findit/handlers/flake/triage_flake_analysis.py:46: if not (key_urlsafe
and suspected_build_number and str(triage_result)):
Should we test "triage_result is not None" instead?

[lijeffrey, 2016-12-08 01:23:46]

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

https://codereview.chromium.org/2554143003/diff/20001/appengine/findit/handle...
appengine/findit/handlers/flake/triage_flake_analysis.py:29:
master_flake_analysis.UpdateTriageResult(
On 2016/12/08 00:14:12, stgao (slow on Monday) wrote:
> Do we assert that the analysis is completed and have a finding?

Added for robustness, but it shouldn't be necessary. There should be no
suspected flake build number if the analysis is not completed, and with no build
number there's no way to triage the result and thus no way to reach this code.

https://codereview.chromium.org/2554143003/diff/20001/appengine/findit/handle...
appengine/findit/handlers/flake/triage_flake_analysis.py:43:
suspected_build_number = self.request.get('suspected_build_number').strip()
On 2016/12/08 00:14:12, stgao (slow on Monday) wrote:
> On 2016/12/07 22:22:52, chanli wrote:
> > I think suspected_build_number is actually
> > master_flake_analysis.suspected_flake_build_number? Do we need to have this
> > parameter?
> 
> +1

Done.

https://codereview.chromium.org/2554143003/diff/20001/appengine/findit/handle...
appengine/findit/handlers/flake/triage_flake_analysis.py:46: if not (key_urlsafe
and suspected_build_number and str(triage_result)):
On 2016/12/08 00:14:12, stgao (slow on Monday) wrote:
> Should we test "triage_result is not None" instead?

Done.

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

https://codereview.chromium.org/2554143003/diff/20001/appengine/findit/templa...
appengine/findit/templates/flake/result.html:61:
parameters['suspected_build_number'] = {{ suspected_flake.build_number }};
On 2016/12/07 22:22:52, chanli wrote:
> Same question as above.

Good catch, done.

[stgao, 2016-12-08 01:39:54]

CL description is not up-to-date.

Code lgtm.

[lijeffrey, 2016-12-08 02:53:31]

Description was changed from

==========
[Findit] Encoding step and test name in flake checker when triaging

Previous internal error was due to sending the information needed to associate
the result with the master flake analysis being encoded as a string
'master_name/builder_name/build_number/step_name/test_name', then splitting by
'/'. However, many test names contain '/'s, leading to incorrect extraction of
key components. This change encodes step and test names on the template side
when passing to the triage handler, and explicitly specifies each component.

BUG=670512
==========

to

==========
[Findit] Using master flake analysis key for updating triage results

Previous internal error was due to sending the information needed to associate
the result with the master flake analysis being encoded as a string
'master_name/builder_name/build_number/step_name/test_name', then splitting by
'/'. However, many test names contain '/'s, leading to incorrect extraction of
key components. This change encodes step and test names on the template side
when passing to the triage handler, and explicitly specifies each component.

BUG=670512
==========

[lijeffrey, 2016-12-08 02:54:25]

Fixed description. Thanks!

[lijeffrey, 2016-12-08 02:54:29]

The CQ bit was checked by lijeffrey@chromium.org

[commit-bot: I haz the power, 2016-12-08 02:54:34]

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-12-08 03:09:19]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481165669330500,
"parent_rev": "74f834f757140b44132da8da6af7d6f953aaf77a", "commit_rev":
"20521525601d389a3b799bb2b6a5a9479fc94c13"}

[commit-bot: I haz the power, 2016-12-08 03:09:22]

Description was changed from

==========
[Findit] Using master flake analysis key for updating triage results

Previous internal error was due to sending the information needed to associate
the result with the master flake analysis being encoded as a string
'master_name/builder_name/build_number/step_name/test_name', then splitting by
'/'. However, many test names contain '/'s, leading to incorrect extraction of
key components. This change encodes step and test names on the template side
when passing to the triage handler, and explicitly specifies each component.

BUG=670512
==========

to

==========
[Findit] Using master flake analysis key for updating triage results

Previous internal error was due to sending the information needed to associate
the result with the master flake analysis being encoded as a string
'master_name/builder_name/build_number/step_name/test_name', then splitting by
'/'. However, many test names contain '/'s, leading to incorrect extraction of
key components. This change encodes step and test names on the template side
when passing to the triage handler, and explicitly specifies each component.

BUG=670512

Committed:
https://chromium.googlesource.com/infra/infra/+/20521525601d389a3b799bb2b6a5a...
==========

[commit-bot: I haz the power, 2016-12-08 03:09:23]

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