TBPL not sorting pushlog JSON output

VERIFIED FIXED

Status

Tree Management Graveyard
TBPL
--
blocker
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: rhelmer, Assigned: rhelmer)

Tracking

({regression})

Trunk
regression

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

8 years ago
Created attachment 467616 [details]
pushlog JSON

Since TBPL bug 584836 landed and added native JSON parsing for pushlog, the order of pushes is sometimes wrong (example JSON exhibiting the problem attached).

Looks ok in Chrome/Firefox 3.6 but not latest Minefield, sicking suspects a possible JSON parser bug.
(Assignee)

Comment 1

8 years ago
Created attachment 467622 [details]
minimal json for testcase
(Assignee)

Comment 2

8 years ago
Created attachment 467623 [details]
testcase
(Assignee)

Comment 3

8 years ago
(In reply to comment #0)
> Looks ok in Chrome/Firefox 3.6 but not latest Minefield, sicking suspects a
> possible JSON parser bug.

This is wrong; it's easier to see in the testcase but Minefield (m-c) and Firefox 3.6 behave the same. Chrome's JSON parser is sorting the keys, and TBPL is apparently depending on this behavior.
Component: DOM → Tinderboxpushlog
Product: Core → Webtools
QA Contact: general → tinderboxpushlog
Summary: wrong order of attributes in native JSON parser → TBPL not sorting pushlog JSON output
Version: unspecified → Trunk
(Assignee)

Comment 4

8 years ago
Created attachment 467635 [details] [diff] [review]
pushlog JSON is not sorted by key; sort in JS

Not sure if this should be fixed on the pushlog side, but here is a script to do it in JS.

Anyone know a prettier way to make sure the sort happens numerically?
Attachment #467635 - Flags: review?(arpad.borsos)
(Assignee)

Updated

8 years ago
Attachment #467635 - Flags: review?(mstange)
(Assignee)

Updated

8 years ago
Attachment #467622 - Attachment mime type: application/octet-stream → application/javascript
(Assignee)

Updated

8 years ago
Attachment #467622 - Attachment mime type: application/javascript → text/plain
(Assignee)

Comment 5

8 years ago
Created attachment 467638 [details]
testcase
Attachment #467623 - Attachment is obsolete: true
(Assignee)

Comment 6

8 years ago
(In reply to comment #3)
> (In reply to comment #0)
> > Looks ok in Chrome/Firefox 3.6 but not latest Minefield, sicking suspects a
> > possible JSON parser bug.
> 
> This is wrong; it's easier to see in the testcase but Minefield (m-c) and
> Firefox 3.6 behave the same. Chrome's JSON parser is sorting the keys, and TBPL
> is apparently depending on this behavior.

Just to clarify, the behavior I'm seeing in Chrome is likely a bug in Chrome; that's what the testcase attachments are about. Firefox's JSON parser looks like it's doing the right thing.

This obscured the problem at first, so it looked like a Firefox and not a TBPL and/or pushlog JSON bug.
(Assignee)

Comment 7

8 years ago
(In reply to comment #6)
> Just to clarify, the behavior I'm seeing in Chrome is likely a bug in Chrome;
> that's what the testcase attachments are about. Firefox's JSON parser looks
> like it's doing the right thing.

There are several bugs in the chromium tracker for this, here is one with recent activity:
http://code.google.com/p/v8/issues/detail?id=164
Comment on attachment 467635 [details] [diff] [review]
pushlog JSON is not sorted by key; sort in JS

>diff -r c2a561d9a397 js/PushlogJSONParser.js
>--- a/js/PushlogJSONParser.js	Thu Aug 19 15:51:36 2010 +0200
>+++ b/js/PushlogJSONParser.js	Thu Aug 19 19:17:09 2010 -0700
>@@ -4,8 +4,12 @@
>     var self = this;
>     $.getJSON(this._getLogUrl(repoName, timeOffset), function(data) {
>       var pushes = [];
>-      for (var i in data) {
>-        var push = data[i];
>+      var sorted = [];
>+      for (k in data) sorted.push(k);
>+      sorted.sort(function(a,b){ return a-b });
>+      for (var i = 0; i < sorted.length; i++) {
>+        var key = sorted[i];
>+        var push = data[key];
>         var patches = [];
>         for (var i in push.changesets) {
>           var patch = push.changesets[i];

Huh, we were using "i" as loop variable both in the outer and in the inner loop? Why did that work?
I've pushed your patch with some changes:
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/fc73f6ae02b7

(In reply to comment #4)
> Anyone know a prettier way to make sure the sort happens numerically?

Ensure that the items in the array are numbers. You can convert them by prefixing them with a plus.
Attachment #467635 - Flags: review?(mstange)
Attachment #467635 - Flags: review?(arpad.borsos)
Attachment #467635 - Flags: review+
Assignee: nobody → robert
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Status: RESOLVED → VERIFIED
(Assignee)

Comment 9

8 years ago
(In reply to comment #8)
> I've pushed your patch with some changes:
> http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/fc73f6ae02b7
> 
> (In reply to comment #4)
> > Anyone know a prettier way to make sure the sort happens numerically?
> 
> Ensure that the items in the array are numbers. You can convert them by
> prefixing them with a plus.

Hmm I don't think that's enough; I tried in my patch with parseInt() around the strings, but still got lexical sort. Consider the output of:

"""
  unsorted = [5,14,13,1,2];
  sorted = [];
  for (var i = 0; i < unsorted.length; i++ ) {
    n = unsorted[i];
    sorted.push(+n);
  }
  alert(sorted.sort());
  alert(sorted.sort(function(a,b){ return a-b }));
"""

First alert:
1,13,14,2,5

Second alert:
1,2,5,13,14
Hmm, you're right, thanks! I'll push the fix.
(Assignee)

Comment 13

8 years ago
(In reply to comment #12)
> Uhm, that was supposed to be
> http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/17ce643cf5aa

Cool, WFM locally now. I think it would have only have bitten when the number went up to the next power of 10, which would be super confusing and hard to reproduce.
Product: Webtools → Tree Management
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.