1164074 - Test chooser in graphs viewer could be better

Status: RESOLVED FIXED

(Tree Management :: Perfherder, defect)

Description

[William Lachance (:wlach)]

Attachment: multi-select-sheriffing-panel.png

The current test chooser is pretty unintuitive. A better interface would be some kind of interface which let you filter through a list and iteratively add specific tests, then add them in a group. As a bonus, this would make things like bug 1134777 easier.

I'm thinking of an interface like this:

[Project]                     | |list of tests to add|
[Platform selector]           | |                    |
[Filter tests input... ]      | |                    |  
|list of filtered tests|      | |    ....            |
|       ....           |      | |                    |
[Add selected tests button]   | |                    |

When the "Add selected tests" button is pressed, anything in the "list of filtered tests" listbox is added to the "list of tests to add" column on the right.

This is a similar idea as what we currently do for the job filtering modification interface in the sheriffing panel (see attached screenshot), you can see the source code to that here:

https://github.com/mozilla/treeherder-ui/blob/master/webapp/app/partials/main/thMultiSelect.html

Comment 1

[MikeLing]

Hello Will, do you still remember me? :)

I'm glad to contribute to Perfherder, I had set my treeherder environment appropriate and jump to Perfherder correctly. Please assign it to me and I will try to my best to fix this issue :)

Great appreciate! 

MikeLing

Comment 2

[MikeLing]

(In reply to William Lachance (:wlach) from comment #0)

Ah, I forget to ask in last comment, the source code you offer in comment 0 is no found. Could you give me another one? And I'm sorry to say I haven't found the Perfherder source code in Treeherder, would you mind to tell me which part should I look if I would like to contribute to Perfherder?  Thank you :)

Comment 3

[William Lachance (:wlach)]

Hi Mike, yes, the UI repository for treeherder has moved. It's now here:

https://github.com/mozilla/treeherder/blob/master/ui/partials/main/thMultiSelect.html

This bug might take a while to implement, I think it would be at least a few days work for me, and I'm as familiar with perfherder as anybody. I might suggest bug 1164058 to start with, as it'll introduce you to some code in the same area. Then take this one on after, if you're feeling comfortable with it...

Comment 4

[MikeLing]

(In reply to William Lachance (:wlach) from comment #3)
> Hi Mike, yes, the UI repository for treeherder has moved. It's now here:
> 
> https://github.com/mozilla/treeherder/blob/master/ui/partials/main/
> thMultiSelect.html
> 
> This bug might take a while to implement, I think it would be at least a few
> days work for me, and I'm as familiar with perfherder as anybody. I might
> suggest bug 1164058 to start with, as it'll introduce you to some code in
> the same area. Then take this one on after, if you're feeling comfortable
> with it...

Sure, I will check the description of bug 1164058 and leave some comment under it. Thank you for you suggestion:)

Comment 5

[MikeLing]

Hi Will,

Although I had talk with Joel by email before I take this bug, but I found it's indeed a difficulty bug for me. Could you offer some more hits for me? Thank you :)

For now,I knew the file I should change is in here: https://github.com/mozilla/treeherder/blob/master/ui/partials/perf/testdatachooser.html and I should check here if I need add some js code: https://github.com/mozilla/treeherder/blob/master/ui/js/graphs.js#L688

Hm, the first question of mine is about how to filtrate the test form testlist. I found there are so many knids of tests in there, but most of them have prefix like 'damp','top50_scoll' and 'txvgs'. I'm not sure if those prefix can help.

Second thing is about the graph's outlook, I just make it as you said in comment 0. I would like to commit a mock up. Please tell me what your think about it :)

I'll try my best to fix it. Thank you

Comment 6

[MikeLing]

Attachment: mockup.png

btw, have you see the pr in https://github.com/mozilla/treeherder/pull/614 ?

Comment 7

[William Lachance (:wlach)]

Comment on attachment 8620972 [details]
mockup.png

Yes, this is more or less what I had in mind!

I'd probably replace "Filter tests" with just "Tests". Remove the "List of filtered tests" text altogether. The button to add tests should just be a simple chevron on the right of the list of tests (following the example of the exclusion profile modification interface in the rest of treeherder, I'll post an example). Replace "List of test to add" with "Tests to add". Make that element the full height of the dialog. Do all that and write the code to make it work and we're good to go. :)

Comment 8

[William Lachance (:wlach)]

Oh wait, I already did post a link to the sheriffing panel. :) See the attachments.

Comment 9

[William Lachance (:wlach)]

(In reply to MikeLing from comment #5)
> Hi Will,
> 
> Although I had talk with Joel by email before I take this bug, but I found
> it's indeed a difficulty bug for me. Could you offer some more hits for me?
> Thank you :)
> 
> For now,I knew the file I should change is in here:
> https://github.com/mozilla/treeherder/blob/master/ui/partials/perf/
> testdatachooser.html and I should check here if I need add some js code:
> https://github.com/mozilla/treeherder/blob/master/ui/js/graphs.js#L688
> 
> Hm, the first question of mine is about how to filtrate the test form
> testlist. I found there are so many knids of tests in there, but most of
> them have prefix like 'damp','top50_scoll' and 'txvgs'. I'm not sure if
> those prefix can help.

