Group similar failures (same directory, same fixture)

Group similar failures (same directory, same fixture)

Also add a nice paper expand button.

BUG=399717
R=ojan@chromium.org
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180065

[jochen (gone - plz use gerrit), 2014-08-08 13:59:21]

ptal, looks like this: http://imgur.com/zl15Uzm

[ojan, 2014-08-11 02:18:56]

This is going to hit the same issue as https://codereview.chromium.org/441393002
of the expanding going away on update. The good news is that
https://codereview.chromium.org/448503003 has landed and someone could do the
work necessary to make it so that model changes persist through updates.

Dan, Jochen are either of you willing to do the legwork to fix the underlying
issue?

Other than that, this patch mostly looks good. Just a few minor things.

https://codereview.chromium.org/458473003/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-test-list-tests.html (right):

https://codereview.chromium.org/458473003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-test-list-tests.html:82: asyncTest("grouping", 2,
function() {
Please also have a test of layout tests.

https://codereview.chromium.org/458473003/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-test-list.html (right):

https://codereview.chromium.org/458473003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-test-list.html:37: <template if="{{ group.name &&
group.tests.length > 1 && !group.expanded}}">
Here and below, put spaces after {{ and before }}.

https://codereview.chromium.org/458473003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-test-list.html:54: var testName =
this._nameForTest(test);
How about if we put a method reasonGroupName on CTFailure and use that here? I
want the '.' vs '/' distinction to be in the model code, not in the ui code.

About the confusing naming: We're trying to s/testName/reason everywhere since
the failure reasons are no longer just tests. We also need to s/test/failure
everywhere in the code (e.g. this component should be ct-failure-list).

https://codereview.chromium.org/458473003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-test-list.html:68: this.testGroups_.push({'step':
step, 'tests': tests});
this.testGroups_ should be sorted by step and tests should be sorted by name?

https://codereview.chromium.org/458473003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-test-list.html:75: if (test.step == 'blink')
Shouldn't this be webkit_tests?

https://codereview.chromium.org/458473003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-test-list.html:76: return test.testName.substr(0,
test.testName.lastIndexOf('/'));
We'll probably want to have more complicated grouping for layout tests since
there are multiple levels of hierarchy there, but we can start with this and see
if we need more.

[jochen (gone - plz use gerrit), 2014-08-11 11:46:57]

updated

I'm not sure whether we should put this into localStorage? Why not
sessionStorage?

Also, we'll probably want some global key managed, so the different parts don't
store stuff with colliding keys

in short: I punted on this issue in this CL :)

[jochen (gone - plz use gerrit), 2014-08-11 12:59:36]

https://codereview.chromium.org/458473003/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-test-list-tests.html (right):

https://codereview.chromium.org/458473003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-test-list-tests.html:82: asyncTest("grouping", 2,
function() {
done

https://codereview.chromium.org/458473003/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-test-list.html (right):

https://codereview.chromium.org/458473003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-test-list.html:37: <template if="{{ group.name &&
group.tests.length > 1 && !group.expanded}}">
done

https://codereview.chromium.org/458473003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-test-list.html:54: var testName =
this._nameForTest(test);
added the method to CTFailure

https://codereview.chromium.org/458473003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-test-list.html:68: this.testGroups_.push({'step':
step, 'tests': tests});
Done

https://codereview.chromium.org/458473003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-test-list.html:75: if (test.step == 'blink')
done

[ojan, 2014-08-11 19:41:06]

lgtm. please address comments before committing.

On 2014/08/11 at 11:46:57, jochen wrote:
> I'm not sure whether we should put this into localStorage? Why not
sessionStorage?
> 
> Also, we'll probably want some global key managed, so the different parts
don't store stuff with colliding keys
> 
> in short: I punted on this issue in this CL :)

I don't think we need to store this at all. If you refresh the page, it should
lose your expand/collapsed state. The part that needs to be fixed is that the
behind-the-scenes auto-updating that happens needs to not lose your collapsed
state.

I guess we can just land this with the known bug and fix the bug after the fact.

https://codereview.chromium.org/458473003/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-test-list-tests.html (right):

https://codereview.chromium.org/458473003/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-test-list-tests.html:54:
Platform.endOfMicrotask(function() {
endOfMicrotask is a weird broken API. It basically fires at a totally random
time. We were using it initially before we realized it was broken. Then we
switched to using requestAnimationFrame and there were problems with that. So
now we're just using setTimeout until we find a reason why that doesn't work
either. :( See https://codereview.chromium.org/462453002/#msg2 for more details.

https://codereview.chromium.org/458473003/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-test-list.html (right):

https://codereview.chromium.org/458473003/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-test-list.html:68:
tests.sort(this._comparator.bind(this, 'name'));
Turns out sugarjs, which we're using, has a method for this:
http://sugarjs.com/api/Array/sortBy

I haven't used it, but I think this would just be:
tests.sortBy('name');

https://codereview.chromium.org/458473003/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-test-list.html:71:
this.testGroups_.sort(this._comparator.bind(this, 'step'));
Ditto

[jochen (gone - plz use gerrit), 2014-08-12 11:55:51]

https://codereview.chromium.org/458473003/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-test-list-tests.html (right):

https://codereview.chromium.org/458473003/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-test-list-tests.html:54:
Platform.endOfMicrotask(function() {
done

https://codereview.chromium.org/458473003/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-test-list.html (right):

https://codereview.chromium.org/458473003/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-test-list.html:68:
tests.sort(this._comparator.bind(this, 'name'));
done

[jochen (gone - plz use gerrit), 2014-08-12 11:56:06]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2014-08-12 11:56:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jochen@chromium.org/458473003/60001

[commit-bot: I haz the power, 2014-08-12 11:57:37]

Change committed as 180065