Add tests for Headless Linux bot

Add tests for Headless Linux bot

BUG=594108
Committed: https://crrev.com/b8b863a043b50ec783dc9cacd683700802a27130
Cr-Commit-Position: refs/heads/master@{#387563}

[perezju, 2016-03-29 15:04:48]

perezju@chromium.org changed reviewers:
+ jbudorick@chromium.org, skyostil@google.com

[jbudorick, 2016-03-29 15:15:14]

You'll also want to add a configuration for this bot to
//tools/mb/mb_config.pyl. That should specify what gn arguments to use (in this
case, probably match what Linux Builder does on chromium.linux and add
is_headless=true)

[Sami, 2016-03-29 15:30:04]

skyostil@chromium.org changed reviewers:
+ skyostil@chromium.org

[Sami, 2016-03-29 15:30:04]

Looks great. Not completely sure if we need "raw" or "console_test_launcher" but
I guess we can revisit.

[perezju, 2016-03-29 16:10:03]

perezju@chromium.org changed reviewers:
+ dpranke@chromium.org

[perezju, 2016-03-29 16:10:03]

Addressed both comments.

Dirk, could you also have a look at the mb/gn configs and let us know whether
they look fine? Thanks!

[Dirk Pranke, 2016-03-29 19:25:20]

https://codereview.chromium.org/1843623002/diff/20001/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/1843623002/diff/20001/tools/mb/mb_config.pyl#...
tools/mb/mb_config.pyl:1122: 'gn', 'headless', 'debug_bot', 'x64',
You should not specify x64; it's the default (except for win gyp builds).

https://codereview.chromium.org/1843623002/diff/20001/tools/mb/mb_config.pyl#...
tools/mb/mb_config.pyl:1636: },
As I just noted on the bug, I'm not sure that we should add an 'is_headless'
flag at all, so let's see if we can figure that out.

Also, you should leave out the 'type': 'gn' flag, but add a 'gyp_defines':
'target_arch=unknown' and add a comment to indicate that it's an error to have
this mixin combined w/ the 'gyp' mixin.

[perezju, 2016-03-30 10:57:33]

Thanks for the comments. PTAL

https://codereview.chromium.org/1843623002/diff/20001/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/1843623002/diff/20001/tools/mb/mb_config.pyl#...
tools/mb/mb_config.pyl:1122: 'gn', 'headless', 'debug_bot', 'x64',
On 2016/03/29 19:25:19, Dirk Pranke wrote:
> You should not specify x64; it's the default (except for win gyp builds).

Done.

https://codereview.chromium.org/1843623002/diff/20001/tools/mb/mb_config.pyl#...
tools/mb/mb_config.pyl:1636: },
On 2016/03/29 19:25:19, Dirk Pranke wrote:
> As I just noted on the bug, I'm not sure that we should add an 'is_headless'
> flag at all, so let's see if we can figure that out.

I've left the flag in for now, but added a TODO to consider removing it.

> Also, you should leave out the 'type': 'gn' flag, but add a 'gyp_defines':
> 'target_arch=unknown' and add a comment to indicate that it's an error to have
> this mixin combined w/ the 'gyp' mixin.

Done.

[Dirk Pranke, 2016-03-31 01:50:18]

lgtm, apart from the fact that skyostil@ posted

https://codereview.chromium.org/1845473003/

and these two aren't going to place nicely together.

At the moment, there's no good way to have MB share/re-use the template in
//build/args, so you will either need to duplicate the settings (lame) or
possibly wait for me to fix this in MB (which is not hard).

https://codereview.chromium.org/1843623002/diff/40001/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/1843623002/diff/40001/tools/mb/mb_config.pyl#...
tools/mb/mb_config.pyl:1638: # It's an error to combine this with the gyp mixin.
Nit: I would probably change this comment to "# headless is GN-only".

[Sami, 2016-03-31 09:44:46]

On 2016/03/31 01:50:18, Dirk Pranke wrote:
> lgtm, apart from the fact that skyostil@ posted
> 
> https://codereview.chromium.org/1845473003/
> 
> and these two aren't going to place nicely together.

