[Findit] Refactoring compile recipe return results into report dict instead of list

[Findit] Refactoring compile recipe return results into report dict instead of list

This cl also gets the size of the revision range to report back to Findit which can be used for load estimation.

BUG=577364
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=298484

[lijeffrey, 2016-01-22 02:23:37]

Description was changed from

==========
[Findit] Adding number of commits to build result properties
[Findit] Adding number of commits in range to result build properties

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

to

==========
[Findit] Adding number of commits in range to result build properties

BUG=577364
==========

[lijeffrey, 2016-01-22 02:23:37]

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

[chanli, 2016-01-22 18:05:27]

https://codereview.chromium.org/1615963005/diff/1/scripts/slave/recipes/findi...
File scripts/slave/recipes/findit/chromium/compile.py (right):

https://codereview.chromium.org/1615963005/diff/1/scripts/slave/recipes/findi...
scripts/slave/recipes/findit/chromium/compile.py:142: revisions_to_check)
I think this number is actually
len(failure_info['builds'][current_failure]['blame_list']), right?

[lijeffrey, 2016-01-22 22:26:44]

https://codereview.chromium.org/1615963005/diff/1/scripts/slave/recipes/findi...
File scripts/slave/recipes/findit/chromium/compile.py (right):

https://codereview.chromium.org/1615963005/diff/1/scripts/slave/recipes/findi...
scripts/slave/recipes/findit/chromium/compile.py:142: revisions_to_check)
On 2016/01/22 18:05:27, chanli wrote:
> I think this number is actually
> len(failure_info['builds'][current_failure]['blame_list']), right?

I think you're right, it should be the same. In the case we use failure_info,
then this CL is not needed. If we use that approach then we can re factor
ScheduleTryJobPipeline and MonitorTryJobPipeline to consume failure_info instead
of master_name, builder_name, etc which it can get from failure_info (I plan to
use MonitorTryJobPipeline to handle updating WfTryJobData). The downside is if
for whatever reason if there's a discrepancy between what the try job derived as
the list of revisions to check and what's in failure_info's blame list then the
data stored may not be representative of what the try job would have tried in a
worst-case scenario. What do you guys think?

[stgao, 2016-01-25 22:21:35]

https://codereview.chromium.org/1615963005/diff/1/scripts/slave/recipes/findi...
File scripts/slave/recipes/findit/chromium/compile.py (right):

https://codereview.chromium.org/1615963005/diff/1/scripts/slave/recipes/findi...
scripts/slave/recipes/findit/chromium/compile.py:141:
step_result.presentation.properties['number_of_commits_in_range'] = len(
As discussed offline, we'd better not add a new build properties.
And instead, we could change the ``results`` into a dict and add more
information into it.

[lijeffrey, 2016-01-29 02:01:20]

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

[lijeffrey, 2016-01-29 02:01:21]

Hey guys, this change should update the generated compile results to store them
in a dict instead of list along with metadata for the try job to be consumed by
findit. PTAL when you have the chance :)

[stgao, 2016-01-29 02:15:52]

https://codereview.chromium.org/1615963005/diff/40001/scripts/slave/recipes/f...
File scripts/slave/recipes/findit/chromium/compile.py (right):

https://codereview.chromium.org/1615963005/diff/40001/scripts/slave/recipes/f...
scripts/slave/recipes/findit/chromium/compile.py:117:
'number_of_commits_in_range': len(revisions_to_check)
'regression_range_size' instead?

https://codereview.chromium.org/1615963005/diff/40001/scripts/slave/recipes/f...
scripts/slave/recipes/findit/chromium/compile.py:133: last_result =
compile_result
There seems a bug (not an obvious one): when testing at the current revision N,
there is an infra failure. When the finally block is run, the ``last_result`` is
actually what it means to be and it is for N-1 instead.

https://codereview.chromium.org/1615963005/diff/40001/scripts/slave/recipes/f...
scripts/slave/recipes/findit/chromium/compile.py:146: '<br/>Culprit: %s' %
compile_results[last_revision])
Shouldn't it be last_revision instead? And it could be undefined if line #132 is
not executed when testing the first revision results in an infra error.

https://codereview.chromium.org/1615963005/diff/40001/scripts/slave/recipes/f...
scripts/slave/recipes/findit/chromium/compile.py:154: return compile_results
Let's return the ``report`` instead. To be consistent with the build property
value and the log.

[lijeffrey, 2016-01-29 09:53:42]

https://codereview.chromium.org/1615963005/diff/1/scripts/slave/recipes/findi...
File scripts/slave/recipes/findit/chromium/compile.py (right):

https://codereview.chromium.org/1615963005/diff/1/scripts/slave/recipes/findi...
scripts/slave/recipes/findit/chromium/compile.py:141:
step_result.presentation.properties['number_of_commits_in_range'] = len(
On 2016/01/25 22:21:35, stgao wrote:
> As discussed offline, we'd better not add a new build properties.
> And instead, we could change the ``results`` into a dict and add more
> information into it.

Done.

https://codereview.chromium.org/1615963005/diff/40001/scripts/slave/recipes/f...
File scripts/slave/recipes/findit/chromium/compile.py (right):

https://codereview.chromium.org/1615963005/diff/40001/scripts/slave/recipes/f...
scripts/slave/recipes/findit/chromium/compile.py:117:
'number_of_commits_in_range': len(revisions_to_check)
On 2016/01/29 02:15:52, stgao wrote:
> 'regression_range_size' instead?

Done.

https://codereview.chromium.org/1615963005/diff/40001/scripts/slave/recipes/f...
scripts/slave/recipes/findit/chromium/compile.py:133: last_result =
compile_result
On 2016/01/29 02:15:52, stgao wrote:
> There seems a bug (not an obvious one): when testing at the current revision
N,
> there is an infra failure. When the finally block is run, the ``last_result``
is
> actually what it means to be and it is for N-1 instead.

Done.

https://codereview.chromium.org/1615963005/diff/40001/scripts/slave/recipes/f...
scripts/slave/recipes/findit/chromium/compile.py:146: '<br/>Culprit: %s' %
compile_results[last_revision])
On 2016/01/29 02:15:52, stgao wrote:
> Shouldn't it be last_revision instead? And it could be undefined if line #132
is
> not executed when testing the first revision results in an infra error.

