Bug 152345 – Teach dashboard code to compare non-integer revisions.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jason Marcell]

Teach dashboard code to compare non-integer revisions.

[Jason Marcell]

Created attachment 267470 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 267470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267470&action=review

I didn't review this at all, just opened at a random place and couldn't help but post a couple comments.

It's great to have tests!

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/index.html:5
> +    <meta charset="UTF-8" />
> +    <meta http-equiv="content-type" content="text/html; charset=utf-8" />

The first line is sufficient. Also, please don't use XML auto-closing syntax in HTML, it's invalid there.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/index.html:9
> +    <link rel="stylesheet" href="https://code.jquery.com/qunit/qunit-1.20.0.css">

We try to make our tests not access external resources. There are already parts of jquery in External directory for the dashboard.

[Jason Marcell]

Created attachment 267492 [details]
Patch

[Jason Marcell]

(In reply to comment #2)
> Comment on attachment 267470 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267470&action=review
> 
> I didn't review this at all, just opened at a random place and couldn't help
> but post a couple comments.
> 
> It's great to have tests!
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/index.html:5
> > +    <meta charset="UTF-8" />
> > +    <meta http-equiv="content-type" content="text/html; charset=utf-8" />
> 
> The first line is sufficient. Also, please don't use XML auto-closing syntax
> in HTML, it's invalid there.

Thanks, I addressed these issues.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/index.html:9
> > +    <link rel="stylesheet" href="https://code.jquery.com/qunit/qunit-1.20.0.css">
> 
> We try to make our tests not access external resources. There are already
> parts of jquery in External directory for the dashboard.

Thanks for clarifying. I wasn't sure if I was supposed to add external code, but it sounds like we have a specific place just for doing that. I put the CSS file in there too. Should it go in External or Styles? Also, check-webkit-style did not like all of the tabs present in QUnit:

    ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/External/qunit-1.20.0.css:300:  Line contains tab character.  [whitespace/tab] [5]

Is that an issue?

[Jason Marcell]

Comment on attachment 267492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267492&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:34
> +

I'm going to remove these lines as I did not realize these settings are persistent. I have a better way of doing this anyway that does not involve touching the production classes but instead uses the mock classes.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:116
> +

I'm going to remove these lines as I did not realize these settings are persistent. I have a better way of doing this anyway that does not involve touching the production classes but instead uses the mock classes.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/QueueView.js:36
> +

I'm going to remove these lines as I did not realize these settings are persistent. I have a better way of doing this anyway that does not involve touching the production classes but instead uses the mock classes.

[Jason Marcell]

Created attachment 267593 [details]
Patch

[Dean Johnson]

Comment on attachment 267593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267593&action=review

Looks good overall. Few things, but otherwise nice work... I know a lot of people will be happy to see these changes merged. :)

Also, thanks for adding QUnit! It's awesome that we have a way to test our front-end now.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:301
>          var sortedRepositories = Dashboard.sortedRepositories;
>          for (var i = 0; i < sortedRepositories.length; ++i) {
>              var repositoryName = sortedRepositories[i].name;
> -            var result = b.revision[repositoryName] - a.revision[repositoryName];
> +            var trac = sortedRepositories[i].trac;
> +            if (!trac || b.revision[repositoryName] === undefined || a.revision[repositoryName] === undefined)
> +                return undefined;
> +            var result = trac.compareRevisions(b.revision[repositoryName], a.revision[repositoryName]);
>              if (result)
>                  return result;
>          }

Could compareIterations and compareIterationsByRevisions share this piece of code? It looks identical.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:272
> +    compareRevisions: function(a, b)
> +    {
> +        var indexA = this.recordedCommits.objectIndexOf(a, "revisionNumber");
> +        var indexB = this.recordedCommits.objectIndexOf(b, "revisionNumber");
> +        if (indexA === undefined || indexB === undefined) {
> +            this.loadMoreHistoricalData();
> +            return undefined;
> +        }
> +        return indexA - indexB;
> +    },

