Bug 153467 – Sort incoming commits via date instead of revision number

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

[Jason Marcell]

Sort incoming commits via date instead of revision number

[Jason Marcell]

Created attachment 269817 [details]
Patch

[Dean Johnson]

Comment on attachment 269817 [details]
Patch

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

r=me w/ passing tests! :)

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:200
> +            this.recordedCommits = newCommits.concat(this.recordedCommits).sort(function(a, b) { return a.date - b.date; });

I'm assuming this is the case, but just to be sure: these dates are either stored as integers or are data types that have logic to handle arithmetic operations and give correct results?

Actually, tests for this would completely eliminate my concern :)

[Jason Marcell]

Created attachment 269946 [details]
Patch

[Jason Marcell]

(In reply to comment #2)
> Comment on attachment 269817 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269817&action=review
> 
> r=me w/ passing tests! :)
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:200
> > +            this.recordedCommits = newCommits.concat(this.recordedCommits).sort(function(a, b) { return a.date - b.date; });
> 
> I'm assuming this is the case, but just to be sure: these dates are either
> stored as integers or are data types that have logic to handle arithmetic
> operations and give correct results?

Yes, that is the case.

> 
> Actually, tests for this would completely eliminate my concern :)

I've added tests.

[Dean Johnson]

Comment on attachment 269946 [details]
Patch

Looks good to me!

[Jason Marcell]

Created attachment 269953 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 269953 [details]
Patch

I don't think that it's right for the tests to live inside public_html, why did we put them there?

The patch doesn't apply, but it looks fine to me.

[Jason Marcell]

(In reply to comment #7)
> Comment on attachment 269953 [details]
> Patch
> 
> I don't think that it's right for the tests to live inside public_html, why
> did we put them there?

I'm definitely open to changing the way these unit tests are organized. I welcome feedback on a better organizational layout. If we come up with something better I can address it in a future bug/patch.

> 
> The patch doesn't apply, but it looks fine to me.

I can rebase the patch so that it applies cleanly.

[Jason Marcell]

Created attachment 269957 [details]
Patch

[Jason Marcell]

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