[Sheriff-o-matic] Use likely_revisions instead of first_failing/last_passing

[Sheriff-o-matic] Use likely_revisions instead of first_failing/last_passing

This will help in the transition to git hashes.

BUG=400408
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180933

[Mathieu, 2014-08-22 19:26:48]

Hi,

Please have a look!

[ojan, 2014-08-23 01:23:06]

Thanks for rushing this through.

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/mo...
File Tools/GardeningServer/model/ct-commit-list.html (right):

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-commit-list.html:38:
CTCommitList.prototype._getCommitsForRepo = function(repository) {
Nit: What was wrong with the old name?

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/mo...
File Tools/GardeningServer/model/ct-commit-log.html (right):

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-commit-log.html:63: if
(repoRevision.lastIndexOf(repository, 0) !== 0)
Use startsWith. http://sugarjs.com/api/String/startsWith

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/mo...
File Tools/GardeningServer/model/ct-failure-group.html (right):

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-failure-group.html:13: this.revisions =
revisions.slice() || [];
No need for the || here, right?

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/mo...
File Tools/GardeningServer/model/ct-failure.html (left):

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-failure.html:13: // FIXME: Move these up one
layer to the groupedFailures object.
Yay!

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-failure-analyzer.html (right):

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:50: // At this point
|a.revisions| and |b.revisions| are sorted lists of
It turns out that we'll be able to keep using git commit positions (the
equivalent of svn revisions) at least for the short-term. So, we should be able
to keep this mostly the same as how it is today. Will just need to change how we
get the revision numbers above. Sorry for the churn. I didn't realize that we'd
be able to use git commit positions till I talked to stip today.