Done.

https://codereview.chromium.org/1615963005/diff/40001/scripts/slave/recipes/f...
scripts/slave/recipes/findit/chromium/compile.py:154: return compile_results
On 2016/01/29 02:15:52, stgao wrote:
> Let's return the ``report`` instead. To be consistent with the build property
> value and the log.

Done.

[stgao, 2016-01-29 16:48:25]

lgtm with a nit.

https://codereview.chromium.org/1615963005/diff/80001/scripts/slave/recipes/f...
File scripts/slave/recipes/findit/chromium/compile.py (right):

https://codereview.chromium.org/1615963005/diff/80001/scripts/slave/recipes/f...
scripts/slave/recipes/findit/chromium/compile.py:134: last_result =
compile_result
We don't have to save the ``last_result``, right?
It could be retrieved by ``compile_result.get(last_revision)``.

[lijeffrey, 2016-01-29 19:55:27]

Description was changed from

==========
[Findit] Adding number of commits in range to result build properties

BUG=577364
==========

to

==========
[Findit] Refactoring return results into report dict instead of list

This cl also gets the size of the revision range to report back to Findit which
can be used for load estimation.

BUG=577364
==========

[lijeffrey, 2016-01-29 20:02:41]

Hey guys, would you mind 1 last quick review?

https://codereview.chromium.org/1615963005/diff/80001/scripts/slave/recipes/f...
File scripts/slave/recipes/findit/chromium/compile.py (right):

https://codereview.chromium.org/1615963005/diff/80001/scripts/slave/recipes/f...
scripts/slave/recipes/findit/chromium/compile.py:134: last_result =
compile_result
On 2016/01/29 16:48:25, stgao wrote:
> We don't have to save the ``last_result``, right?
> It could be retrieved by ``compile_result.get(last_revision)``.

Done.

[lijeffrey, 2016-01-29 20:33:35]

Description was changed from

==========
[Findit] Refactoring return results into report dict instead of list

This cl also gets the size of the revision range to report back to Findit which
can be used for load estimation.

BUG=577364
==========

to

==========
[Findit] Refactoring compile recipe return results into report dict instead of
list

This cl also gets the size of the revision range to report back to Findit which
can be used for load estimation.

BUG=577364
==========

[stgao, 2016-01-29 20:59:59]

On 2016/01/29 20:02:41, lijeffrey wrote:
> Hey guys, would you mind 1 last quick review?

As this CL has conflict with the one by Chan, I will leave this to Chan :)

> 
>
https://codereview.chromium.org/1615963005/diff/80001/scripts/slave/recipes/f...
> File scripts/slave/recipes/findit/chromium/compile.py (right):
> 
>
https://codereview.chromium.org/1615963005/diff/80001/scripts/slave/recipes/f...
> scripts/slave/recipes/findit/chromium/compile.py:134: last_result =
> compile_result
> On 2016/01/29 16:48:25, stgao wrote:
> > We don't have to save the ``last_result``, right?
> > It could be retrieved by ``compile_result.get(last_revision)``.
> 
> Done.

[chanli, 2016-01-29 21:37:39]

On 2016/01/29 20:59:59, stgao wrote:
> On 2016/01/29 20:02:41, lijeffrey wrote:
> > Hey guys, would you mind 1 last quick review?
> 
> As this CL has conflict with the one by Chan, I will leave this to Chan :)
> 
> > 
> >
>
https://codereview.chromium.org/1615963005/diff/80001/scripts/slave/recipes/f...
> > File scripts/slave/recipes/findit/chromium/compile.py (right):
> > 
> >
>
https://codereview.chromium.org/1615963005/diff/80001/scripts/slave/recipes/f...
> > scripts/slave/recipes/findit/chromium/compile.py:134: last_result =
> > compile_result
> > On 2016/01/29 16:48:25, stgao wrote:
> > > We don't have to save the ``last_result``, right?
> > > It could be retrieved by ``compile_result.get(last_revision)``.
> > 
> > Done.

lgtm. I'll adjust my CL according to this change.

[lijeffrey, 2016-01-30 00:11:04]

The CQ bit was checked by lijeffrey@chromium.org

[lijeffrey, 2016-01-30 00:11:06]

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/1615963005/#ps120001
(title: "Fixing whitespace")

[commit-bot: I haz the power, 2016-01-30 00:11:11]

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

[commit-bot: I haz the power, 2016-01-30 00:14:27]

Description was changed from

==========
[Findit] Refactoring compile recipe return results into report dict instead of
list

This cl also gets the size of the revision range to report back to Findit which
can be used for load estimation.

BUG=577364
==========

to

==========
[Findit] Refactoring compile recipe return results into report dict instead of
list

This cl also gets the size of the revision range to report back to Findit which
can be used for load estimation.

BUG=577364

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=298484
==========

[commit-bot: I haz the power, 2016-01-30 00:14:28]

Committed patchset #7 (id:120001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=298484