[Webkit-unassigned] [Bug 152345] Teach dashboard code to compare non-integer revisions.

[bugzilla-daemon at webkit.org]

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

--- Comment #10 from Jason Marcell <jmarcell at apple.com> ---
Comment on attachment 267593
  --> https://bugs.webkit.org/attachment.cgi?id=267593
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".

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151218/687ced41/attachment.html>