apk-based test runner work. Not enabled yet. This CL is a combination of upstreaming, ndk/ant-ifi…

apk-based test runner work.  Not enabled yet.  This CL is a combination of upstreaming, ndk/ant-ification, and other tweaks.


BUG=None
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=128679

[John Grabowski, 2012-03-23 01:33:45]

jam: for ipc.gyp change (OWNERS)
mark: for base.gyp change (OWNERS)

bulach: for everything else.

Not enabled by default; not complete enough to do so.  (Can't run tests yet.) To
turn this on, do 
GYP_DEFINES="$GYP_DEFINES gtest_target_type=shared_library" android_gyp 
then 'make' will build ipc and base as solibs, and the apk will be built.

Requires ant1.8 which will be installed on the bots before landing.

[bulach, 2012-03-23 10:30:48]

lgtm

thanks john! a few nits below, looking forward to have these tests up and
running!

http://chromiumcodereview.appspot.com/9834037/diff/1/base/base.gyp
File base/base.gyp (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/base/base.gyp#newcode12
base/base.gyp:12: 'test/android/native_test.gyp'
is it possible to make this include conditional?

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/java/o...
File base/test/android/java/org/chromium/ChromeNativeTestActivity.java (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/java/o...
base/test/android/java/org/chromium/ChromeNativeTestActivity.java:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
nit: 2012

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
File base/test/android/native_test.gyp (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test.gyp:11: 'message': 'building native test runner',
nit: when I think about "runner", the host side python infrastructure comes to
mind.. maybe "Building native test package" ?

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
File base/test/android/native_test_launcher.cc (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test_launcher.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit:2012

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test_launcher.cc:66: virtual void
OnTestProgramStart(const ::testing::UnitTest& unit_test);
nit: while at it, OVERRIDE on all these methods..

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test_launcher.cc:86: unit_test.test_to_run_count());
nit: unindent

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test_launcher.cc:92: test_info.test_case_name(),
test_info.name());
nit: unindent

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test_launcher.cc:102: test_part_result.summary());
nit: indent 99-102

http://chromiumcodereview.appspot.com/9834037/diff/1/build/android/single_tes...
File build/android/single_test_runner.py (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/build/android/single_tes...
build/android/single_test_runner.py:57: 'local')
55-56 seem to be unused?

http://chromiumcodereview.appspot.com/9834037/diff/1/build/android/test_packa...
File build/android/test_package_apk.py (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/build/android/test_packa...
build/android/test_package_apk.py:2: # Copyright (c) 2011 The Chromium Authors.
All rights reserved.
nit:2012

http://chromiumcodereview.appspot.com/9834037/diff/1/build/android/test_packa...
build/android/test_package_apk.py:77:
'org.chromium.native_test.ChromeNativeTestActivity')
nit: >80cols

[Peter Beverloo, 2012-03-23 11:15:12]

+tedchoc and jcivelli, also some nits.

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/Androi...
File base/test/android/AndroidManifest.xml (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/Androi...
base/test/android/AndroidManifest.xml:8: <uses-sdk android:minSdkVersion="8" />
That'd be Android 2.2. Let's lock this to ICS to be consistent, thus use a
minimum of 14.

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
File base/test/android/native_test_apk.xml (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test_apk.xml:12: <property name="target"
value="android-8"/>
Does this correspond to the used platform? That'd be inconsistent with
"sdk.version" above..

http://chromiumcodereview.appspot.com/9834037/diff/1/build/all_android.gyp
File build/all_android.gyp (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/build/all_android.gyp#ne...
build/all_android.gyp:50: }]],
Should this be experimental for now?

[Mark Mentovai, 2012-03-23 15:00:16]

LGTM. I only reviewed base.gyp and native_test.gyp.

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
File base/test/android/native_test.gyp (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test.gyp:7: ['OS=="android"', {
Because a condition has a [ and { on the same line, I normally just close them
on the same line too, like
    ]},

That’ll probably clear up the stuff I bring up below about indentation.

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test.gyp:38: }
Something’s weird with the indentation here

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test.gyp:40: ]
and here

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test.gyp:42: 
Blank line at EOF.

[jam, 2012-03-23 17:17:51]

ipc lgtm

[John Grabowski, 2012-03-24 01:59:18]

Thx for feedback

http://chromiumcodereview.appspot.com/9834037/diff/1/base/base.gyp
File base/base.gyp (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/base/base.gyp#newcode12
base/base.gyp:12: 'test/android/native_test.gyp'
On 2012/03/23 10:30:48, bulach wrote:
> is it possible to make this include conditional?

That isn't necessary and isn't the standard, just as win_precompile.gypi is not
conditionally included on Windows.

Note, however, that targets present in that file *are* conditional on Android.

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/Androi...
File base/test/android/AndroidManifest.xml (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/Androi...
base/test/android/AndroidManifest.xml:8: <uses-sdk android:minSdkVersion="8" />
On 2012/03/23 11:15:13, Peter Beverloo wrote:
> That'd be Android 2.2. Let's lock this to ICS to be consistent, thus use a
> minimum of 14.

Technically it only needs 8 to run but ok

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/java/o...
File base/test/android/java/org/chromium/ChromeNativeTestActivity.java (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/java/o...
base/test/android/java/org/chromium/ChromeNativeTestActivity.java:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
On 2012/03/23 10:30:48, bulach wrote:
> nit: 2012

Technically this was written in 2011... but ok.

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
File base/test/android/native_test.gyp (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test.gyp:7: ['OS=="android"', {
On 2012/03/23 15:00:16, Mark Mentovai wrote:
> Because a condition has a [ and { on the same line, I normally just close them
> on the same line too, like
>     ]},
> 
> That’ll probably clear up the stuff I bring up below about indentation.

Done (2 cases)
Hopefully this works for you

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test.gyp:11: 'message': 'building native test runner',
On 2012/03/23 10:30:48, bulach wrote:
> nit: when I think about "runner", the host side python infrastructure comes to
> mind.. maybe "Building native test package" ?

Done.

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
File base/test/android/native_test_apk.xml (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test_apk.xml:12: <property name="target"
value="android-8"/>
On 2012/03/23 11:15:13, Peter Beverloo wrote:
> Does this correspond to the used platform? That'd be inconsistent with
> "sdk.version" above..

sdk.version removed
target upped to -14

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
File base/test/android/native_test_launcher.cc (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test_launcher.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2012/03/23 10:30:48, bulach wrote:
> nit:2012

Done.

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test_launcher.cc:66: virtual void
OnTestProgramStart(const ::testing::UnitTest& unit_test);
On 2012/03/23 10:30:48, bulach wrote:
> nit: while at it, OVERRIDE on all these methods..

Done.

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test_launcher.cc:86: unit_test.test_to_run_count());
On 2012/03/23 10:30:48, bulach wrote:
> nit: unindent

I think you mean "indent more to line up with open-paren"

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test_launcher.cc:92: test_info.test_case_name(),
test_info.name());
On 2012/03/23 10:30:48, bulach wrote:
> nit: unindent

I think you mean "indent more to line up with open-paren"

http://chromiumcodereview.appspot.com/9834037/diff/1/base/test/android/native...
base/test/android/native_test_launcher.cc:102: test_part_result.summary());
On 2012/03/23 10:30:48, bulach wrote:
> nit: indent 99-102

Done.

http://chromiumcodereview.appspot.com/9834037/diff/1/build/all_android.gyp
File build/all_android.gyp (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/build/all_android.gyp#ne...
build/all_android.gyp:50: }]],
On 2012/03/23 11:15:13, Peter Beverloo wrote:
> Should this be experimental for now?

Yes and no.  Right now, is only on if a special gyp_define is used.
One of the next steps is to fix FYI bot to compile both ways.
In that case yes it is experimental but no not an experimental target.
I can't change FYI bot over to the new mechanism until it is rock solid.

http://chromiumcodereview.appspot.com/9834037/diff/1/build/android/single_tes...
File build/android/single_test_runner.py (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/build/android/single_tes...
build/android/single_test_runner.py:57: 'local')
On 2012/03/23 10:30:48, bulach wrote:
> 55-56 seem to be unused?

That is true.  I left them here to remind me to make sure symbols work from a
crash.

http://chromiumcodereview.appspot.com/9834037/diff/1/build/android/test_packa...
File build/android/test_package_apk.py (right):

http://chromiumcodereview.appspot.com/9834037/diff/1/build/android/test_packa...
build/android/test_package_apk.py:2: # Copyright (c) 2011 The Chromium Authors.
All rights reserved.
On 2012/03/23 10:30:48, bulach wrote:
> nit:2012

Done.

http://chromiumcodereview.appspot.com/9834037/diff/1/build/android/test_packa...
build/android/test_package_apk.py:77:
'org.chromium.native_test.ChromeNativeTestActivity')
On 2012/03/23 10:30:48, bulach wrote:
> nit: >80cols

Done.

[commit-bot: I haz the power, 2012-03-24 01:59:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jrg@chromium.org/9834037/4003

[commit-bot: I haz the power, 2012-03-24 03:57:38]

Change committed as 128679

[Satish, 2012-03-27 13:53:24]

Some drive-by comments..

http://codereview.chromium.org/9834037/diff/4003/base/base.gyp
File base/base.gyp (right):

http://codereview.chromium.org/9834037/diff/4003/base/base.gyp#newcode12
base/base.gyp:12: 'test/android/native_test.gyp'
should this file have a ".gypi" extension?

http://codereview.chromium.org/9834037/diff/4003/base/test/android/AndroidMan...
File base/test/android/AndroidManifest.xml (right):

http://codereview.chromium.org/9834037/diff/4003/base/test/android/AndroidMan...
base/test/android/AndroidManifest.xml:7: <!-- This is the platform API where
NativeActivity was introduced. -->
In ChromeNativeTestActivity.java we don't seem to be using NativeActivity and
have a comment indicating it won't be used in future. Should this comment be
removed?

http://codereview.chromium.org/9834037/diff/4003/base/test/android/java/org/c...
File base/test/android/java/org/chromium/ChromeNativeTestActivity.java (right):

http://codereview.chromium.org/9834037/diff/4003/base/test/android/java/org/c...
base/test/android/java/org/chromium/ChromeNativeTestActivity.java:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
could change year to 2012

http://codereview.chromium.org/9834037/diff/4003/base/test/android/java/org/c...
base/test/android/java/org/chromium/ChromeNativeTestActivity.java:5: package
org.chromium.native_test;
should this file be under a 'org/chromium/native_test' directory instead of
'org/chromium'?

[klobag.chromium, 2012-03-28 00:28:56]

Drive by review.

http://codereview.chromium.org/9834037/diff/4003/base/test/android/AndroidMan...
File base/test/android/AndroidManifest.xml (right):

http://codereview.chromium.org/9834037/diff/4003/base/test/android/AndroidMan...
base/test/android/AndroidManifest.xml:1: <?xml version="1.0" encoding="utf-8"?>
Need copyright.

http://codereview.chromium.org/9834037/diff/4003/base/test/android/AndroidMan...
base/test/android/AndroidManifest.xml:3: package="org.chromium.native_test"
should this be org.chromium.base.native_test? In Android, package name is
unique. So we can't run base unittest together with net unittest.

http://codereview.chromium.org/9834037/diff/4003/base/test/android/AndroidMan...
base/test/android/AndroidManifest.xml:27: <uses-permission
android:name="android.permission.VIBRATE"/>
Why we need VIBRATE for the base unit test?

Should we review all this permissions?

http://codereview.chromium.org/9834037/diff/4003/base/test/android/java/org/c...
File base/test/android/java/org/chromium/ChromeNativeTestActivity.java (right):

http://codereview.chromium.org/9834037/diff/4003/base/test/android/java/org/c...
base/test/android/java/org/chromium/ChromeNativeTestActivity.java:5: package
org.chromium.native_test;
On 2012/03/27 13:53:24, Satish wrote:
> should this file be under a 'org/chromium/native_test' directory instead of
> 'org/chromium'?

For future proof where we may add resources, we should put it under
base/test/android/java/src/org/chromium/native_test.

http://codereview.chromium.org/9834037/diff/4003/base/test/android/native_tes...
File base/test/android/native_test.gyp (right):

http://codereview.chromium.org/9834037/diff/4003/base/test/android/native_tes...
base/test/android/native_test.gyp:19: 'native_test_launcher.cc'
Should we put native and Java in separate target?

http://codereview.chromium.org/9834037/diff/4003/base/test/android/native_tes...
base/test/android/native_test.gyp:26:
'<(PRODUCT_DIR)/ChromeNativeTests-debug.apk',
Why we have to call "-debug"?

http://codereview.chromium.org/9834037/diff/4003/base/test/android/native_tes...
File base/test/android/native_test_apk.xml (right):

http://codereview.chromium.org/9834037/diff/4003/base/test/android/native_tes...
base/test/android/native_test_apk.xml:1: <?xml version="1.0" encoding="UTF-8"?>
copyright?