Down the road, we should still change this code to be git hash friendly, but the
way I want to do that is to expose timestamps in the JSON from the server, so
the logic here would stay the same still, it would just use timestamps instead
of revision numbers.

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:54: var
_getLatestRevisionForTree = function(revisions) {
This should have a FIXME to sort by timestamp of the last (or first?) commit.

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:124:
_processFailuresForRangeGroup: function(range_group, alerts, annotations) {
s/range_group/rangeGroup/

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:155: // Get the actual tree on
which this failure occurred ('blink', 'chromium')
IMO, this is pretty clear. No need to give comments describing what the code
clearly does. Comments should be to explain high-level design or to explain why
the code is doing something non-obvious.

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:158: // FIXME: No tree
corresponding to master URL!
FIXMEs should state what to fix. Something like...
// FIXME: Figure out what tree masters that aren't in masterToTree should have.

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:159: return;
We don't actually want to early return here. We'll soon have the default mode in
sheriff-o-matic be to show all failures, even ones that don't have a tree. Was
not early returning causing JS errors or something?

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:177: // Skip further
processing if no failures were collected (e.g. the tree
Ditto: This is clearly what the next line of code does.

[Mathieu, 2014-08-24 23:42:11]

Have a look!

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/mo...
File Tools/GardeningServer/model/ct-commit-list.html (right):

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-commit-list.html:38:
CTCommitList.prototype._getCommitsForRepo = function(repository) {
On 2014/08/23 01:23:05, ojan-only-code-yellow-reviews wrote:
> Nit: What was wrong with the old name?

As a new comer to the project it initially confused me since we also have
_commits in another file. Let me know if you feel strongly, I'll change it back.

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/mo...
File Tools/GardeningServer/model/ct-commit-log.html (right):

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-commit-log.html:63: if
(repoRevision.lastIndexOf(repository, 0) !== 0)
On 2014/08/23 01:23:05, ojan-only-code-yellow-reviews wrote:
> Use startsWith. http://sugarjs.com/api/String/startsWith

Done.

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/mo...
File Tools/GardeningServer/model/ct-failure-group.html (right):

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-failure-group.html:13: this.revisions =
revisions.slice() || [];
On 2014/08/23 01:23:05, ojan-only-code-yellow-reviews wrote:
> No need for the || here, right?

Done.

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/mo...
File Tools/GardeningServer/model/ct-failure.html (left):

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/mo...
Tools/GardeningServer/model/ct-failure.html:13: // FIXME: Move these up one
layer to the groupedFailures object.
On 2014/08/23 01:23:05, ojan-only-code-yellow-reviews wrote:
> Yay!

Acknowledged.

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-failure-analyzer.html (right):

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:50: // At this point
|a.revisions| and |b.revisions| are sorted lists of
On 2014/08/23 01:23:06, ojan-only-code-yellow-reviews wrote:
> It turns out that we'll be able to keep using git commit positions (the
> equivalent of svn revisions) at least for the short-term. So, we should be
able
> to keep this mostly the same as how it is today. Will just need to change how
we
> get the revision numbers above. Sorry for the churn. I didn't realize that
we'd
> be able to use git commit positions till I talked to stip today.
> 
> Down the road, we should still change this code to be git hash friendly, but
the
> way I want to do that is to expose timestamps in the JSON from the server, so
> the logic here would stay the same still, it would just use timestamps instead
> of revision numbers.

No problem. Are you fine with this implementation then? It's not perfect, but it
works well for hashes and numbers.

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:54: var
_getLatestRevisionForTree = function(revisions) {
On 2014/08/23 01:23:06, ojan-only-code-yellow-reviews wrote:
> This should have a FIXME to sort by timestamp of the last (or first?) commit.

Done.

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:124:
_processFailuresForRangeGroup: function(range_group, alerts, annotations) {
On 2014/08/23 01:23:05, ojan-only-code-yellow-reviews wrote:
> s/range_group/rangeGroup/

Done.

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:155: // Get the actual tree on
which this failure occurred ('blink', 'chromium')
On 2014/08/23 01:23:06, ojan-only-code-yellow-reviews wrote:
> IMO, this is pretty clear. No need to give comments describing what the code
> clearly does. Comments should be to explain high-level design or to explain
why
> the code is doing something non-obvious.

Sorry, I was annotating as I went along.

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:158: // FIXME: No tree
corresponding to master URL!
On 2014/08/23 01:23:05, ojan-only-code-yellow-reviews wrote:
> FIXMEs should state what to fix. Something like...
> // FIXME: Figure out what tree masters that aren't in masterToTree should
have.

Done.

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:159: return;
On 2014/08/23 01:23:05, ojan-only-code-yellow-reviews wrote:
> We don't actually want to early return here. We'll soon have the default mode
in
> sheriff-o-matic be to show all failures, even ones that don't have a tree. Was
> not early returning causing JS errors or something?

Nah, it just didn't seem clean to be using |tree| as undefined here.

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:177: // Skip further
processing if no failures were collected (e.g. the tree
On 2014/08/23 01:23:05, ojan-only-code-yellow-reviews wrote:
> Ditto: This is clearly what the next line of code does.

Done.

[ojan, 2014-08-25 03:54:13]

lgtm, but please change the implementation of the comparator back before
committing.

Thanks for fixing this. While this didn't end up being necessary for the git
migration, it lays the groundwork for fixing the bug where we show branch
commits (i.e. builder_alerts can filter out branch commits from the
likely_revisions list for us) and just generally making the code better.

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-failure-analyzer.html (right):

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:50: // At this point
|a.revisions| and |b.revisions| are sorted lists of
On 2014/08/24 23:42:11, Mathieu Perreault wrote:
> On 2014/08/23 01:23:06, ojan-only-code-yellow-reviews wrote:
> > It turns out that we'll be able to keep using git commit positions (the
> > equivalent of svn revisions) at least for the short-term. So, we should be
> able
> > to keep this mostly the same as how it is today. Will just need to change
how
> we
> > get the revision numbers above. Sorry for the churn. I didn't realize that
> we'd
> > be able to use git commit positions till I talked to stip today.
> > 
> > Down the road, we should still change this code to be git hash friendly, but
> the
> > way I want to do that is to expose timestamps in the JSON from the server,
so
> > the logic here would stay the same still, it would just use timestamps
instead
> > of revision numbers.
> 
> No problem. Are you fine with this implementation then? It's not perfect, but
it
> works well for hashes and numbers.

I'd rather use the old implementation. It's considerably simpler since it only
needs to handle numbers. I don't think we'll ever try to make this work with
hashes since we'd just use timestamps if we wanted to move off commit positions.

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:159: return;
On 2014/08/24 23:42:11, Mathieu Perreault wrote:
> On 2014/08/23 01:23:05, ojan-only-code-yellow-reviews wrote:
> > We don't actually want to early return here. We'll soon have the default
mode
> in
> > sheriff-o-matic be to show all failures, even ones that don't have a tree.
Was
> > not early returning causing JS errors or something?
> 
> Nah, it just didn't seem clean to be using |tree| as undefined here. 

Yeah. Long-term, the right thing here is to grab the tree name out of the JSON
that we grab off the server and to have the server ensure that there's always a
valid tree name. The problem with that right now is that a) the server uses the
wrong tree names and b) doesn't ensure a valid tree name in the same cases we
get undefined here. Both of those are pretty easy to fix.

Anyways, I filed crbug.com/406969 for this.

[Mathieu, 2014-08-25 13:03:22]

Question

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-failure-analyzer.html (right):

https://codereview.chromium.org/498523002/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:50: // At this point
|a.revisions| and |b.revisions| are sorted lists of
On 2014/08/25 03:54:13, ojan-only-code-yellow-reviews wrote:
> On 2014/08/24 23:42:11, Mathieu Perreault wrote:
> > On 2014/08/23 01:23:06, ojan-only-code-yellow-reviews wrote:
> > > It turns out that we'll be able to keep using git commit positions (the
> > > equivalent of svn revisions) at least for the short-term. So, we should be
> > able
> > > to keep this mostly the same as how it is today. Will just need to change
> how
> > we
> > > get the revision numbers above. Sorry for the churn. I didn't realize that
> > we'd
> > > be able to use git commit positions till I talked to stip today.
> > > 
> > > Down the road, we should still change this code to be git hash friendly,
but
> > the
> > > way I want to do that is to expose timestamps in the JSON from the server,
> so
> > > the logic here would stay the same still, it would just use timestamps
> instead
> > > of revision numbers.
> > 
> > No problem. Are you fine with this implementation then? It's not perfect,
but
> it
> > works well for hashes and numbers.
> 
> I'd rather use the old implementation. It's considerably simpler since it only
> needs to handle numbers. I don't think we'll ever try to make this work with
> hashes since we'd just use timestamps if we wanted to move off commit
positions.

Much of my rewrite was because the old arguments were objects and now we have a
list e.g. ['chromium:1', 'blink:2']. Not use how to bring it back to the old
impl and still use |revisions|. Let's chat on what you mean later.

[ojan, 2014-08-25 18:09:41]

As I think about this more, I think the right end result for this code is to
have a CTFailureGroup hold on to a CTCommitList instead of vending a new one
from the commitList method. And CTFailureGroup should not hold on to the raw
list of revisions we get off the server. Instead CTCommitList should take the
list, pre-process it appropriately so it only holds on to a map of
tree->revisions, and then have accessors to get the right values, e.g.

// Returns the list of trees for which this CTCommitList has revisions.
CTCommitList.prototype.trees = function() {...}
CTCommitList.prototype.revisions = function(tree) {...}

This would require plumbing the commitlog through ct-failure-analyzer, but I
think that's OK.

This way we don't expose the tree:revision syntax to the rest of the codebase
and we simplify the rest of the code, e.g. the comparator could change back to
the old implementation, which is considerably simpler IMO.

It was a mistake for CTFailure to hold on to the raw data from the server as
well.

WDYT? I'm hesitant to put more roadblocks between you and landing this, So I'm
OK with you landing this patch without making this change if you'd like to. But
please fix the issues I list below if you're going to do that.

Sorry for not saying this earlier, it only just occurred to me as I was staring
at the failureComparator code.

https://codereview.chromium.org/498523002/diff/120001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-failure-analyzer.html (right):

https://codereview.chromium.org/498523002/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-analyzer.html:57: var rev =
revisions[revisions.length - 1 - i];
Nit: I'd find this easier to read if you started with i = revisions.length - 1
and just decremented i. It'd be more clear that this is walking backwards
through the array.

Also, what happens right now when i == revisions.length? I don't see a check for
that. I think you'd throw an exception...which the Promise would promptly
silently swallow. :( We're working on fixing the latter.

https://codereview.chromium.org/498523002/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-analyzer.html:58: while
(!rev.startsWith(tree)) {
Nit: Lets check startsWith(tree + ':') to avoid issues where one tree name is a
prefix of the other, e.g. I could easily imagine having v8 and v8-experimental
or something like that.
var treeWithColon = tree + ':';
while (!rev.startsWith(treeWithColon)) {

https://codereview.chromium.org/498523002/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-analyzer.html:81: if (rev_a == rev_b) {
Can we move this check right after line 65? I think it'd make the code easier to
follow since all the splitting and ids and what not isn't needed in this block.

https://codereview.chromium.org/498523002/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-analyzer.html:82: // In case of
tree-revision equality, try a fallback.
This comment is a little confusing. Would be more clear if it said what the
fallback was. Or just leave the comment out.

More importantly, I don't think the fallback you have is right. What if the last
revisions are from different trees?

https://codereview.chromium.org/498523002/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-analyzer.html:90: return b_id == a_id ? 0 :
(b_id > a_id) ? 1 : -1;
Can this just be the following?
return b_id - a_id;

[Mathieu, 2014-08-26 15:47:40]

Reworked, so you'll probably need to re-review. Please make sure the data
binding is how you like it. 

The sorting order was brought back to previous impl, and double-checked to be
the same as the running version, by visual comparison.

Have a look!

https://codereview.chromium.org/498523002/diff/120001/Tools/GardeningServer/u...
File Tools/GardeningServer/ui/ct-failure-analyzer.html (right):

https://codereview.chromium.org/498523002/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-analyzer.html:57: var rev =
revisions[revisions.length - 1 - i];
On 2014/08/25 18:09:40, ojan-only-code-yellow-reviews wrote:
> Nit: I'd find this easier to read if you started with i = revisions.length - 1
> and just decremented i. It'd be more clear that this is walking backwards
> through the array.
> 
> Also, what happens right now when i == revisions.length? I don't see a check
for
> that. I think you'd throw an exception...which the Promise would promptly
> silently swallow. :( We're working on fixing the latter.

Acknowledged.

https://codereview.chromium.org/498523002/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-analyzer.html:58: while
(!rev.startsWith(tree)) {
On 2014/08/25 18:09:40, ojan-only-code-yellow-reviews wrote:
> Nit: Lets check startsWith(tree + ':') to avoid issues where one tree name is
a
> prefix of the other, e.g. I could easily imagine having v8 and v8-experimental
> or something like that.
> var treeWithColon = tree + ':';
> while (!rev.startsWith(treeWithColon)) {

Acknowledged.

https://codereview.chromium.org/498523002/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-analyzer.html:81: if (rev_a == rev_b) {
On 2014/08/25 18:09:40, ojan-only-code-yellow-reviews wrote:
> Can we move this check right after line 65? I think it'd make the code easier
to
> follow since all the splitting and ids and what not isn't needed in this
block.

Acknowledged.

https://codereview.chromium.org/498523002/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-analyzer.html:82: // In case of
tree-revision equality, try a fallback.
On 2014/08/25 18:09:40, ojan-only-code-yellow-reviews wrote:
> This comment is a little confusing. Would be more clear if it said what the
> fallback was. Or just leave the comment out.
> 
> More importantly, I don't think the fallback you have is right. What if the
last
> revisions are from different trees?

Acknowledged.

https://codereview.chromium.org/498523002/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-analyzer.html:90: return b_id == a_id ? 0 :
(b_id > a_id) ? 1 : -1;
On 2014/08/25 18:09:41, ojan-only-code-yellow-reviews wrote:
> Can this just be the following?
> return b_id - a_id;

Acknowledged.

https://codereview.chromium.org/498523002/diff/120001/Tools/GardeningServer/u...
Tools/GardeningServer/ui/ct-failure-analyzer.html:90: return b_id == a_id ? 0 :
(b_id > a_id) ? 1 : -1;
On 2014/08/25 18:09:41, ojan-only-code-yellow-reviews wrote:
> Can this just be the following?
> return b_id - a_id;

Acknowledged.

[ojan, 2014-08-26 22:39:25]

lgtm

Yay! This is awesome. I really appreciate your work on this stuff.

https://codereview.chromium.org/498523002/diff/140001/Tools/GardeningServer/m...
File Tools/GardeningServer/model/ct-commit-list.html (right):

https://codereview.chromium.org/498523002/diff/140001/Tools/GardeningServer/m...
Tools/GardeningServer/model/ct-commit-list.html:55: this.revisions[repo].sort();
Why are these not already sorted since the thing we get from the server json is
sorted? I care about this because we will likely one day get git hashes and
sorting will no longer work.

[Mathieu, 2014-08-27 03:38:51]

Nasty merge! Submitting

https://codereview.chromium.org/498523002/diff/140001/Tools/GardeningServer/m...
File Tools/GardeningServer/model/ct-commit-list.html (right):

https://codereview.chromium.org/498523002/diff/140001/Tools/GardeningServer/m...
Tools/GardeningServer/model/ct-commit-list.html:55: this.revisions[repo].sort();
On 2014/08/26 22:39:25, ojan-only-code-yellow-reviews wrote:
> Why are these not already sorted since the thing we get from the server json
is
> sorted? I care about this because we will likely one day get git hashes and
> sorting will no longer work.

Done. You're right.

[Mathieu, 2014-08-27 03:39:09]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-08-27 03:39:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mathp@chromium.org/498523002/160001

[commit-bot: I haz the power, 2014-08-27 03:40:29]

Committed patchset #6 (160001) as 180933