We agreed with Juan to land my patch first and then rebase this one on top of
that (as well as the MB change you mentioned).

[perezju, 2016-04-01 11:13:03]

I've added the compile_targets to this CL as per the comments in
https://codereview.chromium.org/1840063002

Will wait for Dirk's fix in MB to re-use the template in
//build/args

[Dirk Pranke, 2016-04-01 16:56:41]

I'm not sure what you mean by "Will wait for Dirk's fix in MB to re-use the
template in //build/args"; I don't actually have that fix yet, and probably will
not get to it today.

Is the bot currently broken w/o this patch, or is it running because you left
the build-side configuration alone?

https://codereview.chromium.org/1843623002/diff/60001/testing/buildbot/chromi...
File testing/buildbot/chromium.fyi.json (right):

https://codereview.chromium.org/1843623002/diff/60001/testing/buildbot/chromi...
testing/buildbot/chromium.fyi.json:6214: "test": "headless_browsertests"
this change confuses me a bit.

"override_compile_targets" is used when you need to build something other than
what the test name would suggest, i.e., you needed to actually build
"headless_libs" and "headless_tests" in order for "headless_browsertests" to
run.

But, I don't think that's the case, right?

If what you want is to build "headless_libs" and "headless_tests" *in addition
to* "headless_browsertests", put those two in a list called
"additional_compile_targets" at the top level, next to "gtest_tests".

[perezju, 2016-04-04 11:32:47]

On 2016/04/01 16:56:41, Dirk Pranke wrote:
> Is the bot currently broken w/o this patch, or is it running because you left
> the build-side configuration alone?

The bot is not running yet. We're still missing a master restart for the new
bot/slave config to be picked up. Also, as this is a new bot on a fyi waterfall,
we're also OK with the bot being broken for a few days while setting up all the
pieces.

> I'm not sure what you mean by "Will wait for Dirk's fix in MB to re-use the
> template in //build/args"; I don't actually have that fix yet, and probably
will
> not get to it today.

As you mentioned before:

> At the moment, there's no good way to have MB share/re-use the template in
> //build/args, so you will either need to duplicate the settings (lame) or
> possibly wait for me to fix this in MB (which is not hard).

So, if it's a matter of a couple of days, we'll wait for you "to fix this in
MB". Otherwise let me know, so I can follow the alternative of duplicating the
//build/args settings.

https://codereview.chromium.org/1843623002/diff/60001/testing/buildbot/chromi...
File testing/buildbot/chromium.fyi.json (right):

https://codereview.chromium.org/1843623002/diff/60001/testing/buildbot/chromi...
testing/buildbot/chromium.fyi.json:6214: "test": "headless_browsertests"
On 2016/04/01 16:56:40, Dirk Pranke wrote:
> this change confuses me a bit.
> 
> "override_compile_targets" is used when you need to build something other than
> what the test name would suggest, i.e., you needed to actually build
> "headless_libs" and "headless_tests" in order for "headless_browsertests" to
> run.
> 
> But, I don't think that's the case, right?
> 
> If what you want is to build "headless_libs" and "headless_tests" *in addition
> to* "headless_browsertests", put those two in a list called
> "additional_compile_targets" at the top level, next to "gtest_tests".

