Add failed builders to sheriff-o-matic.

Add failed builders to sheriff-o-matic.

This displays the non-layout builders and their failed steps at the top.

I am having trouble with ct-failing-builders-tests.html...

R=abarth@chromium.org,ojan@chromium.org,esprehn@chromium.org
CC=team-ojan@chromium.org

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

[michaelpg, 2014-07-07 23:15:20]

abarth@ or esprehn@, could you take a look to help me figure out why
ct-failing-builders-tests.html is failing?

https://codereview.chromium.org/371133003/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-failing-builders-tests.html (right):

https://codereview.chromium.org/371133003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failing-builders-tests.html:32: ok(steps.innerText
== 'compile' ||
Without the timeout, this throws:
    Uncaught TypeError: Cannot read property 'innerText' of null

In each <ct-builder>'s shadow root, I see that the builderName attribute is
correctly placed into the DOM, but the stepList property is not. The debugger
shows that the ct-builder's stepList property is getting set inside the element,
but it just doesn't appear in the shadow root until some time has passed.

How can I account for this in the test?

<template>
....
  <template if="{{ stepList }}">   
    <span class="details">{{ stepList }}</span>    
  </template> 
  <!-- "details" span is not created yet but this.stepList is a truthy string
-->     
....
</template>

[abarth-chromium, 2014-07-07 23:56:05]

https://codereview.chromium.org/371133003/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-failing-builders-tests.html (right):

https://codereview.chromium.org/371133003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failing-builders-tests.html:32: ok(steps.innerText
== 'compile' ||
Maybe call Platform.endOfMicrotask again instead of setTimeout?

[michaelpg, 2014-07-07 23:58:17]

https://codereview.chromium.org/371133003/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-failing-builders-tests.html (right):

https://codereview.chromium.org/371133003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-failing-builders-tests.html:32: ok(steps.innerText
== 'compile' ||
On 2014/07/07 23:56:05, abarth wrote:
> Maybe call Platform.endOfMicrotask again instead of setTimeout?

I'm not sure what endOfMicrotask does (haven't found documentation and the code
is minified). I tried adding another call inside of the outer endOfMicrotask
call but it didn't seem to have an effect.

[abarth-chromium, 2014-07-07 23:59:45]

https://codereview.chromium.org/371133003/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-builder.html (right):

https://codereview.chromium.org/371133003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-builder.html:73: },
The microtask checkpoint is a queue.  These lines of code add some more work to
the queue (i.e., to process that the stepList changed).  Your test code appears
to be running after this code has run but before the work it has queued runs. 
If you add another Platform.endOfMicrotask call, you'll go back to the end of
the queue and the system should have processed the change to the step list by
then.

[michaelpg, 2014-07-08 00:56:03]

https://codereview.chromium.org/371133003/diff/1/Tools/GardeningServer/ui/ct-...
File Tools/GardeningServer/ui/ct-builder.html (right):

https://codereview.chromium.org/371133003/diff/1/Tools/GardeningServer/ui/ct-...
Tools/GardeningServer/ui/ct-builder.html:73: },
On 2014/07/07 23:59:45, abarth wrote:
> The microtask checkpoint is a queue.  These lines of code add some more work
to
> the queue (i.e., to process that the stepList changed).  Your test code
appears
> to be running after this code has run but before the work it has queued runs.

Yes, this is exactly what's happening -- the template changes haven't been
performed yet.
 
> If you add another Platform.endOfMicrotask call, you'll go back to the end of
> the queue and the system should have processed the change to the step list by
> then.

Do you mean nest the call, or add them sequentially? Regardless, I've tried
adding a dozen calls and it doesn't help. Using el.async instead of
Platform.endOfMicrotask does work, though. :-\

[abarth-chromium, 2014-07-08 02:02:58]

On 2014/07/08 at 00:56:03, michaelpg wrote:
> > If you add another Platform.endOfMicrotask call, you'll go back to the end
of
> > the queue and the system should have processed the change to the step list
by
> > then.
> 
> Do you mean nest the call, or add them sequentially? Regardless, I've tried
adding a dozen calls and it doesn't help. Using el.async instead of
Platform.endOfMicrotask does work, though. :-\

I meant nested...  Not sure what to recommend.  I'd need to debug it locally to
see what's going on.

