Add --site-per-process flavor of //content tests to Site Isolation Android bot.

Add --site-per-process flavor of //content tests to Site Isolation Android bot.

BUG=654569
Committed: https://crrev.com/bf2f3855e7e22042ad5d11e785b13d6e7c84127a
Cr-Commit-Position: refs/heads/master@{#426336}

[Łukasz Anforowicz, 2016-10-18 20:44:24]

lukasza@chromium.org changed reviewers:
+ alexmos@chromium.org

[Łukasz Anforowicz, 2016-10-18 20:44:24]

Alex, can you take a look please?

This CL adds 1) --site-per-process mode and 2) site-per-process-specific test
filters.  If everything here works fine, we can do the similar thing for other
gtest tests already run by Site Isolation Linux (e.g. for
components_browsertests and components_unittests).

[alexmos, 2016-10-18 23:36:10]

Thanks!  This looks good, but one question about build config below.

https://codereview.chromium.org/2431793002/diff/1/testing/buildbot/chromium.f...
File testing/buildbot/chromium.fyi.json (right):

https://codereview.chromium.org/2431793002/diff/1/testing/buildbot/chromium.f...
testing/buildbot/chromium.fyi.json:11393:
"--gtest-filter-file=../../testing/buildbot/filters/site-per-process.content_browsertests.filter",
Will this be properly picked up by swarming?  Looking at content/test/BUILD.gn,
I see this:
     if (is_win || is_linux) {
        data += [
"//testing/buildbot/filters/site-per-process.content_browsertests.filter" ]
      }

This seems like it needs an is_android check as well now.

https://codereview.chromium.org/2431793002/diff/1/testing/buildbot/chromium.f...
testing/buildbot/chromium.fyi.json:11396: "name":
"site_per_process_content_browsertests",
Why do we want to run the same sets of tests with and without
--site-per-process?  Is it to distinguish from the set of tests that have been
flaky on this bot even without --site-per-process?

https://codereview.chromium.org/2431793002/diff/1/testing/buildbot/chromium.f...
testing/buildbot/chromium.fyi.json:11425: "--test-arguments=--site-per-process"
Glad we don't have a filter file for unit tests, and fingers crossed that
Android won't break any additional ones. :)

[Łukasz Anforowicz, 2016-10-19 00:06:39]

Can you take another look please?

https://codereview.chromium.org/2431793002/diff/1/testing/buildbot/chromium.f...
File testing/buildbot/chromium.fyi.json (right):

https://codereview.chromium.org/2431793002/diff/1/testing/buildbot/chromium.f...
testing/buildbot/chromium.fyi.json:11393:
"--gtest-filter-file=../../testing/buildbot/filters/site-per-process.content_browsertests.filter",
On 2016/10/18 23:36:10, alexmos wrote:
> Will this be properly picked up by swarming?  Looking at
content/test/BUILD.gn,
> I see this:
>      if (is_win || is_linux) {
>         data += [
> "//testing/buildbot/filters/site-per-process.content_browsertests.filter" ]
>       }
> 
> This seems like it needs an is_android check as well now.

Thanks for pointing this out.  Done.  For a moment I wondered whether to simply
pull in the filter file on all architectures, but then I decided to just add
|is_android|, because we seem to be really granular with other things in this
BUILD.gn file.

https://codereview.chromium.org/2431793002/diff/1/testing/buildbot/chromium.f...
testing/buildbot/chromium.fyi.json:11396: "name":
"site_per_process_content_browsertests",
On 2016/10/18 23:36:10, alexmos wrote:
> Why do we want to run the same sets of tests with and without
> --site-per-process?  Is it to distinguish from the set of tests that have been
> flaky on this bot even without --site-per-process?

Yes - exactly.  I'd like to do this for a while, especially given that we've
seen the bot getting red from time to time without --site-per-process.  We can
reevaluate if this impacts the runtimes too much.

Also - this way things are similar to how linux tests are setup on the CQ
(including the step names like "site_per_process_content_browsertests").

https://codereview.chromium.org/2431793002/diff/1/testing/buildbot/chromium.f...
testing/buildbot/chromium.fyi.json:11425: "--test-arguments=--site-per-process"
On 2016/10/18 23:36:10, alexmos wrote:
> Glad we don't have a filter file for unit tests, and fingers crossed that
> Android won't break any additional ones. :)

Yes.  I was wondering whether to include an Android-specific filter file (at
least for content_browsertests) in the CL under review...  I think - let's see
first if there are any Android-specific failures and maybe we'll get lucky (and
maybe what I saw in https://crbug.com/654569#c4 was just transient failures or
failures specific to my local test environment).

[alexmos, 2016-10-19 00:40:47]

Thanks, LGTM.

[Łukasz Anforowicz, 2016-10-19 15:53:48]

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

[Łukasz Anforowicz, 2016-10-19 15:53:49]

Thanks Alex.  Dirk, can you take a look please?

[Dirk Pranke, 2016-10-19 16:41:20]

lgtm

[Łukasz Anforowicz, 2016-10-19 17:07:26]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-10-19 17:07:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 17:43:45]

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

[commit-bot: I haz the power, 2016-10-19 17:43:46]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Łukasz Anforowicz, 2016-10-19 18:03:57]

Thanks.  sky@, could you PTAL at content/test/BUILD.gn?

[Łukasz Anforowicz, 2016-10-19 18:04:20]

lukasza@chromium.org changed reviewers:
+ sky@chromium.org

[Łukasz Anforowicz, 2016-10-19 18:04:20]

sky@, could you PTAL at content/test/BUILD.gn?

[sky, 2016-10-19 19:34:52]

LGTM - but did you verify these tests aren't flakey on android and the extra
load can be handled?

[Łukasz Anforowicz, 2016-10-19 19:56:28]

On 2016/10/19 19:34:52, sky wrote:
> LGTM - but did you verify these tests aren't flakey on android and the extra
> load can be handled?

I haven't verify if the tests are flakey on android.

I have not looped in  infra-dev@chromium.org for capacity and load planning.

I thought that both of the things above are not needed, because the changes here
are not affecting CQ - they are only affecting a new FYI bot for Site Isolation
project.  Please shout if you think this is wrong and I should take some extra
action here.

[sky, 2016-10-19 23:03:05]

My mistake, you are right.

[Łukasz Anforowicz, 2016-10-19 23:14:19]

On 2016/10/19 23:03:05, sky wrote:
> My mistake, you are right.

Thanks for double-checking.  Better safe than sorry :-).

[Łukasz Anforowicz, 2016-10-19 23:14:25]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-10-19 23:15:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 23:35:39]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-21 13:13:16]

Description was changed from

==========
Add --site-per-process flavor of //content tests to Site Isolation Android bot.

BUG=654569
==========

to

==========
Add --site-per-process flavor of //content tests to Site Isolation Android bot.

BUG=654569

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

[commit-bot: I haz the power, 2016-10-21 13:13:17]

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