cc: Adding presubmit to include blink tests

cc: Adding presubmit to include blink tests

Changes in the src/cc directory can often affect the layout tests in Blink
however Chromium commit queue doesn't run these tests by default. This leads to
breakages being discovered only when the Blink dep is rolled into Chromium.

By adding the following line to the CL's description the Blink tests will be
run in addition to the normal Chromium tests;
 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

BUG=483372
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/cf78875226f38e6add4b9559093bbb4ca2c5c311
Cr-Commit-Position: refs/heads/master@{#330911}

[mithro-old, 2015-05-20 03:18:38]

The CQ bit was checked by mithro@mithis.com to run a CQ dry run

[commit-bot: I haz the power, 2015-05-20 03:19:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142393002/1

[mithro-old, 2015-05-20 03:21:46]

mithro@mithis.com changed reviewers:
+ danakj@chromium.org, noel@chromium.org

[mithro-old, 2015-05-20 03:21:47]

Hi,

This patch automatically adds the blink tests to the run as suggested in
https://code.google.com/p/chromium/issues/detail?id=483372

Still currently testing the CQ_INCLUDE_TRYBOTS flag does as requested. The place
I copied the hook from (src/tools/perf/PRESUBMIT.py) used CQ_EXTRA_TRYBOTS
instead but I thought I would send it out now.

Tim 'mithro' Ansell

[Noel Gordon, 2015-05-20 03:42:54]

Looks about right. One question: does that change add 

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel
  
or

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel

?

[mithro-old, 2015-05-20 04:18:25]

On 2015/05/20 03:42:54, noel gordon wrote:
> Looks about right. One question: does that change add 
> 
>
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel
>   
> or
> 
> CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel
> 
> ?

You can see the line the PRESUBMIT added to this CL description;

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel

This format seems to work as I then ran this CL through the commit queue in "dry
run" mode (cool new feature) and it appears to be running the bots. See
https://chromium-cq-status.appspot.com/patch-status/1142393002/1

I'm a little concerned that the Blink bots do appear to be the "long pole" on
submitting these changes now. I think the little bit extra wait is probably
worth the extra confidence in not breaking Blink. Dana, what do you think?

I'm also wondering if the GetPreferredTryMasters function (see just above the
PostUploadHook) should share the output with the PostUploadHook, but that is
probably worth bumping to another CL.

Tim 'mithro' Ansell

[commit-bot: I haz the power, 2015-05-20 04:28:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-20 04:28:22]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2015-05-20 05:17:14]

On 2015/05/20 04:18:25, mithro wrote:
> On 2015/05/20 03:42:54, noel gordon wrote:
> > Looks about right. One question: does that change add 
> > 
> >
>
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel
> >   
> > or
> > 
> >
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel
> > 
> > ?
> 
> You can see the line the PRESUBMIT added to this CL description;
> 
>
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel

Possible to use the short form?

> I'm a little concerned that the Blink bots do appear to be the "long pole" on
> submitting these changes now. I think the little bit extra wait is probably
> worth the extra confidence in not breaking Blink. Dana, what do you think?

In my mind, it's a much longer pole for the Blink gardener to work out what went
wrong and find the CC offending change that needs reverting. Once the
Blink/Chrome merge is done, the long pole of Blink layout test bots will be a
given.

Until then, given your concerns, one option might be to pick the fastest of
these Blink layout try bots, eg., which one does Skia use, and maybe use that?

[danakj, 2015-05-20 21:05:26]

Does just using linux make things faster?

https://codereview.chromium.org/1142393002/diff/1/cc/PRESUBMIT.py
File cc/PRESUBMIT.py (right):

https://codereview.chromium.org/1142393002/diff/1/cc/PRESUBMIT.py#newcode353
cc/PRESUBMIT.py:353: 'mac_blink_rel',
Do you think we benefit much from adding mac/win here? I think linux is probably
enough for now?

[mithro-old, 2015-05-21 03:30:17]

The CQ bit was checked by mithro@mithis.com to run a CQ dry run

