Enable components_browsertests on Android

Enable components_browsertests on Android

BUG=465351
Committed: https://crrev.com/4ba34ff136727244ead91780152b3248fe7ce0fd
Cr-Commit-Position: refs/heads/master@{#326449}

[Jaekyun Seok (inactive), 2015-04-10 02:12:28]

jaekyun@chromium.org changed reviewers:
+ blundell@chromium.org, caitkp@chromium.org

[Jaekyun Seok (inactive), 2015-04-10 02:12:28]

blundell@ and caitkp@, please review changes of components/*.

[Jaekyun Seok (inactive), 2015-04-10 02:14:02]

jaekyun@chromium.org changed reviewers:
+ jbudorick@chromium.org, klundberg@chromium.org

[Jaekyun Seok (inactive), 2015-04-10 02:14:02]

klundberg@ and jbudorick@, please review changes of build/android/*.

[blundell, 2015-04-10 08:24:20]

Is there someone with Android expertise who could do the initial review here and
then once they've LGTM'd it Cait or I could stamp it for OWNERS?

[Jaekyun Seok (inactive), 2015-04-10 10:06:27]

jaekyun@chromium.org changed reviewers:
+ sievers@chromium.org, tedchoc@chromium.org

[Jaekyun Seok (inactive), 2015-04-10 10:06:28]

tedchoc@ and sievers@, could you please review changes of components/* as
experts of Android?

[jbudorick, 2015-04-10 13:34:32]

https://codereview.chromium.org/1075783002/diff/40001/build/android/pylib/gte...
File build/android/pylib/gtest/gtest_config.py (right):

https://codereview.chromium.org/1075783002/diff/40001/build/android/pylib/gte...
build/android/pylib/gtest/gtest_config.py:26: 'components_browsertests',
I'm not sure which bots, if any, are still using this list to determine which
tests they should run, but as it could be > 0, this should be in
EXPERIMENTAL_TEST_SUITES for now.

https://codereview.chromium.org/1075783002/diff/40001/components/components_b...
File components/components_browsertests.isolate (right):

https://codereview.chromium.org/1075783002/diff/40001/components/components_b...
components/components_browsertests.isolate:58: '../testing/test_env.py',
Is this really necessary on Android?

https://codereview.chromium.org/1075783002/diff/40001/components/dom_distille...
File components/dom_distiller/content/distillable_page_utils_browsertest.cc
(right):

https://codereview.chromium.org/1075783002/diff/40001/components/dom_distille...
components/dom_distiller/content/distillable_page_utils_browsertest.cc:52:
base::FilePath pak_file;
Would it be better to just have the Android-specific parts get pak_dir, then
have the pak_file stuff be platform-independent?

https://codereview.chromium.org/1075783002/diff/40001/components/dom_distille...
File components/dom_distiller/content/distiller_page_web_contents_browsertest.cc
(right):

https://codereview.chromium.org/1075783002/diff/40001/components/dom_distille...
components/dom_distiller/content/distiller_page_web_contents_browsertest.cc:65:
#if defined(OS_ANDROID)
ditto

https://codereview.chromium.org/1075783002/diff/40001/components/test/android...
File
components/test/android/browsertests_apk/components_browser_tests_android.cc
(right):

https://codereview.chromium.org/1075783002/diff/40001/components/test/android...
components/test/android/browsertests_apk/components_browser_tests_android.cc:48:
// TODO(nileshagrawal): Refactor and deduplicate with
I really don't like adding a third (!) native test APK wrapper.

[Ted C, 2015-04-10 16:42:43]

This does seem like an unfortunate amount of duplicate/boiler plate code as John
pointed out.

https://codereview.chromium.org/1075783002/diff/40001/components/test/android...
File
components/test/android/browsertests_apk/components_browser_tests_android.cc
(right):

https://codereview.chromium.org/1075783002/diff/40001/components/test/android...
components/test/android/browsertests_apk/components_browser_tests_android.cc:48:
// TODO(nileshagrawal): Refactor and deduplicate with
On 2015/04/10 13:34:32, jbudorick wrote:
> I really don't like adding a third (!) native test APK wrapper.

FWIW, this code looks easily shareable to be as all the params could be passed
in from java.

But I don't know if that is what you're disliking in this context.

https://codereview.chromium.org/1075783002/diff/40001/components/test/android...
components/test/android/browsertests_apk/components_browser_tests_android.cc:81:
"content_browsertests_devtools_remote");
Does this need to be components_browsertests_...?

https://codereview.chromium.org/1075783002/diff/40001/components/test/android...
File components/test/android/browsertests_apk/res/layout/test_activity.xml
(right):

https://codereview.chromium.org/1075783002/diff/40001/components/test/android...
components/test/android/browsertests_apk/res/layout/test_activity.xml:3: <!--
Copyright (c) 2015 The Chromium Authors. All rights reserved.
don't need the (c) here

[Jaekyun Seok (inactive), 2015-04-13 01:36:02]

PTAL.

I filed http://crbug.com/476410 to refactor codes later to use a single native
test APK wrapper.

https://codereview.chromium.org/1075783002/diff/40001/build/android/pylib/gte...
File build/android/pylib/gtest/gtest_config.py (right):

https://codereview.chromium.org/1075783002/diff/40001/build/android/pylib/gte...
build/android/pylib/gtest/gtest_config.py:26: 'components_browsertests',
On 2015/04/10 13:34:31, jbudorick wrote:
> I'm not sure which bots, if any, are still using this list to determine which
> tests they should run, but as it could be > 0, this should be in
> EXPERIMENTAL_TEST_SUITES for now.

Done.

https://codereview.chromium.org/1075783002/diff/40001/components/components_b...
File components/components_browsertests.isolate (right):

https://codereview.chromium.org/1075783002/diff/40001/components/components_b...
components/components_browsertests.isolate:58: '../testing/test_env.py',
On 2015/04/10 13:34:31, jbudorick wrote:
> Is this really necessary on Android?

I moved this the above block.

https://codereview.chromium.org/1075783002/diff/40001/components/dom_distille...
File components/dom_distiller/content/distillable_page_utils_browsertest.cc
(right):

https://codereview.chromium.org/1075783002/diff/40001/components/dom_distille...
components/dom_distiller/content/distillable_page_utils_browsertest.cc:52:
base::FilePath pak_file;
On 2015/04/10 13:34:31, jbudorick wrote:
> Would it be better to just have the Android-specific parts get pak_dir, then
> have the pak_file stuff be platform-independent?

Done.

https://codereview.chromium.org/1075783002/diff/40001/components/dom_distille...
File components/dom_distiller/content/distiller_page_web_contents_browsertest.cc
(right):

https://codereview.chromium.org/1075783002/diff/40001/components/dom_distille...
components/dom_distiller/content/distiller_page_web_contents_browsertest.cc:65:
#if defined(OS_ANDROID)
On 2015/04/10 13:34:32, jbudorick wrote:
> ditto

Done.

https://codereview.chromium.org/1075783002/diff/40001/components/test/android...
File
components/test/android/browsertests_apk/components_browser_tests_android.cc
(right):

https://codereview.chromium.org/1075783002/diff/40001/components/test/android...
components/test/android/browsertests_apk/components_browser_tests_android.cc:48:
// TODO(nileshagrawal): Refactor and deduplicate with
On 2015/04/10 16:42:43, Ted C (OOO 4.13 - 4.21) wrote:
> On 2015/04/10 13:34:32, jbudorick wrote:
> > I really don't like adding a third (!) native test APK wrapper.
> 
> FWIW, this code looks easily shareable to be as all the params could be passed
> in from java.
> 
> But I don't know if that is what you're disliking in this context.

I filed http://crbug/476410 to follow up this later.

https://codereview.chromium.org/1075783002/diff/40001/components/test/android...
components/test/android/browsertests_apk/components_browser_tests_android.cc:81:
"content_browsertests_devtools_remote");
On 2015/04/10 16:42:43, Ted C (OOO 4.13 - 4.21) wrote:
> Does this need to be components_browsertests_...?

Done.

https://codereview.chromium.org/1075783002/diff/40001/components/test/android...
File components/test/android/browsertests_apk/res/layout/test_activity.xml
(right):

https://codereview.chromium.org/1075783002/diff/40001/components/test/android...
components/test/android/browsertests_apk/res/layout/test_activity.xml:3: <!--
Copyright (c) 2015 The Chromium Authors. All rights reserved.
On 2015/04/10 16:42:43, Ted C (OOO 4.13 - 4.21) wrote:
> don't need the (c) here

Done.

[Jaekyun Seok (inactive), 2015-04-14 21:53:15]

sievers@, could you please review changes of components/* as experts of Android
because tedchoc@ is OOO until next week?

[jbudorick, 2015-04-14 22:31:50]

How long do these take to run locally?

[Jaekyun Seok (inactive), 2015-04-14 23:22:01]

On 2015/04/14 22:31:50, jbudorick wrote:
> How long do these take to run locally?

It took 147.768s for 21 testcases to run with my test nexus 7 device.

[no sievers, 2015-04-15 18:32:54]

Ted would be a better reviewer, but looks fine with a few nits.

https://codereview.chromium.org/1075783002/diff/80001/build/android/pylib/gte...
File build/android/pylib/gtest/filter/components_browsertests_disabled (right):

https://codereview.chromium.org/1075783002/diff/80001/build/android/pylib/gte...
build/android/pylib/gtest/filter/components_browsertests_disabled:1: #
http://crbug.com/187500: extends RenderViewTest
We should really get rid of these '*_disabled' files.
The correct mechanism is to disable failing tests in the source code.

https://codereview.chromium.org/1075783002/diff/80001/components/test/android...
File
components/test/android/browsertests_apk/components_browser_tests_android.cc
(right):

https://codereview.chromium.org/1075783002/diff/80001/components/test/android...
components/test/android/browsertests_apk/components_browser_tests_android.cc:1:
// Copyright (c) 2015 The Chromium Authors. All rights reserved.
nit: drop '(c)' here and elsewhere

https://codereview.chromium.org/1075783002/diff/80001/components/test/android...
File components/test/android/browsertests_apk/components_browser_tests_android.h
(right):

https://codereview.chromium.org/1075783002/diff/80001/components/test/android...
components/test/android/browsertests_apk/components_browser_tests_android.h:14:
}  // namespace content
nit: s/content/components

[Jaekyun Seok (inactive), 2015-04-15 23:04:37]

blundell@ and caitkp@, could you please take a look at this CL now as owners of
components/* because sievers@ said that this CL looked fine as an expert of
Android?

https://codereview.chromium.org/1075783002/diff/80001/build/android/pylib/gte...
File build/android/pylib/gtest/filter/components_browsertests_disabled (right):

https://codereview.chromium.org/1075783002/diff/80001/build/android/pylib/gte...
build/android/pylib/gtest/filter/components_browsertests_disabled:1: #
http://crbug.com/187500: extends RenderViewTest
On 2015/04/15 18:32:53, sievers wrote:
> We should really get rid of these '*_disabled' files.
> The correct mechanism is to disable failing tests in the source code.

Done.

https://codereview.chromium.org/1075783002/diff/80001/components/test/android...
File
components/test/android/browsertests_apk/components_browser_tests_android.cc
(right):

https://codereview.chromium.org/1075783002/diff/80001/components/test/android...
components/test/android/browsertests_apk/components_browser_tests_android.cc:1:
// Copyright (c) 2015 The Chromium Authors. All rights reserved.
On 2015/04/15 18:32:53, sievers wrote:
> nit: drop '(c)' here and elsewhere

Done.

https://codereview.chromium.org/1075783002/diff/80001/components/test/android...
File components/test/android/browsertests_apk/components_browser_tests_android.h
(right):

https://codereview.chromium.org/1075783002/diff/80001/components/test/android...
components/test/android/browsertests_apk/components_browser_tests_android.h:14:
}  // namespace content
On 2015/04/15 18:32:53, sievers wrote:
> nit: s/content/components

Done.

[blundell, 2015-04-16 09:32:06]

rubberstamp-ish LGTM

you should add OWNERS of //components/test/android

[Jaekyun Seok (inactive), 2015-04-16 10:11:42]

On 2015/04/16 09:32:06, blundell wrote:
> rubberstamp-ish LGTM
> 
> you should add OWNERS of //components/test/android

Done.

[Jaekyun Seok (inactive), 2015-04-16 10:15:17]

jbudorick@, could you please take a look at changes of build/android/* again?

[jbudorick, 2015-04-16 13:50:24]

build/android/ lgtm

[Jaekyun Seok (inactive), 2015-04-16 20:06:00]

jaekyun@chromium.org changed reviewers:
+ cjhopman@chromium.org, thakis@chromium.org

[Jaekyun Seok (inactive), 2015-04-16 20:06:01]

cjhopman@ and thakis@, please review changes of build/all.gyp.

[Nico, 2015-04-16 21:18:11]

build/all.gyp lgtm (is there a gn version of all.gyp? should this be added there
too?)

[Nico, 2015-04-16 21:19:12]

also, does this have to be added to the json files in  testing/buildbot ?

[cjhopman, 2015-04-16 21:26:38]

lgtm

[Jaekyun Seok (inactive), 2015-04-16 22:44:04]

On 2015/04/16 21:19:12, Nico (afk until Mon Apr 20) wrote:
> is there a gn version of all.gyp? should this be added there
too?

There doesn't seem such a file for GN build.

> also, does this have to be added to the json files in  testing/buildbot ?

That will be done later after some stability checkup.

[Nico, 2015-04-16 22:46:51]

OK!
On Apr 16, 2015 3:44 PM, <jaekyun@chromium.org> wrote:

> On 2015/04/16 21:19:12, Nico (afk until Mon Apr 20) wrote:
>
>> is there a gn version of all.gyp? should this be added there
>>
> too?
>
> There doesn't seem such a file for GN build.
>
>  also, does this have to be added to the json files in  testing/buildbot ?
>>
>
> That will be done later after some stability checkup.
>
>
> https://codereview.chromium.org/1075783002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jaekyun Seok (inactive), 2015-04-16 23:38:18]

jaekyun@chromium.org changed reviewers:
+ xhwang@chromium.org

[Jaekyun Seok (inactive), 2015-04-16 23:38:19]

xhwang@, could you please review changes on components/test/DEPS as an owner of
media/* because I added media/base/media_switches.h in the file?

[Jaekyun Seok (inactive), 2015-04-16 23:40:33]

sievers@, could you please review changes on components/test/DEPS again as an
owner of content/* because I added some header files of content/ in the file?

[xhwang, 2015-04-16 23:49:29]

media dependency lgtm

[no sievers, 2015-04-17 17:28:23]

On 2015/04/16 23:40:33, Jaekyun Seok wrote:
> sievers@, could you please review changes on components/test/DEPS again as an
> owner of content/* because I added some header files of content/ in the file?

You should have a components owner review the DEPS changes under components/ :)

[Jaekyun Seok (inactive), 2015-04-19 20:59:16]

On 2015/04/17 17:28:23, sievers wrote:
> On 2015/04/16 23:40:33, Jaekyun Seok wrote:
> > sievers@, could you please review changes on components/test/DEPS again as
an
> > owner of content/* because I added some header files of content/ in the
file?
> 
> You should have a components owner review the DEPS changes under components/
:)

blundell@ has given lgtm already as an owner of components/.

The latest error messages are like the followings.

** Presubmit ERRORS **
Missing LGTM from OWNERS of dependencies added to DEPS:
    '+content/public/app/content_jni_onload.h',
    '+content/public/app/content_main.h',
    '+content/public/common/content_switches.h',
    '+content/shell/android/shell_jni_registrar.h',
    '+content/shell/app/shell_main_delegate.h',

So I need lgtm from an owner of content/.

[Jaekyun Seok (inactive), 2015-04-21 02:25:31]

sievers, please give "lgtm" to this CL. You gave "looks fine" already, but
"lgtm" is needed to pass the presubmit test.

[Yaron, 2015-04-21 14:31:35]

On 2015/04/21 02:25:31, Jaekyun Seok wrote:
> sievers, please give "lgtm" to this CL. You gave "looks fine" already, but
> "lgtm" is needed to pass the presubmit test.

Happened to see this through watchlist. The content/ DEP lgtm given
crbug.com/476410 to clean-up

[Yaron, 2015-04-21 14:32:18]

On 2015/04/21 14:31:35, Yaron wrote:
> On 2015/04/21 02:25:31, Jaekyun Seok wrote:
> > sievers, please give "lgtm" to this CL. You gave "looks fine" already, but
> > "lgtm" is needed to pass the presubmit test.
> 
> Happened to see this through watchlist. The content/ DEP lgtm given
> crbug.com/476410 to clean-up

(at least I think I have the necessary owners)

[Jaekyun Seok (inactive), 2015-04-21 21:05:40]

On 2015/04/21 14:31:35, Yaron wrote:
> On 2015/04/21 02:25:31, Jaekyun Seok wrote:
> > sievers, please give "lgtm" to this CL. You gave "looks fine" already, but
> > "lgtm" is needed to pass the presubmit test.
> 
> Happened to see this through watchlist. The content/ DEP lgtm given
> crbug.com/476410 to clean-up

Thanks for reviewing this CL!

[Jaekyun Seok (inactive), 2015-04-21 21:05:50]

The CQ bit was checked by jaekyun@chromium.org

[Jaekyun Seok (inactive), 2015-04-21 21:05:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1075783002/#ps120001
(title: "Add components/test/android/OWNERS")

[commit-bot: I haz the power, 2015-04-21 21:06:46]

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

[commit-bot: I haz the power, 2015-04-21 21:12:50]

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

[commit-bot: I haz the power, 2015-04-21 21:12:51]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Jaekyun Seok (inactive), 2015-04-21 22:01:57]

On 2015/04/21 21:12:51, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
>   ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

Rebased to fix failures, but this CL still needs LGTM from an owner of content/
like the followings.

** Presubmit ERRORS **
Missing LGTM from OWNERS of dependencies added to DEPS:
    '+content/public/app/content_jni_onload.h',
    '+content/public/app/content_main.h',
    '+content/public/common/content_switches.h',
    '+content/shell/app/shell_main_delegate.h',

[no sievers, 2015-04-22 19:46:00]

On 2015/04/21 22:01:57, Jaekyun Seok wrote:
> On 2015/04/21 21:12:51, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> >   ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
> >   ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
> 
> Rebased to fix failures, but this CL still needs LGTM from an owner of
content/
> like the followings.
> 
> ** Presubmit ERRORS **
> Missing LGTM from OWNERS of dependencies added to DEPS:
>     '+content/public/app/content_jni_onload.h',
>     '+content/public/app/content_main.h',
>     '+content/public/common/content_switches.h',
>     '+content/shell/app/shell_main_delegate.h',

lgtm

[Jaekyun Seok (inactive), 2015-04-22 20:52:26]

The CQ bit was checked by jaekyun@chromium.org

[Jaekyun Seok (inactive), 2015-04-22 20:52:26]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
jbudorick@chromium.org, cjhopman@chromium.org, yfriedman@chromium.org,
thakis@chromium.org, blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1075783002/#ps140001
(title: "Rebased")

[commit-bot: I haz the power, 2015-04-22 20:56:31]

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

[Jaekyun Seok (inactive), 2015-04-22 21:22:42]

The CQ bit was unchecked by jaekyun@chromium.org

[Jaekyun Seok (inactive), 2015-04-22 21:30:07]

The CQ bit was checked by jaekyun@chromium.org

[Jaekyun Seok (inactive), 2015-04-22 21:30:08]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
jbudorick@chromium.org, cjhopman@chromium.org, yfriedman@chromium.org,
sievers@chromium.org, thakis@chromium.org, blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1075783002/#ps160001
(title: "Fix build error on Mac")

[commit-bot: I haz the power, 2015-04-22 21:33:17]

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

[Jaekyun Seok (inactive), 2015-04-22 21:46:05]

The CQ bit was unchecked by jaekyun@chromium.org

[Jaekyun Seok (inactive), 2015-04-22 22:34:17]

On 2015/04/22 21:33:17, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1075783002/160001

Sorry for uploads after the last review. There was missing declaration of
MAYBE_PrintWebViewHelperPreviewTest in
components/printing/renderer/print_web_view_helper.h.

But I made typos on Patch Set 9, and so I uploaded Patch Set 10 again.

I will send this to CQ later after some more tests.

[Jaekyun Seok (inactive), 2015-04-23 03:16:25]

The CQ bit was checked by jaekyun@chromium.org

[Jaekyun Seok (inactive), 2015-04-23 03:16:26]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
jbudorick@chromium.org, cjhopman@chromium.org, yfriedman@chromium.org,
sievers@chromium.org, thakis@chromium.org, blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1075783002/#ps200001
(title: "Fix GN build and add java dependency to components/test/DEPS")

[commit-bot: I haz the power, 2015-04-23 03:17:14]

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

[commit-bot: I haz the power, 2015-04-23 04:22:59]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-04-23 04:23:40]

Patchset 11 (id:??) landed as
https://crrev.com/4ba34ff136727244ead91780152b3248fe7ce0fd
Cr-Commit-Position: refs/heads/master@{#326449}