This seems to make the assumption that a > b. Is it ever possible that b > a? If so, maybe use indexA as max(IndexA, indexB) and indexB as min(indexA, indexB).

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Utilities.js:170
> +{

I think this may make more sense as something like: indexOfObjectWithProperty. I don't like how verbose that is necessarily though, but it does seem a bit confusing without reading the function code. Ideas?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Utilities.js:172
> +        if (this[i][property] === searchTerm) return i;

Nit: I think this would look a little nicer on two lines.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/MockOverrides.js:5
> +        if (x) {

What is x? Could we use a better variable?

[Daniel Bates]

Comment on attachment 267593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267593&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:276
> -            var result = b.revision[repositoryName] - a.revision[repositoryName];
> +            var trac = sortedRepositories[i].trac;
> +            if (!trac || b.revision[repositoryName] === undefined || a.revision[repositoryName] === undefined)
> +                return undefined;

Can you give an example where we would not have a revision for a repository?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:298
> +            var trac = sortedRepositories[i].trac;
> +            if (!trac || b.revision[repositoryName] === undefined || a.revision[repositoryName] === undefined)
> +                return undefined;
> +            var result = trac.compareRevisions(b.revision[repositoryName], a.revision[repositoryName]);

Ditto.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:95
> +            if (!trac.latestRecordedRevisionNumber || comparison === undefined || comparison > 0) {

Can you give an example when comparison === undefined?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:100
> +            totalRevisionsBehind += trac.commitsOnBranch(branch.name, function(commit, index, arr) { return index > arr.objectIndexOf(latestProductiveRevisionNumber, "revisionNumber"); }).length;

The name arr is not a good name for the array of commits. I am assuming arr is short for "array". Regardless, neither "arr" nor "array" are good names because the former is an abbreviation of the latter (and I do not find it more canonical than the word "array") and the latter is a generic term that does not convey anything about the contents of the array. Maybe a better name would be commits or allCommits?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:145
> +        var commits = trac.commitsOnBranch(branch.name, function(commit, index, arr) { return index >= arr.objectIndexOf(firstRevisionNumber, "revisionNumber") && index <= arr.objectIndexOf(lastRevisionNumber, "revisionNumber"); });

Ditto.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:68
> +        return this.recordedCommits.filter(function(commit, index, arr) {

Ditto.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:266
> +        var indexA = this.recordedCommits.objectIndexOf(a, "revisionNumber");
> +        var indexB = this.recordedCommits.objectIndexOf(b, "revisionNumber");

Can you elaborate on when a commit would not have a revision?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:269
> +            this.loadMoreHistoricalData();
> +            return undefined;

This does not seem like the correct approach. Having a comparison function fetch more data and have a polymorphic return type is weird.

[Jason Marcell]

(In reply to comment #7)
> Comment on attachment 267593 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267593&action=review
> 
> Looks good overall. Few things, but otherwise nice work... I know a lot of
> people will be happy to see these changes merged. :)
> 
> Also, thanks for adding QUnit! It's awesome that we have a way to test our
> front-end now.
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:301
> >          var sortedRepositories = Dashboard.sortedRepositories;
> >          for (var i = 0; i < sortedRepositories.length; ++i) {
> >              var repositoryName = sortedRepositories[i].name;
> > -            var result = b.revision[repositoryName] - a.revision[repositoryName];
> > +            var trac = sortedRepositories[i].trac;
> > +            if (!trac || b.revision[repositoryName] === undefined || a.revision[repositoryName] === undefined)
> > +                return undefined;
> > +            var result = trac.compareRevisions(b.revision[repositoryName], a.revision[repositoryName]);
> >              if (result)
> >                  return result;
> >          }
> 
> Could compareIterations and compareIterationsByRevisions share this piece of
> code? It looks identical.

Yes, both compareIterations and compareIterationsByRevisions are very similar. The latter was introduced here:
https://bugs.webkit.org/show_bug.cgi?id=143885

We should probably find a way to collapse these into a single function if possible so that we are not duplicating code.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:272
> > +    compareRevisions: function(a, b)
> > +    {
> > +        var indexA = this.recordedCommits.objectIndexOf(a, "revisionNumber");
> > +        var indexB = this.recordedCommits.objectIndexOf(b, "revisionNumber");
> > +        if (indexA === undefined || indexB === undefined) {
> > +            this.loadMoreHistoricalData();
> > +            return undefined;
> > +        }
> > +        return indexA - indexB;
> > +    },
> 
> This seems to make the assumption that a > b. Is it ever possible that b >
> a? If so, maybe use indexA as max(IndexA, indexB) and indexB as min(indexA,
> indexB).

This does not make the assumption that a > b.

This is a pretty common pattern used for things like sort functions. The expected API is as follows: negative return value if the first parameter is less than the second, zero if they’re equal, a positive return value otherwise.

See, for examples of this pattern, the following:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort 

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Utilities.js:170
> > +{
> 
> I think this may make more sense as something like:
> indexOfObjectWithProperty. I don't like how verbose that is necessarily
> though, but it does seem a bit confusing without reading the function code.
> Ideas?

That doesn't seem like a bad name, but I'm open to other suggestions. I'll use that if there are no better suggestions.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Utilities.js:172
> > +        if (this[i][property] === searchTerm) return i;
> 
> Nit: I think this would look a little nicer on two lines.

I'll add a line break.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/MockOverrides.js:5
> > +        if (x) {
> 
> What is x? Could we use a better variable?

x is actually the value that the user passes to the setter. Should we use 'value' as the variable name instead?

Some background: My intention for the MockOverrides.js file was to provide a place where we would override class methods (rather than creating a derived Mock subclass). The idea is that you want to cherry-pick some functionality in a particular method of some class while leaving some other functionality out. To that end, I copied this code from the original setter (bad variable name and all) while leaving out some code that getting in the way of my testing. I know copy/paste code is usually a bad idea, but it's going to help with testing :/

And if you're wondering why I overrode the method in the original class instead of just creating a Mock subclass that overrides the method, this is a class that I'm not directly instantiating myself, so I'm not in a position to provide a Mock class as a substitute.

[Jason Marcell]

Comment on attachment 267593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267593&action=review

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:276
>> +                return undefined;
> 
> Can you give an example where we would not have a revision for a repository?

Ideally we shouldn't, but I believe I have seen instances where the defaultBranches() method of a subclass of Buildbot provided branches for a queue/iteration that were not involved in the build. I can look into this further.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:95
>> +            if (!trac.latestRecordedRevisionNumber || comparison === undefined || comparison > 0) {
> 
> Can you give an example when comparison === undefined?

When the dashboard code first starts up, the Trac instances pull some minimum amount of historical data. The revision(s) in question aren't necessarily present in the "recordCommits" array at first. If they are not in the "recordedCommits" array yet, then we have no way of reasoning about the comparison of the two commits in question. What, then, should I return? I chose to return "undefined".

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:100
>> +            totalRevisionsBehind += trac.commitsOnBranch(branch.name, function(commit, index, arr) { return index > arr.objectIndexOf(latestProductiveRevisionNumber, "revisionNumber"); }).length;
> 
> The name arr is not a good name for the array of commits. I am assuming arr is short for "array". Regardless, neither "arr" nor "array" are good names because the former is an abbreviation of the latter (and I do not find it more canonical than the word "array") and the latter is a generic term that does not convey anything about the contents of the array. Maybe a better name would be commits or allCommits?

It's actually comparing the individual "commit" to the Trac's internal "recordedCommits" array. I'll use the name "recordedCommits" here to remain consistent.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:266
>> +        var indexB = this.recordedCommits.objectIndexOf(b, "revisionNumber");
> 
> Can you elaborate on when a commit would not have a revision?

The Trac class loads commits incrementally. At first, the revision in question may not be in the recordedCommits array. If that's the case we "loadMoreHistoricalData" and it will be there the next time.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:269
>> +            return undefined;
> 
> This does not seem like the correct approach. Having a comparison function fetch more data and have a polymorphic return type is weird.

I'm open to changing this. I agree that side effects in functions should generally be avoided. Perhaps we can shift the loading of more data out to the call site of this function.

I might disagree with you, however, on the decision to return "undefined". We need a way to convey that we have an invalid comparison. When the dashboard code first starts up, the Trac instances pull some minimum amount of historical data. The revision(s) in question aren't necessarily present in the "recordCommits" array at first. If they are not in the "recordedCommits" array yet, then we have no way of reasoning about the comparison of the two commits in question. What, then, should I return? I chose to return "undefined".

[Jason Marcell]

Comment on attachment 267593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267593&action=review

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:276
>> +                return undefined;
> 
> Can you give an example where we would not have a revision for a repository?

Ideally we shouldn't, but I believe I have seen instances where the defaultBranches() method of a subclass of Buildbot provided branches for a queue/iteration that were not involved in the build. I can look into this further.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:95
>> +            if (!trac.latestRecordedRevisionNumber || comparison === undefined || comparison > 0) {
> 
> Can you give an example when comparison === undefined?

When the dashboard code first starts up, the Trac instances pull some minimum amount of historical data. The revision(s) in question aren't necessarily present in the "recordCommits" array at first. If they are not in the "recordedCommits" array yet, then we have no way of reasoning about the comparison of the two commits in question. What, then, should I return? I chose to return "undefined".

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:100
>> +            totalRevisionsBehind += trac.commitsOnBranch(branch.name, function(commit, index, arr) { return index > arr.objectIndexOf(latestProductiveRevisionNumber, "revisionNumber"); }).length;
> 
> The name arr is not a good name for the array of commits. I am assuming arr is short for "array". Regardless, neither "arr" nor "array" are good names because the former is an abbreviation of the latter (and I do not find it more canonical than the word "array") and the latter is a generic term that does not convey anything about the contents of the array. Maybe a better name would be commits or allCommits?

It's actually comparing the individual "commit" to the Trac's internal "recordedCommits" array. I'll use the name "recordedCommits" here to remain consistent.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:266
>> +        var indexB = this.recordedCommits.objectIndexOf(b, "revisionNumber");
> 
> Can you elaborate on when a commit would not have a revision?

The Trac class loads commits incrementally. At first, the revision in question may not be in the recordedCommits array. If that's the case we "loadMoreHistoricalData" and it will be there the next time.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:269
>> +            return undefined;
> 
> This does not seem like the correct approach. Having a comparison function fetch more data and have a polymorphic return type is weird.

I'm open to changing this. I agree that side effects in functions should generally be avoided. Perhaps we can shift the loading of more data out to the call site of this function.

I might disagree with you, however, on the decision to return "undefined". We need a way to convey that we have an invalid comparison. When the dashboard code first starts up, the Trac instances pull some minimum amount of historical data. The revision(s) in question aren't necessarily present in the "recordCommits" array at first. If they are not in the "recordedCommits" array yet, then we have no way of reasoning about the comparison of the two commits in question. What, then, should I return? I chose to return "undefined".

[Jason Marcell]

Created attachment 268424 [details]
1. Adding QUnit for unit testing.

[Jason Marcell]

Created attachment 268425 [details]
2. Adding a unit test to test BuildbotQueueView._appendPendingRevisionCount.

[Jason Marcell]

I want to continue working on the feedback that I've received from my previous PFR, but I don't want that to hold up landing these other patches. We should be able to land QUnit as its own patch and the unit tests as their own patch as well.

[Jason Marcell]

Created attachment 268503 [details]
3. Refactor compareIterations to remove duplicate code.

[Jason Marcell]

(In reply to comment #9)
> (In reply to comment #7)
> > Comment on attachment 267593 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=267593&action=review
> > 
> > Looks good overall. Few things, but otherwise nice work... I know a lot of
> > people will be happy to see these changes merged. :)
> > 
> > Also, thanks for adding QUnit! It's awesome that we have a way to test our
> > front-end now.
> > 
> > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:301
> > >          var sortedRepositories = Dashboard.sortedRepositories;
> > >          for (var i = 0; i < sortedRepositories.length; ++i) {
> > >              var repositoryName = sortedRepositories[i].name;
> > > -            var result = b.revision[repositoryName] - a.revision[repositoryName];
> > > +            var trac = sortedRepositories[i].trac;
> > > +            if (!trac || b.revision[repositoryName] === undefined || a.revision[repositoryName] === undefined)
> > > +                return undefined;
> > > +            var result = trac.compareRevisions(b.revision[repositoryName], a.revision[repositoryName]);
> > >              if (result)
> > >                  return result;
> > >          }
> > 
> > Could compareIterations and compareIterationsByRevisions share this piece of
> > code? It looks identical.
> 
> Yes, both compareIterations and compareIterationsByRevisions are very
> similar. The latter was introduced here:
> https://bugs.webkit.org/show_bug.cgi?id=143885
> 
> We should probably find a way to collapse these into a single function if
> possible so that we are not duplicating code.

I have addressed this in its own patch. Please see the following:
https://bugs.webkit.org/attachment.cgi?id=268503&action=review

[Daniel Bates]

Comment on attachment 268424 [details]
1. Adding QUnit for unit testing.

Please file a new bug to add QUnit that blocks this bug (bug #152345) unless you plan to address the original purpose of this bug as described in comment #0 in the short term (at most a week).

It is preferred to break a patch into dependent bugs instead of separating a patch into sub-patches associated with the same bug because it makes it keeps each bug focused on exactly change. Among other benefits, using the one patch per bug strategy allows for having a dedicated forum for talking about the patch that may be otherwise hard to follow (or considered noise to some people) when interleaved in a bigger conversation about a bug that has multiple patches. In the rare cases that its reasonable to have one bug with multiple patches (parts) and each part lands as they are written then we tend to format the ChangeLog entry with the same bug title and bug URL for each of these parts and mention in the description portion of the ChangeLog entry for each patch that it represents a part of the bug fix. One such example of this can be seen in the ChangeLog entries in the patches on bug #136516 that landed in <http://trac.webkit.org/changeset/173260> and <http://trac.webkit.org/changeset/173332>, respectively.

[Daniel Bates]

Comment on attachment 268425 [details]
2. Adding a unit test to test BuildbotQueueView._appendPendingRevisionCount.

Similar to my remarks in comment #17, I suggest that we either file a separate bug for this patch or combine this patch with the patch for adding QUnit (attachment #268424 [details]) and file a single bug for both. The benefit of the latter approach is that we directly convey the purpose of adding the QUnit framework as the means for supporting the effort to add a unit test.

[Jason Marcell]

I will combine the two patches and file a new bug with them.

[Daniel Bates]

Comment on attachment 268503 [details]
3. Refactor compareIterations to remove duplicate code.

I can see how this patch could be tangentially thought of as "part" of the bug fix assuming you fix up the ChangeLog entry as described in my remark in comment #17. You may want to consider filing a new bug for this part if you do not plan to address the original bug (comment #0) in a timely manner.

[Jason Marcell]

Right, I only addressed it here because multiple people have commented on this section of code in the course of reviewing my original patch. I will do the same as with the QUnit patch: I will file a separate bug with a separate patch.

[Jason Marcell]

Comment on attachment 268424 [details]
1. Adding QUnit for unit testing.

See: https://bugs.webkit.org/show_bug.cgi?id=152910

[Jason Marcell]

Comment on attachment 268425 [details]
2. Adding a unit test to test BuildbotQueueView._appendPendingRevisionCount.

See: https://bugs.webkit.org/show_bug.cgi?id=152910

[Jason Marcell]

Comment on attachment 268503 [details]
3. Refactor compareIterations to remove duplicate code.

See: https://bugs.webkit.org/show_bug.cgi?id=152913

[Jason Marcell]

Created attachment 270457 [details]
Patch

[Daniel Bates]

Comment on attachment 270457 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270457&action=review

> Tools/ChangeLog:20
> +        (Trac.prototype.commitsOnBranchLaterThanRevision): Finds revisions on a branch great later than the specified revision.

Nit: This sentence does not read well.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:290
> +            indexA = trac.indexOfRevision(a.revision[repositoryName]);

This will define a global variable named indexA. We should make this a local variable as you did for variable trac.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:291
> +            indexB = trac.indexOfRevision(b.revision[repositoryName]);

Ditto.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:73
> +    _commitsOnBranch: function(branchName, filter)
>      {
> +        if (!filter)
> +            filter = function(commit) { return true; };
>          return this.recordedCommits.filter(function(commit, index, array) {
>              return (!commit.containsBranchLocation || commit.branches.includes(branchName)) && filter(commit, index, array);
>          });
>      },

We always pass a filter to this function and that filter only looks at the index position. So, I suggest we remove the filter argument and have this function take optional begin and end index positions:

_commitsOnBranch(branchName, beginPosition, endPosition)
{
    beginPosition = beginPosition || 0;
    endPosition = endPosition || this.recordedCommits.length - 1;
    let commits = [];
    for (let i = beginPosition; i <= endPosition; ++i) {
        let commit = this.recordedCommits[i]
        if (!commit.containsBranchLocation || commit.branches.includes(branchName))
            commits.push(commit);
    }
    return commits;
},

This will allow us to remove the need to iterate over the entire array of recorded commits once we know either a begin or end position (or both).

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:82
> +        var indexToBeLaterThan = this.indexOfRevision(revision);
> +        function selectCommitsLaterThanRevision(commit, index, allCommits) {
> +            return index > indexToBeLaterThan;
> +        }
> +        var commits = this._commitsOnBranch(branchName, selectCommitsLaterThanRevision.bind(this));
> +        return commits;

Then we can implement this as:

let indexToBeLaterThan = this.indexOfRevision(revision);
console.assert(indexToBeLaterThan !== -1, revision + " is not in the list of recorded commits");
if (indexToBeLaterThan === -1)
    return [];
return this._commitsOnBranch(branchName, null, indexToBeLaterThan + 1);

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:93
> +        var indexOfFirstRevision = this.indexOfRevision(firstRevision);
> +        var indexOfLastRevision = this.indexOfRevision(lastRevision);
> +        function selectCommitsInRange(commit, index, allCommits) {
> +            return index >= indexOfFirstRevision && index <= indexOfLastRevision;
> +        }
> +        var commits = this._commitsOnBranch(branchName, selectCommitsInRange.bind(this));
> +        return commits;

And we can implement this as:

let indexOfFirstRevision = this.indexOfRevision(firstRevision);
console.assert(indexOfFirstRevision !== -1, firstRevision + " is not in the list of recorded commits");
if (indexOfFirstRevision === -1)
    return [];
let indexOfLastRevision = this.indexOfRevision(lastRevision);
console.assert(indexOfLastRevision !== -1, indexOfLastRevision + " is not in the list of recorded commits");
if (indexOfLastRevision === -1)
    return [];
return this._commitOnBranch(branchName, null, indexOfFirstRevision, indexOfLastRevision);

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Utilities.js:177
> +Array.prototype.indexOfObjectPassingTest = function(predicate)
> +{
> +    for (var i = 0; i < this.length; ++i) {
> +        if (predicate(this[i]))
> +            return i;
> +    }
> +
> +    return -1;
> +};

This function is only used by Trac.indexOfRevision(). Unless you plan to make use of this function in a near future patch, I suggest that we inline the implementation of this function into Trac.indexOfRevision() and remove this function.

[Daniel Bates]

(In reply to comment #26)
> > [...]
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:82
> > +        var indexToBeLaterThan = this.indexOfRevision(revision);
> > +        function selectCommitsLaterThanRevision(commit, index, allCommits) {
> > +            return index > indexToBeLaterThan;
> > +        }
> > +        var commits = this._commitsOnBranch(branchName, selectCommitsLaterThanRevision.bind(this));
> > +        return commits;
> 
> Then we can implement this as:
> 
> [...]
> return this._commitsOnBranch(branchName, null, indexToBeLaterThan + 1);

err, I meant to write:

return this._commitsOnBranch(branchName, indexToBeLaterThan + 1);

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:93
> > +        var indexOfFirstRevision = this.indexOfRevision(firstRevision);
> > +        var indexOfLastRevision = this.indexOfRevision(lastRevision);
> > +        function selectCommitsInRange(commit, index, allCommits) {
> > +            return index >= indexOfFirstRevision && index <= indexOfLastRevision;
> > +        }
> > +        var commits = this._commitsOnBranch(branchName, selectCommitsInRange.bind(this));
> > +        return commits;
> 
> And we can implement this as:
> 
> [...]
> return this._commitOnBranch(branchName, null, indexOfFirstRevision,
> indexOfLastRevision);
> 

err, I meant to write:

return this._commitsOnBranch(branchName, indexOfFirstRevision, indexOfLastRevision);

[Daniel Bates]

Comment on attachment 270457 [details]
Patch

I would like to see another iteration of this patch.

[Jason Marcell]

(In reply to comment #26)
> Comment on attachment 270457 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270457&action=review
> 
> > Tools/ChangeLog:20
> > +        (Trac.prototype.commitsOnBranchLaterThanRevision): Finds revisions on a branch great later than the specified revision.
> 
> Nit: This sentence does not read well.
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:290
> > +            indexA = trac.indexOfRevision(a.revision[repositoryName]);
> 
> This will define a global variable named indexA. We should make this a local
> variable as you did for variable trac.
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:291
> > +            indexB = trac.indexOfRevision(b.revision[repositoryName]);
> 
> Ditto.
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:73
> > +    _commitsOnBranch: function(branchName, filter)
> >      {
> > +        if (!filter)
> > +            filter = function(commit) { return true; };
> >          return this.recordedCommits.filter(function(commit, index, array) {
> >              return (!commit.containsBranchLocation || commit.branches.includes(branchName)) && filter(commit, index, array);
> >          });
> >      },
> 
> We always pass a filter to this function and that filter only looks at the
> index position. So, I suggest we remove the filter argument and have this
> function take optional begin and end index positions:
> 
> _commitsOnBranch(branchName, beginPosition, endPosition)
> {
>     beginPosition = beginPosition || 0;
>     endPosition = endPosition || this.recordedCommits.length - 1;

What if endPosition is 0? Maybe we should instead do something like:

if (endPosition === undefined)
    endPosition = this.recordedCommits.length - 1;

>     let commits = [];

Why are you using 'let' instead of 'var'?

>     for (let i = beginPosition; i <= endPosition; ++i) {
>         let commit = this.recordedCommits[i]
>         if (!commit.containsBranchLocation ||
> commit.branches.includes(branchName))
>             commits.push(commit);
>     }
>     return commits;
> },
> 
> This will allow us to remove the need to iterate over the entire array of
> recorded commits once we know either a begin or end position (or both).
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:82
> > +        var indexToBeLaterThan = this.indexOfRevision(revision);
> > +        function selectCommitsLaterThanRevision(commit, index, allCommits) {
> > +            return index > indexToBeLaterThan;
> > +        }
> > +        var commits = this._commitsOnBranch(branchName, selectCommitsLaterThanRevision.bind(this));
> > +        return commits;
> 
> Then we can implement this as:
> 
> let indexToBeLaterThan = this.indexOfRevision(revision);
> console.assert(indexToBeLaterThan !== -1, revision + " is not in the list of
> recorded commits");
> if (indexToBeLaterThan === -1)
>     return [];
> return this._commitsOnBranch(branchName, null, indexToBeLaterThan + 1);
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:93
> > +        var indexOfFirstRevision = this.indexOfRevision(firstRevision);
> > +        var indexOfLastRevision = this.indexOfRevision(lastRevision);
> > +        function selectCommitsInRange(commit, index, allCommits) {
> > +            return index >= indexOfFirstRevision && index <= indexOfLastRevision;
> > +        }
> > +        var commits = this._commitsOnBranch(branchName, selectCommitsInRange.bind(this));
> > +        return commits;
> 
> And we can implement this as:
> 
> let indexOfFirstRevision = this.indexOfRevision(firstRevision);
> console.assert(indexOfFirstRevision !== -1, firstRevision + " is not in the
> list of recorded commits");
> if (indexOfFirstRevision === -1)
>     return [];
> let indexOfLastRevision = this.indexOfRevision(lastRevision);
> console.assert(indexOfLastRevision !== -1, indexOfLastRevision + " is not in
> the list of recorded commits");
> if (indexOfLastRevision === -1)
>     return [];
> return this._commitOnBranch(branchName, null, indexOfFirstRevision,
> indexOfLastRevision);
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Utilities.js:177
> > +Array.prototype.indexOfObjectPassingTest = function(predicate)
> > +{
> > +    for (var i = 0; i < this.length; ++i) {
> > +        if (predicate(this[i]))
> > +            return i;
> > +    }
> > +
> > +    return -1;
> > +};
> 
> This function is only used by Trac.indexOfRevision(). Unless you plan to
> make use of this function in a near future patch, I suggest that we inline
> the implementation of this function into Trac.indexOfRevision() and remove
> this function.

[Jason Marcell]

Created attachment 270620 [details]
Patch

[Jason Marcell]

Created attachment 270705 [details]
Patch

[Jason Marcell]

Comment on attachment 270705 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270705&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:171
> +            if (!latestProductiveRevisionNumber || !trac.latestRecordedRevisionNumber || !nextRevision)

trac.nextRevision can potentially return undefined if there isn't a nextRevision. We need to check for that and continue if that's the case.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:304
> +        return undefined;

I needed to return undefined here because returning the same commit if no later commits were found is technically inaccurate and was causing a bug. Also, since I am potentially returning undefined, I needed to add a check at the call site for undefined.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:151
> +{

This is a new unit test to check for the case where there are no pending commits.

I'm doing a lot of setup here to explicitly set the state whereas a lot of the other tests in this module rely on the module's setup function to set the state up. As Dan and I have discussed, it's probably better to favor this style where we explicitly set the state for each unit tests. I think it would be a good idea to submit a new patch, after landing this one, that makes the other tests in this module match this more explicit style.

[Jason Marcell]

Created attachment 270708 [details]
Patch

[Daniel Bates]

Comment on attachment 270708 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270708&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:292
> +            if (indexA > -1 && indexB > -1) {

This is OK as-is. I suggest explicitly comparing against -1.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:172
> +            var nextRevision = trac.nextRevision(branch.name, latestProductiveRevisionNumber);
> +            if (!latestProductiveRevisionNumber || !trac.latestRecordedRevisionNumber || !nextRevision)
>                  continue;

This is very minor. I suggest we compute nextRevision and early return (if necessary) after line 171.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:237
>              if (context.firstRevision <= context.lastRevision)

This is incorrect for Git SHAs, which are not numeric.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:96
> +        console.assert(indexOfLastRevision !== -1, indexOfLastRevision + " is not in the list of recorded commits");

indexOfLastRevision => lastRevision

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:304
> +        return undefined;

I suggest either returning null or we may want to define a named constant (it could equal null), maybe Trac.NO_MORE_REVISIONS, if we feel this may improve the readability at the call site.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:310
> +        for (var i = 0; i < commits.length; ++i)

Nit: Missing braces. We add braces whenever the body of the control-flow statement is more than one line.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:167
> +        return {
> +            revision: {
> +                "openSource": 33021
> +            }
> +        };

This is OK as-is. I would have written this as:

var iteration = {
    revision: { "openSource": 33021 },
};
return iteration;

[Daniel Bates]

Comment on attachment 270708 [details]
Patch

r- for the correctness issue in BuildbotQueueView.prototype._revisionContentWithPopoverForIteration().

[Jason Marcell]

Created attachment 270813 [details]
Patch

[Jason Marcell]

(In reply to comment #34)
> Comment on attachment 270708 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270708&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:292
> > +            if (indexA > -1 && indexB > -1) {
> 
> This is OK as-is. I suggest explicitly comparing against -1.

Done.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:172
> > +            var nextRevision = trac.nextRevision(branch.name, latestProductiveRevisionNumber);
> > +            if (!latestProductiveRevisionNumber || !trac.latestRecordedRevisionNumber || !nextRevision)
> >                  continue;
> 
> This is very minor. I suggest we compute nextRevision and early return (if
> necessary) after line 171.

Done.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:237
> >              if (context.firstRevision <= context.lastRevision)
> 
> This is incorrect for Git SHAs, which are not numeric.

I addressed this.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:96
> > +        console.assert(indexOfLastRevision !== -1, indexOfLastRevision + " is not in the list of recorded commits");
> 
> indexOfLastRevision => lastRevision
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:304
> > +        return undefined;
> 
> I suggest either returning null or we may want to define a named constant
> (it could equal null), maybe Trac.NO_MORE_REVISIONS, if we feel this may
> improve the readability at the call site.

I went with Tac.NO_MORE_REVISIONS and defined it as 'null'.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:310
> > +        for (var i = 0; i < commits.length; ++i)
> 
> Nit: Missing braces. We add braces whenever the body of the control-flow
> statement is more than one line.

Fixed.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:167
> > +        return {
> > +            revision: {
> > +                "openSource": 33021
> > +            }
> > +        };
> 
> This is OK as-is. I would have written this as:
> 
> var iteration = {
>     revision: { "openSource": 33021 },
> };
> return iteration;

Fixed.

[Daniel Bates]

Comment on attachment 270813 [details]
Patch

r=me

[Jason Marcell]

Dan, thank you for all of your help and for your review!

Landed: http://trac.webkit.org/changeset/196402