Hi Mike, it's no problem that you're finding this a bit difficult, you're still new to perfherder. It'll probably take us a bit of work (maybe a couple weeks?) to get this exactly right, but that's ok. Try to figure out as much as you can on your own and I'll answer questions as I am able. :) And feel free to work on other stuff at the same time if you feel blocked.

To answer your question on filtering, the trick is going to be populating the list of possible tests to add based on what the user enters into the filter box. We currently use twitter's typeahead.js library for this which we *won't* be using in the future (it's a great library, but not what we want here), but you see how we use the name property of the series with its filter engine here:

https://github.com/mozilla/treeherder/blob/master/ui/js/graphs.js#L727

Part of your task will be writing a function which takes a string and passes back all the tests whose name has that string in it (subject also to the above filtering on project on platform). You can use that function to populate the list dialog which the user can use to select the test. Every time the string in the filter box changes, update the box with the new list of tests.

Comment 10

[MikeLing]

Attachment: mockup.png

Hi Will,

I add a mockup for the tests chooser graph, please tell me if it fit your mind :)

Actually, I still confuse about how to write a filter. In the last comment, I haven't find out how we use the name property of the series with typeahead's filter engine in https://github.com/mozilla/treeherder/blob/master/ui/js/graphs.js#L727. It looks like add the series data we select in testcooser view to the graph view rather than filter the series data.

And I found another place maybe the place we use typeahead (https://github.com/mozilla/treeherder/blob/master/ui/js/graphs.js#L766). But I don't know how to combine typeahead with multiple form control. However, if I need to write a function to filter input string and pass back all the result to user without typeahead. I think the first problem is how to get the series data from remote.

Could you offer me some hints? maybe tell me what documents or code I should to read will be enough. Thank you very much:)

Comment 11

[MikeLing]

Attachment: directive.patch

I write a directive to finish job, but the problem is it seems doesn't work at all. I mean nothing happened when I click the 'move_right' button but it should move the item I selected to the right. 

I try to figure out why but failed, could you give me a hit about that? Thank you :)

Comment 12

[William Lachance (:wlach)]

(In reply to MikeLing from comment #11)
> Created attachment 8625149 [details] [diff] [review]
> directive.patch
> 
> I write a directive to finish job, but the problem is it seems doesn't work
> at all. I mean nothing happened when I click the 'move_right' button but it
> should move the item I selected to the right. 
> 
> I try to figure out why but failed, could you give me a hit about that?
> Thank you :)

It's really not obvious to me from the code what's going on. It might be easier for me to just run the whole thing and see how it works. To make that easier, could you just attach a PR for all your changes at and attach it to this bug? Please ask for feedback (f?) so it appears on my queue.

Comment 13

[MikeLing]

Attachment: PR

After talk with Joel on email. I add a perf.js in js/directives and quote it in perf. html. But still can't work.

Comment 14

[William Lachance (:wlach)]

Comment on attachment 8626540 [details] [review]
PR

Just more karma conf modifications to make, I think. Commented in the PR.

Give my suggestion a try and fix the nits noticed by jfrench. Then please re-raise f? :)

Comment 15

[MikeLing]

Comment on attachment 8626540 [details] [review]
PR

Build success! :)

Comment 16

[William Lachance (:wlach)]

Comment on attachment 8626540 [details] [review]
PR

Cool! Left some comments in the PR. Looking forward to the next iteration of this.

Comment 17

[William Lachance (:wlach)]

Comment on attachment 8626540 [details] [review]
PR

Still needs a bit of work, good progress though!

Comment 18

[MikeLing]

Comment on attachment 8626540 [details] [review]
PR

One more question, should I restore those directives back to ui/js/directive because we don't need directive in this issue any more? Or just leave it alone?

Beside that, I think it can work appropriate now :)

Comment 19

[William Lachance (:wlach)]

Comment on attachment 8626540 [details] [review]
PR

Getting close! See PR for comments.

Comment 20

[William Lachance (:wlach)]

Comment on attachment 8626540 [details] [review]
PR

Still a few more things to fixup. Almost.

Comment 21

[William Lachance (:wlach)]

Comment on attachment 8626540 [details] [review]
PR

One last problem with old typeahead CSS. Fix that and we're good to go!

Comment 22

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/6fefdf0147267f2da15f4a7ccea61bfb46f0d98a
Bug 1164074 - Further improve Perf test chooser

https://github.com/mozilla/treeherder/commit/12f80ce4486497e33131faeaae19429f1e00ff4e
Bug 1164074 - Remove some extra typeahead CSS

Comment 23

[William Lachance (:wlach)]

Comment on attachment 8626540 [details] [review]
PR

I found some last bits of typeahead CSS in the perf.css file, which I removed myself before pushing. Great to have this finished! Thanks Mike Ling!