[commit-bot: I haz the power, 2015-05-21 03:30:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142393002/1

[mithro-old, 2015-05-21 03:58:02]

I've run this change through the commit queue on dry run twice now and got the
following result;

 * It seems to take roughly 1 hour +/- 10 minutes
 * The difference in speed between the win/mac/linux bots seems insignificant
compared to the time take to actually run (they run in parallel so finish pretty
much simultaneously).

I'm unsure if this would change during MTV busy hours or not? I know Mac
resources are currently fairly constrained but they are purchasing hardware to
fix that...

Noel - any thoughts on the value of running just Linux verses running all of
Linux, Mac and Windows?

[Noel Gordon, 2015-05-21 04:12:12]

Start with Linux for now: CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel is
what Skia uses.

[Noel Gordon, 2015-05-21 05:15:34]

https://codereview.chromium.org/1142393002/diff/1/cc/PRESUBMIT.py
File cc/PRESUBMIT.py (right):

https://codereview.chromium.org/1142393002/diff/1/cc/PRESUBMIT.py#newcode364
cc/PRESUBMIT.py:364: rietveld_obj.update_description(issue, description)
nit: indent 2

[commit-bot: I haz the power, 2015-05-21 05:16:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-21 05:16:26]

Dry run: This issue passed the CQ dry run.

[mithro-old, 2015-05-21 05:26:58]

I ended up refactoring this change to use the GetPreferredTryMasters method to
prevent them getting out of sync.

> Start with Linux for now: 
> CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel is
> what Skia uses.

Done.

https://codereview.chromium.org/1142393002/diff/1/cc/PRESUBMIT.py
File cc/PRESUBMIT.py (right):

https://codereview.chromium.org/1142393002/diff/1/cc/PRESUBMIT.py#newcode353
cc/PRESUBMIT.py:353: 'mac_blink_rel',
On 2015/05/20 21:05:26, danakj wrote:
> Do you think we benefit much from adding mac/win here? I think linux is
probably
> enough for now?

We decided to just use linux_blink_rel for now.

https://codereview.chromium.org/1142393002/diff/1/cc/PRESUBMIT.py#newcode364
cc/PRESUBMIT.py:364: rietveld_obj.update_description(issue, description)
On 2015/05/21 05:15:34, noel gordon wrote:
> nit: indent 2

Done.

[Noel Gordon, 2015-05-21 06:15:16]

OK, sounds good. Let me review in detail now.

[Noel Gordon, 2015-05-21 06:20:23]

LGTM with nits (not sure I can approve python CC changes though)

https://codereview.chromium.org/1142393002/diff/40001/cc/PRESUBMIT.py
File cc/PRESUBMIT.py (right):

https://codereview.chromium.org/1142393002/diff/40001/cc/PRESUBMIT.py#newcode345
cc/PRESUBMIT.py:345: original_description = rietveld_obj.get_description(issue)
original_description -> description ...

https://codereview.chromium.org/1142393002/diff/40001/cc/PRESUBMIT.py#newcode347
cc/PRESUBMIT.py:347: r'^CQ_INCLUDE_TRYBOTS=.*', original_description, re.M |
re.I):
then close up this line (it should fit on one line now).

https://codereview.chromium.org/1142393002/diff/40001/cc/PRESUBMIT.py#newcode356
cc/PRESUBMIT.py:356: description = original_description
Write this as:

new_description = description

[mithro-old, 2015-05-21 06:25:00]

Patchset #5 (id:80001) has been deleted

[mithro-old, 2015-05-21 06:25:11]

Done.

https://codereview.chromium.org/1142393002/diff/40001/cc/PRESUBMIT.py
File cc/PRESUBMIT.py (right):

https://codereview.chromium.org/1142393002/diff/40001/cc/PRESUBMIT.py#newcode345
cc/PRESUBMIT.py:345: original_description = rietveld_obj.get_description(issue)
On 2015/05/21 06:20:23, noel gordon wrote:
> original_description -> description ...

Done.

https://codereview.chromium.org/1142393002/diff/40001/cc/PRESUBMIT.py#newcode347
cc/PRESUBMIT.py:347: r'^CQ_INCLUDE_TRYBOTS=.*', original_description, re.M |
re.I):
On 2015/05/21 06:20:23, noel gordon wrote:
> then close up this line (it should fit on one line now).

Done.

https://codereview.chromium.org/1142393002/diff/40001/cc/PRESUBMIT.py#newcode356
cc/PRESUBMIT.py:356: description = original_description
On 2015/05/21 06:20:23, noel gordon wrote:
> Write this as:
> 
> new_description = description

Done.

[Noel Gordon, 2015-05-21 06:27:09]

LGTM - thanks for working on this TIm.

[mithro-old, 2015-05-21 06:32:20]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2015-05-21 06:32:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142393002/100001

[commit-bot: I haz the power, 2015-05-21 07:26:43]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-05-21 07:27:35]

Patchset 5 (id:??) landed as
https://crrev.com/cf78875226f38e6add4b9559093bbb4ca2c5c311
Cr-Commit-Position: refs/heads/master@{#330911}