[michaelpg, 2014-07-08 23:44:38]

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-failing-builders-tests.html (right):

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failing-builders-tests.html:24: el.async(function()
{
This works. But still waiting to hear from someone smarter than me about what we
actually want to do here.

[ojan, 2014-07-10 02:26:43]

A few nits.

We should figure out what's going on with the async thing before committing
though. Either there's a polymer bug here, or there's something we don't
understand about how polymer works. In either case, would be good to know before
we plow forward.

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-failing-builders-tests.html (right):

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failing-builders-tests.html:24: el.async(function()
{
I'm also not sure what Platform.endOfMicroTask actually does. :( Your original
code (except without the setTimeout) looked correct to me. You set .builders and
then call endOfMicroTask. Seems like that ordering should work. esprehn, any
idea what's going on here?

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failing-builders-tests.html:28: ok(steps.innerText
== 'compile' ||
Nit: use textContent. innerText is super crazy (e.g. does line breaks and what
not based off the rendered state).

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-failing-builders.html (right):

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failing-builders.html:25: steps="{{
builders[builder] }}">
Nit: lets use 4 space indent for wrapped lines. That way the wrapping isn't
affected by the length of the tagname.

Also, FWIW, I wouldn't have wrapped this at all.

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-unexpected-failures.html (right):

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-unexpected-failures.html:27: ct-building-grid {
This is unused?

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-unexpected-failures.html:32: margin-left: 365px;
I realize the old code probably did something like this, but lets instead use
flexbox. This is just trying to get the failing builders list to be to the right
of the button, right?

[ojan, 2014-07-10 02:27:48]

Also, I gave up on the team-ojan mailing list. It seemed like a good idea at the
time, but it's looking less like we'll have a dedicated team working on this and
more of a spread out collaboration among a bunch of different teams.

[michaelpg, 2014-07-10 02:53:28]

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-failing-builders-tests.html (right):

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failing-builders-tests.html:24: el.async(function()
{
On 2014/07/10 02:26:43, ojan-only-code-yellow-reviews wrote:
> I'm also not sure what Platform.endOfMicroTask actually does. :( Your original
> code (except without the setTimeout) looked correct to me. You set .builders
and
> then call endOfMicroTask. Seems like that ordering should work. esprehn, any
> idea what's going on here?

The problem is endOfMicrotask doesn't actually do enough here.

https://github.com/Polymer/platform-dev/issues/33 has been opened.

async looks like the way to go for now.

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failing-builders-tests.html:28: ok(steps.innerText
== 'compile' ||
On 2014/07/10 02:26:43, ojan-only-code-yellow-reviews wrote:
> Nit: use textContent. innerText is super crazy (e.g. does line breaks and what
> not based off the rendered state).

Done, sorry, I knew this already.

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-failing-builders.html (right):

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failing-builders.html:25: steps="{{
builders[builder] }}">
On 2014/07/10 02:26:43, ojan-only-code-yellow-reviews wrote:
> Nit: lets use 4 space indent for wrapped lines. That way the wrapping isn't
> affected by the length of the tagname.
> 
> Also, FWIW, I wouldn't have wrapped this at all.

Done.

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-unexpected-failures.html (right):

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-unexpected-failures.html:27: ct-building-grid {
On 2014/07/10 02:26:43, ojan-only-code-yellow-reviews wrote:
> This is unused?

yes, not sure how this got here.

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-unexpected-failures.html:32: margin-left: 365px;
On 2014/07/10 02:26:43, ojan-only-code-yellow-reviews wrote:
> I realize the old code probably did something like this, but lets instead use
> flexbox. This is just trying to get the failing builders list to be to the
right
> of the button, right?

The goal is to get the failing builders to align with the right-hand side of the
page. It looks kind of ugly any other way I tried it. 365px corresponds with the
width + margin of ct-builder-grid, which sets its own width to 350px.

The problem is ct-unexpected-failures doesn't know how ct-failure-stream ->
ct-failure-card -> ct-builder-grid is laid out. 

Screenshot:
https://drive.google.com/a/google.com/file/d/0B6HSBrih6pNUU3hyZnBtcHNENXc/edi...

[ojan, 2014-07-10 17:58:50]

lgtm

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-unexpected-failures.html (right):

https://codereview.chromium.org/371133003/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-unexpected-failures.html:32: margin-left: 365px;
On 2014/07/10 02:53:28, Michael Giuffrida wrote:
> On 2014/07/10 02:26:43, ojan-only-code-yellow-reviews wrote:
> > I realize the old code probably did something like this, but lets instead
use
> > flexbox. This is just trying to get the failing builders list to be to the
> right
> > of the button, right?
> 
> The goal is to get the failing builders to align with the right-hand side of
the
> page. It looks kind of ugly any other way I tried it. 365px corresponds with
the
> width + margin of ct-builder-grid, which sets its own width to 350px.
> 
> The problem is ct-unexpected-failures doesn't know how ct-failure-stream ->
> ct-failure-card -> ct-builder-grid is laid out. 
> 
> Screenshot:
>
https://drive.google.com/a/google.com/file/d/0B6HSBrih6pNUU3hyZnBtcHNENXc/edi...
> 

I see. What we really want here is grid layout and/or CSS variables. But, since
we don't have either of those shipping, we'll have to make due with something
like this.

https://codereview.chromium.org/371133003/diff/60001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-builder-tests.html (right):

https://codereview.chromium.org/371133003/diff/60001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-builder-tests.html:34:
Platform.endOfMicrotask(function() {
Should we use async here has well then? Seems like endOfMicrotask is something
we should never use.

[michaelpg, 2014-07-14 18:40:22]

https://codereview.chromium.org/371133003/diff/60001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-builder-tests.html (right):

https://codereview.chromium.org/371133003/diff/60001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-builder-tests.html:34:
Platform.endOfMicrotask(function() {
On 2014/07/10 17:58:50, ojan-only-code-yellow-reviews wrote:
> Should we use async here has well then? Seems like endOfMicrotask is something
> we should never use.

Done.

[michaelpg, 2014-07-16 01:30:54]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-07-16 01:31:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaelpg@chromium.org/371133003/100001

[commit-bot: I haz the power, 2014-07-16 02:35:45]

Change committed as 178231