655880 - Next/prev unstarred failure key shortcuts have wrong order

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Steve Fink [:sfink] [:s:]]

Bug 626439 added 'n' and 'p' keys for navigating through unstarred failures, but the order that failures are traversed is incorrect, so it appears to be jumping randomly through the builds.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Fix ordering of unstarred failure traversal with n/p keys

Sorry about the flurry of review requests. (There are more coming.)

This changes 'n'/'p' to use the sort order that the builds are displayed in.

Comment 2

[Arpad Borsos [:Swatinem]]

Comment on attachment 531193 [details] [diff] [review]
Fix ordering of unstarred failure traversal with n/p keys

Review of attachment 531193 [details] [diff] [review]:
-----------------------------------------------------------------

Well the order was intentionally the order of the boxes in the overview on the top right (red before orange). It’s right that that conflicts with the order of the builds in the normal view.

While I do like the patch, I would still want to get some feedback from philor regarding the behavior change.

::: js/UserInterface.js
@@ +623,5 @@
>        // Move between unstarred failing jobs with 'N' and 'P' keys.
>        if (event.which == 110 || event.which == 112) {
> +        var unstarred = self._getFailingJobs().filter(function (job) {
> +          return self._isUnstarredFailure(job);
> +        });

Nice simplification, but please keep the comment.

@@ +650,3 @@
>                  result = 0;
> +              else if (result < 0)
> +                result = unstarred.length;

again, good simplification.

@@ +984,5 @@
>      return Controller.keysFromObject(Config.OSNames).map(function buildHTMLForPushResultsOnOS(os) {
>        if (!push.results || !push.results[os])
>          return '';
> +      return (push.results[os].opt   ? self._buildHTMLForOS(os, " opt"  , push.results[os].opt, order  ) : '') +
> +             (push.results[os].debug ? self._buildHTMLForOS(os, " debug", push.results[os].debug, order) : '');

The intention of the indentation is so that “order” nicely lines up.

Comment 3

[Arpad Borsos [:Swatinem]]

Comment on attachment 531193 [details] [diff] [review]
Fix ordering of unstarred failure traversal with n/p keys

Review of attachment 531193 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/UserInterface.js
@@ +650,3 @@
>                  result = 0;
> +              else if (result < 0)
> +                result = unstarred.length;

I missed that somehow, the - 1 is missing there.

Comment 4

[Steve Fink [:sfink] [:s:]]

(In reply to comment #2)
> Comment on attachment 531193 [details] [diff] [review] [review]
> Well the order was intentionally the order of the boxes in the overview on
> the top right (red before orange). It’s right that that conflicts with the
> order of the builds in the normal view.

Oh! That makes sense. I always forget about those builds in the top right; I only use them for a count.

> While I do like the patch, I would still want to get some feedback from
> philor regarding the behavior change.

Yes, now I'm not sure either ordering is correct. Does separating the warnings and errors in the top right really help? Or conversely, does it matter whether it jumps all around in the push builds area?

> @@ +984,5 @@
> >      return Controller.keysFromObject(Config.OSNames).map(function buildHTMLForPushResultsOnOS(os) {
> >        if (!push.results || !push.results[os])
> >          return '';
> > +      return (push.results[os].opt   ? self._buildHTMLForOS(os, " opt"  , push.results[os].opt, order  ) : '') +
> > +             (push.results[os].debug ? self._buildHTMLForOS(os, " debug", push.results[os].debug, order) : '');
> 
> The intention of the indentation is so that “order” nicely lines up.

Oops, wasn't paying attention.

(In reply to comment #3)
> @@ +650,3 @@
> >                  result = 0;
> > +              else if (result < 0)
> > +                result = unstarred.length;
> 
> I missed that somehow, the - 1 is missing there.

Doh!

Comment 5

[Phil Ringnalda (:philor)]

I personally think of the boxes in the top right as a sort of unreliable look at how the tip of the tree is currently doing, and as having very nearly nothing to do with starring, so I didn't have any idea that was what the keyboard shortcuts were advancing through. I just thought they were broken, since they would bounce around an unstarred orange I could see right there in front of me. Did we actually want to make them do that, or did we settle for that because that was the only list of failed builds we have, or did we just not realize what we were getting?

Comment 6

[Phil Ringnalda (:philor)]

Comment on attachment 531193 [details] [diff] [review]
Fix ordering of unstarred failure traversal with n/p keys

f+ in a very narrow way: given a set of failed builds that a keyboard shortcut is going to iterate through, I don't think there's any need to have it go through all of the red and purple (and whatever color unknown is) ones first, and then loop back around and go through the orange ones. The sorting of the top right boxes was sort of reasonable when there were just two colors, and red mostly meant "omg, you broke compiling" while orange mostly meant "it's just another of our many many randomoranges." Now that red is quite often mislabeled purple and we're sticking purple things that aren't your fault or problem in with red things, and orange is more likely to be the reason you have to back out than to be randomorange, it's not at all a severity or priority sort, so it's just confusing.

But only + in that narrow sense: I think we would be better off having no keyboard shortcut at all than having one which only goes through the failing builds which haven't had a green run afterward while ignoring unstarred intermittent failures.

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Fix ordering of unstarred failure traversal with n/p keys

philor clarified that last paragraph over IRC. The problem is that n/p were not iterating over all unstarred failing jobs; they were iterating only over the latest result from each type of test. If there is more than one result for a given job type, that's just about the opposite of what you'd want: a test went orange, you retriggered, it came out green -- and it doesn't let you select the initial failure to star it as an intermittent orange.

I've updated the patch to iterate over *all* unstarred failing jobs instead, using some code that is uncomfortably similar to other code already there. I suppose I ought to refactor it for reuse...

Comment 8

[Arpad Borsos [:Swatinem]]

Well yes, it was intentionally tied to the colored boxes up right. Good that we have cleared it up that we would want some very different semantics here. Off to the review I am ;-)

Comment 9

[Arpad Borsos [:Swatinem]]

Comment on attachment 531548 [details] [diff] [review]
Fix ordering of unstarred failure traversal with n/p keys

Review of attachment 531548 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/UserInterface.js
@@ +523,5 @@
> +      return !(result.state == 'pending' ||
> +               result.state == 'retry' ||
> +               result.state == 'unknown');
> +    });
> +  },

You are right, that is awfully familiar :-D

Comment 10

[Arpad Borsos [:Swatinem]]

We should probably discuss what to do with the boxes on the upper right. If they don’t provide any real value we should fix them.
File a followup for discussion please.

Comment 11

[Steve Fink [:sfink] [:s:]]

(In reply to comment #10)
> We should probably discuss what to do with the boxes on the upper right. If
> they don’t provide any real value we should fix them.
> File a followup for discussion please.

bug 656732

Comment 12

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/c4e9cde6dd1c