Done. But just to double-check (because I'm not very familiar with these areas),
on the recipe side the compile step seems to include "All" and
"chromium_builder_tests" targets.
https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave...

Is the change on 'additional_compile_targets' overriding that setting? Or do I
need to make another change on the recipe config for this bot?
https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave...

[Dirk Pranke, 2016-04-04 21:05:20]

On 2016/04/04 11:32:47, perezju wrote:
> On 2016/04/01 16:56:41, Dirk Pranke wrote:
> > Is the bot currently broken w/o this patch, or is it running because you
left
> > the build-side configuration alone?
> 
> The bot is not running yet. We're still missing a master restart for the new
> bot/slave config to be picked up. Also, as this is a new bot on a fyi
waterfall,
> we're also OK with the bot being broken for a few days while setting up all
the
> pieces.

Okay.

> 
> > I'm not sure what you mean by "Will wait for Dirk's fix in MB to re-use the
> > template in //build/args"; I don't actually have that fix yet, and probably
> will
> > not get to it today.
> 
> As you mentioned before:
> 
> > At the moment, there's no good way to have MB share/re-use the template in
> > //build/args, so you will either need to duplicate the settings (lame) or
> > possibly wait for me to fix this in MB (which is not hard).
> 
> So, if it's a matter of a couple of days, we'll wait for you "to fix this in
> MB". Otherwise let me know, so I can follow the alternative of duplicating the
> //build/args settings.

Yes, I should be able to get to this shortly.

> 
>
https://codereview.chromium.org/1843623002/diff/60001/testing/buildbot/chromi...
> File testing/buildbot/chromium.fyi.json (right):
> 
>
https://codereview.chromium.org/1843623002/diff/60001/testing/buildbot/chromi...
> testing/buildbot/chromium.fyi.json:6214: "test": "headless_browsertests"
> On 2016/04/01 16:56:40, Dirk Pranke wrote:
> > this change confuses me a bit.
> > 
> > "override_compile_targets" is used when you need to build something other
than
> > what the test name would suggest, i.e., you needed to actually build
> > "headless_libs" and "headless_tests" in order for "headless_browsertests" to
> > run.
> > 
> > But, I don't think that's the case, right?
> > 
> > If what you want is to build "headless_libs" and "headless_tests" *in
addition
> > to* "headless_browsertests", put those two in a list called
> > "additional_compile_targets" at the top level, next to "gtest_tests".
> 
> Done. But just to double-check (because I'm not very familiar with these
areas),
> on the recipe side the compile step seems to include "All" and
> "chromium_builder_tests" targets.
>
https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave...
> 
> Is the change on 'additional_compile_targets' overriding that setting? Or do I
> need to make another change on the recipe config for this bot?
>
https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave...

The build-side changes pick up some default targets, if none are specified, and
then
the src-side targets get added on. Plus, there's the problem in that the
*.expected/*.json
files don't actually represent reality, because when we generate the data we
don't read
the real src-side files, so you can't trust the output completely.

So, in short, I'm not 100% sure what will happen. You will either get all four
targets,
or possibly just the two source-side targets, or possibly some other
combination.

We should try it and see and, eventually, fix things so that we *just* get the
two
source-side targets if need be.

[perezju, 2016-04-05 09:30:10]

On 2016/04/04 21:05:20, Dirk Pranke wrote:
> We should try it and see and, eventually, fix things so that we *just* get the
> two
> source-side targets if need be.

OK :) sgtm

[perezju, 2016-04-07 13:19:31]

Took the chance to slip the headless_unittests in from
https://codereview.chromium.org/1866063002/

https://codereview.chromium.org/1843623002/diff/40001/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/1843623002/diff/40001/tools/mb/mb_config.pyl#...
tools/mb/mb_config.pyl:1638: # It's an error to combine this with the gyp mixin.
On 2016/03/31 01:50:18, Dirk Pranke wrote:
> Nit: I would probably change this comment to "# headless is GN-only".

Done.

[perezju, 2016-04-14 12:05:20]

Dirk, please have another look. If I understand correctly, this should be now
unblocked.

[Dirk Pranke, 2016-04-14 16:25:01]

lgtm.

I'm waffling a bit on whether it should be 'Headless_Linux_(dbg).json' to match
the case, but I think I prefer all-lower-case for now.

[perezju, 2016-04-15 08:06:48]

The CQ bit was checked by perezju@chromium.org

[commit-bot: I haz the power, 2016-04-15 08:06:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1843623002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1843623002/160001

[commit-bot: I haz the power, 2016-04-15 09:20:16]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-04-15 09:21:26]

Description was changed from

==========
Add tests for Headless Linux bot

BUG=594108
==========

to

==========
Add tests for Headless Linux bot

BUG=594108

Committed: https://crrev.com/b8b863a043b50ec783dc9cacd683700802a27130
Cr-Commit-Position: refs/heads/master@{#387563}
==========

[commit-bot: I haz the power, 2016-04-15 09:21:27]

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