apk-based test runner work for android

apk-based test runner work for android. 2 unit test bundles converted over (ipc, base).

OFF by default; enable with a gyp var.  E.g.
  GYP_DEFINES="$GYP_DEFINES gtest_target_type=shared_library" android_gyp

Some useful commands:
adb uninstall org.chromium.native_test
adb install -r out/Release/base_unittests_apk/ChromeNativeTests-debug.apk
adb shell am start -n org.chromium.native_test/org.chromium.native_test.ChromeNativeTestActivity

For the moment, all apks can be built simultaneously but use the same
activity name.  Thus you cannot have more than one installed at the
same time.

BUG=None
TEST=


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

[John Grabowski, 2012-04-12 00:09:20]

Sending to Androiders before others.

Rework of http://chromiumcodereview.appspot.com/9834037/
Lots of ideas/logic/code/mojo stolen from Marcus

[Yaron, 2012-04-12 01:29:52]

So it definitely seems like you want a template for that but seems reasonable to
have someone work on that in parallel

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/all_android.gyp
File build/all_android.gyp (right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/all_android.gy...
build/all_android.gyp:52: '../testing/android/native_test.gyp:native_test_apk',
Wait, but why is this needed if you've already specified it in the global list
above?

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/android/buildb...
File build/android/buildbot_functions.sh (right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/android/buildb...
build/android/buildbot_functions.sh:206: -s
out/Release/replaceme_apk/ChromeNativeTests-debug.apk \
replaceme_apk? I dont' see where this instance of replaceme is replaced

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/android/buildb...
build/android/buildbot_functions.sh:208: happy_failure=$(cat $tempfile | grep
NATIVE_LOADER_FAILED | wc -l)
is a failure really ever happy?

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/gene...
File testing/android/generate_native_test.py (right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/gene...
testing/android/generate_native_test.py:151: # NATIVE_LOADER_FAILED wnen run.
s/wnen/when

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/java...
File
testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java
(right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/java...
testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java:53:
// which run tests,.  For example, we look for
s/,././

[bulach, 2012-04-12 15:51:42]

nice stuff, looking forward to have this in place!!

a few general suggestions below:

http://codereview.chromium.org/10051021/diff/2001/base/android/java/base.xml
File base/android/java/base.xml (right):

http://codereview.chromium.org/10051021/diff/2001/base/android/java/base.xml#...
base/android/java/base.xml:15: <property name="dest.dir"
location="${PRODUCT_DIR}/base_java"/>
hmm, not too keen on base_java.. how about either:
${PRODUCT_DIR}/java/base (so that everything goes in the same java dir

or 
${PRODUCT_DIR}/base/java (so that it reflects the directory structure?)

http://codereview.chromium.org/10051021/diff/2001/base/base.gyp
File base/base.gyp (right):

http://codereview.chromium.org/10051021/diff/2001/base/base.gyp#newcode689
base/base.gyp:689: 'base_unittests',
not quite clear what's the benefit of having two actions here...
would it simplify if:
1. 674, just condition it on android.. tough luck if someone call with the wrong
gtest_target_type.

2. make just one action, "generate_base_unittests_apk", call the .py script, and
then let that script call ant directly.. 

3. make all the dependencies explicit on the target on 680 itself: there's a
duplication on 679 and 689, not sure if that's needed..

end goal being to have a single target that will call a single script to
magically from foo.so / manifest.xml / friends to foo.apk... ensure all
necessary gyp variables are passed to this script..

how does it sound?

http://codereview.chromium.org/10051021/diff/2001/build/android/buildbot_func...
File build/android/buildbot_functions.sh (right):

http://codereview.chromium.org/10051021/diff/2001/build/android/buildbot_func...
build/android/buildbot_functions.sh:208: happy_failure=$(cat $tempfile | grep
NATIVE_LOADER_FAILED | wc -l)
nit: a grep -c makes wc unecessary... but honestly, this logic is spread across
way too many layers... just move all this logic into the run_tests.py itself, I
guess all decisions on what a failure is should be self contained there..

http://codereview.chromium.org/10051021/diff/2001/build/android/run_tests.py
File build/android/run_tests.py (right):

http://codereview.chromium.org/10051021/diff/2001/build/android/run_tests.py#...
build/android/run_tests.py:81: if not apk:
nit: maybe invert and "if apk:" would be, erm, less negative?

http://codereview.chromium.org/10051021/diff/2001/build/android/single_test_r...
File build/android/single_test_runner.py (right):

http://codereview.chromium.org/10051021/diff/2001/build/android/single_test_r...
build/android/single_test_runner.py:59: 'local')
56-59 seem unnecessary in this context..

http://codereview.chromium.org/10051021/diff/2001/build/android/test_package.py
File build/android/test_package.py (right):

http://codereview.chromium.org/10051021/diff/2001/build/android/test_package....
build/android/test_package.py:177: timed_out, overall_fail)
see below, default values and non-named params without static analysis tool are
evil..

http://codereview.chromium.org/10051021/diff/2001/build/android/test_result.py
File build/android/test_result.py (right):

http://codereview.chromium.org/10051021/diff/2001/build/android/test_result.p...
build/android/test_result.py:64: def FromOkAndFailed(ok, failed,
timed_out=False, overall_fail=False):
my rant:
if we had tools to statically check this...
the problem is that it's not possible for reviewers to know whether a param has
a default or not, and then which one that is.. I'd much rather *not* have
default params at all but then again.. :)


http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Default_Argume...
Calling code must use named values for arguments with a default value. This
helps document the code somewhat and helps prevent and detect interface breakage
when more arguments are added.

http://codereview.chromium.org/10051021/diff/2001/ipc/ipc.gyp
File ipc/ipc.gyp (right):

http://codereview.chromium.org/10051021/diff/2001/ipc/ipc.gyp#newcode98
ipc/ipc.gyp:98: # See base.gyp for TODO(jrg)s about this strategy.
ditto..

http://codereview.chromium.org/10051021/diff/2001/testing/android/AndroidMani...
File testing/android/AndroidManifest.xml (right):

http://codereview.chromium.org/10051021/diff/2001/testing/android/AndroidMani...
testing/android/AndroidManifest.xml:26: <!-- TODO(jrg): add more permissions as
needed by unit tests. -->
nit: I'd remove this TODO..

http://codereview.chromium.org/10051021/diff/2001/testing/android/generate_na...
File testing/android/generate_native_test.py (right):

http://codereview.chromium.org/10051021/diff/2001/testing/android/generate_na...
testing/android/generate_native_test.py:14: # all unit test bundles.
as above, I'd suggest calling ant and building the apk here as well..

http://codereview.chromium.org/10051021/diff/2001/testing/android/generate_na...
testing/android/generate_native_test.py:93: raise Exception('Unbelievable output
directory; bailing for safety')
nit: probably best to do this sanity check at the ctor time and require that
output directory contain '/out/' right there..

http://codereview.chromium.org/10051021/diff/2001/testing/android/generate_na...
testing/android/generate_native_test.py:105: logging.warn('Replacing
\'replaceme\' with ' + self._root_name)
nit: use " inside the string, avoids escaping..

http://codereview.chromium.org/10051021/diff/2001/testing/android/generate_na...
testing/android/generate_native_test.py:114: """Copy the shlib and jars into the
apk (if relevant)"""
not sure what you mean "into the apk".. the apk itself is basically a zip file,
so I guess what you mean is into the target apk directory?

http://codereview.chromium.org/10051021/diff/2001/testing/android/generate_na...
testing/android/generate_native_test.py:144: help='Comma seperated list of jars
to include')
nit: instead of splitting here, maybe just use --jar with action='append'? 
then let the caller specify as many --jar as they want.. 
options.jar would end up here already as a list, so no need to know the format /
split..

http://codereview.chromium.org/10051021/diff/2001/testing/android/generate_na...
testing/android/generate_native_test.py:147: options, args =
parser.parse_args(argv)  # pylint: disable=W0612
is W0612 unused locals? would replacing args with _ work?

http://codereview.chromium.org/10051021/diff/2001/testing/android/java/src/or...
File
testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java
(right):

http://codereview.chromium.org/10051021/diff/2001/testing/android/java/src/or...
testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java:41:
// triggers nativeTestFailed().
is it needed? the runner should be smart enough to detect all failures (i.e.,
essentially pexpect there should capture a [RUN] without a corresponding [OK] as
an error...).

http://codereview.chromium.org/10051021/diff/2001/testing/android/java/src/or...
testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java:45:
} catch (UnsatisfiedLinkError e) {
you probably need to wrap just loadLibrary, not the Thread creation..

http://codereview.chromium.org/10051021/diff/2001/testing/android/native_test...
File testing/android/native_test.gyp (right):

http://codereview.chromium.org/10051021/diff/2001/testing/android/native_test...
testing/android/native_test.gyp:50: # We must export JNI_OnLoad().
which option is doing this? maybe add a note?

http://codereview.chromium.org/10051021/diff/2001/testing/android/native_test...
File testing/android/native_test_launcher.cc (right):

http://codereview.chromium.org/10051021/diff/2001/testing/android/native_test...
testing/android/native_test_launcher.cc:182: * the definition was never
upstreamed.  Why?  Is there an alternative
ha! I know why.. ;)
http://codereview.chromium.org/9599010/diff/1/base/android/jni_android.h#newc...

feel free to add there now that we're using..

[John Grabowski, 2012-04-13 01:11:37]

Feedback applied; thx

http://chromiumcodereview.appspot.com/10051021/diff/2001/base/android/java/ba...
File base/android/java/base.xml (right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/base/android/java/ba...
base/android/java/base.xml:15: <property name="dest.dir"
location="${PRODUCT_DIR}/base_java"/>
On 2012/04/12 15:51:42, bulach wrote:
> hmm, not too keen on base_java.. how about either:
> ${PRODUCT_DIR}/java/base (so that everything goes in the same java dir
> 
> or 
> ${PRODUCT_DIR}/base/java (so that it reflects the directory structure?)

went with java/base to closer reflect other use cases (e.g. obj in a dir, ...)

http://chromiumcodereview.appspot.com/10051021/diff/2001/base/base.gyp
File base/base.gyp (right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/base/base.gyp#newcod...
base/base.gyp:689: 'base_unittests',
On 2012/04/12 15:51:42, bulach wrote:
> not quite clear what's the benefit of having two actions here...
> would it simplify if:
> 1. 674, just condition it on android.. tough luck if someone call with the
wrong
> gtest_target_type.
> 
> 2. make just one action, "generate_base_unittests_apk", call the .py script,
and
> then let that script call ant directly.. 
> 
> 3. make all the dependencies explicit on the target on 680 itself: there's a
> duplication on 679 and 689, not sure if that's needed..
> 
> end goal being to have a single target that will call a single script to
> magically from foo.so / manifest.xml / friends to foo.apk... ensure all
> necessary gyp variables are passed to this script..
> 
> how does it sound?

I like 2 and 3.
I don't like 1; Chrome philosophy is "hide behind a flag until ready".

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/all_android.gyp
File build/all_android.gyp (right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/all_android.gy...
build/all_android.gyp:52: '../testing/android/native_test.gyp:native_test_apk',
On 2012/04/12 01:29:53, Yaron wrote:
> Wait, but why is this needed if you've already specified it in the global list
> above?

oops... shouldn't be above yet

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/android/buildb...
File build/android/buildbot_functions.sh (right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/android/buildb...
build/android/buildbot_functions.sh:206: -s
out/Release/replaceme_apk/ChromeNativeTests-debug.apk \
On 2012/04/12 01:29:53, Yaron wrote:
> replaceme_apk? I dont' see where this instance of replaceme is replaced

It isn't.  This test is for compiling the template itself.

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/android/buildb...
build/android/buildbot_functions.sh:208: happy_failure=$(cat $tempfile | grep
NATIVE_LOADER_FAILED | wc -l)
On 2012/04/12 01:29:53, Yaron wrote:
> is a failure really ever happy?

Yes.  If compiling the template yells about "no shared library", that implies we
could build and run it successfully.

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/android/buildb...
build/android/buildbot_functions.sh:208: happy_failure=$(cat $tempfile | grep
NATIVE_LOADER_FAILED | wc -l)
On 2012/04/12 15:51:42, bulach wrote:
> nit: a grep -c makes wc unecessary... but honestly, this logic is spread
across
> way too many layers... just move all this logic into the run_tests.py itself,
I
> guess all decisions on what a failure is should be self contained there..

Not sure I agree.  We are building the template.  
Once it is stable this will likely get removed.

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/android/run_te...
File build/android/run_tests.py (right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/android/run_te...
build/android/run_tests.py:81: if not apk:
On 2012/04/12 15:51:42, bulach wrote:
> nit: maybe invert and "if apk:" would be, erm, less negative?

Done.

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/android/single...
File build/android/single_test_runner.py (right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/android/single...
build/android/single_test_runner.py:59: 'local')
On 2012/04/12 15:51:42, bulach wrote:
> 56-59 seem unnecessary in this context..

right; removed

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/android/test_r...
File build/android/test_result.py (right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/android/test_r...
build/android/test_result.py:64: def FromOkAndFailed(ok, failed,
timed_out=False, overall_fail=False):
On 2012/04/12 15:51:42, bulach wrote:
> my rant:
> if we had tools to statically check this...
> the problem is that it's not possible for reviewers to know whether a param
has
> a default or not, and then which one that is.. I'd much rather *not* have
> default params at all but then again.. :)
> 
> 
>
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Default_Argume...
> Calling code must use named values for arguments with a default value. This
> helps document the code somewhat and helps prevent and detect interface
breakage
> when more arguments are added.

Done.

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/Andr...
File testing/android/AndroidManifest.xml (right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/Andr...
testing/android/AndroidManifest.xml:26: <!-- TODO(jrg): add more permissions as
needed by unit tests. -->
On 2012/04/12 15:51:42, bulach wrote:
> nit: I'd remove this TODO..

However, it is an open action item once I get the tests working, and explains
diff with downstream.

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/gene...
File testing/android/generate_native_test.py (right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/gene...
testing/android/generate_native_test.py:93: raise Exception('Unbelievable output
directory; bailing for safety')
On 2012/04/12 15:51:42, bulach wrote:
> nit: probably best to do this sanity check at the ctor time and require that
> output directory contain '/out/' right there..

Makes me feel safer to do it "right before" the rmtree

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/gene...
testing/android/generate_native_test.py:105: logging.warn('Replacing
\'replaceme\' with ' + self._root_name)
On 2012/04/12 15:51:42, bulach wrote:
> nit: use " inside the string, avoids escaping..

Done.

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/gene...
testing/android/generate_native_test.py:114: """Copy the shlib and jars into the
apk (if relevant)"""
On 2012/04/12 15:51:42, bulach wrote:
> not sure what you mean "into the apk".. the apk itself is basically a zip
file,
> so I guess what you mean is into the target apk directory?

clarified

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/gene...
testing/android/generate_native_test.py:144: help='Comma seperated list of jars
to include')
On 2012/04/12 15:51:42, bulach wrote:
> nit: instead of splitting here, maybe just use --jar with action='append'? 
> then let the caller specify as many --jar as they want.. 
> options.jar would end up here already as a list, so no need to know the format
/
> split..

Done.

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/gene...
testing/android/generate_native_test.py:147: options, args =
parser.parse_args(argv)  # pylint: disable=W0612
On 2012/04/12 15:51:42, bulach wrote:
> is W0612 unused locals? would replacing args with _ work?

Done.

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/gene...
testing/android/generate_native_test.py:151: # NATIVE_LOADER_FAILED wnen run.
On 2012/04/12 01:29:53, Yaron wrote:
> s/wnen/when

Done.

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/java...
File
testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java
(right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/java...
testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java:41:
// triggers nativeTestFailed().
On 2012/04/12 15:51:42, bulach wrote:
> is it needed? the runner should be smart enough to detect all failures (i.e.,
> essentially pexpect there should capture a [RUN] without a corresponding [OK]
as
> an error...).

Yes, it is needed.  If we crash we have to wait for a "no activity in 60sec"
timeout which sucks.

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/java...
testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java:45:
} catch (UnsatisfiedLinkError e) {
On 2012/04/12 15:51:42, bulach wrote:
> you probably need to wrap just loadLibrary, not the Thread creation..

Not sure I agree; is possible loadLibrary() works but didn't resolve
nativeRunTests().  But offhand I can't say that'll throw the same exception.

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/java...
testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java:53:
// which run tests,.  For example, we look for
On 2012/04/12 01:29:53, Yaron wrote:
> s/,././

Done.

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/nati...
File testing/android/native_test.gyp (right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/nati...
testing/android/native_test.gyp:50: # We must export JNI_OnLoad().
On 2012/04/12 15:51:42, bulach wrote:
> which option is doing this? maybe add a note?

I don't understand.  this option right here, the one we are removing, prevents a
.so from re-exporting any symbol that comes in via .a.  But JNI_OnLoad() comes
in from native_test_launcher.cc which is in a .a which is pulled in by
libbase_unittests.so

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/nati...
File testing/android/native_test_launcher.cc (right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/nati...
testing/android/native_test_launcher.cc:182: * the definition was never
upstreamed.  Why?  Is there an alternative
On 2012/04/12 15:51:42, bulach wrote:
> ha! I know why.. ;)
>
http://codereview.chromium.org/9599010/diff/1/base/android/jni_android.h#newc...
> 
> feel free to add there now that we're using..

Done.

[bulach, 2012-04-13 09:42:51]

lgtm

thanks john! this is truly exciting, looking forward to it! 
a few suggestions and nits below:

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/android/buildb...
File build/android/buildbot_functions.sh (right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/build/android/buildb...
build/android/buildbot_functions.sh:208: happy_failure=$(cat $tempfile | grep
NATIVE_LOADER_FAILED | wc -l)
On 2012/04/13 01:11:37, John Grabowski wrote:
> On 2012/04/12 15:51:42, bulach wrote:
> > nit: a grep -c makes wc unecessary... but honestly, this logic is spread
> across
> > way too many layers... just move all this logic into the run_tests.py
itself,
> I
> > guess all decisions on what a failure is should be self contained there..
> 
> Not sure I agree.  We are building the template.  
> Once it is stable this will likely get removed.

well, even if it's temporary, I'd rather have the logic in the existing place
rather than spreaded in multiple levels of scripts (more below..)

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/java...
File
testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java
(right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/java...
testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java:41:
// triggers nativeTestFailed().
On 2012/04/13 01:11:37, John Grabowski wrote:
> On 2012/04/12 15:51:42, bulach wrote:
> > is it needed? the runner should be smart enough to detect all failures
(i.e.,
> > essentially pexpect there should capture a [RUN] without a corresponding
[OK]
> as
> > an error...).
> 
> Yes, it is needed.  If we crash we have to wait for a "no activity in 60sec"
> timeout which sucks.

right, sorry, I wasn't clear: rather than add yet-another failure case, wouldn't
it be sufficient to print something like:
[RUN] Native Loader Failure / LOG / [FAIL] and reuse the existing mechanism?

that would simplify the upper layers..

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/java...
testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java:56:
Log.e(TAG, "[ NATIVE_LOADER_FAILED ]");
see above, we could probably reuse the existing mechanism here..

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/nati...
File testing/android/native_test.gyp (right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/nati...
testing/android/native_test.gyp:50: # We must export JNI_OnLoad().
On 2012/04/13 01:11:37, John Grabowski wrote:
> On 2012/04/12 15:51:42, bulach wrote:
> > which option is doing this? maybe add a note?
> 
> I don't understand.  this option right here, the one we are removing, prevents
a
> .so from re-exporting any symbol that comes in via .a.  But JNI_OnLoad() comes
> in from native_test_launcher.cc which is in a .a which is pulled in by
> libbase_unittests.so

maybe just rephrase:
# JNI_OnLoad is implemented in a .a and we need to
# re-export in the .so.

(my confusion came from the fact that it is already mark as exported in the .cc
file..).

http://chromiumcodereview.appspot.com/10051021/diff/8004/build/android/buildb...
File build/android/buildbot_try_compile_test.sh (right):

http://chromiumcodereview.appspot.com/10051021/diff/8004/build/android/buildb...
build/android/buildbot_try_compile_test.sh:19: bb_native_test_compile_run_tests
should this be on the fyi bot too?

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/gene...
File testing/android/generate_native_test.py (right):

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/gene...
testing/android/generate_native_test.py:173: options, _ =
parser.parse_args(argv)  # pylint: disable=W0612
nit: remove the pylint annotation?

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/nati...
File testing/android/native_test.gyp (right):

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/nati...
testing/android/native_test.gyp:62: 'target_name':
'testing_android_jni_headers',
nit: how about native_test_jni_headers? the "testing" bit seems like this is
running a test for the android_jni_headers..

[Peter Beverloo, 2012-04-13 10:01:49]

Thanks jrg! Some more nits..

http://chromiumcodereview.appspot.com/10051021/diff/8004/build/android/test_p...
File build/android/test_package_apk.py (right):

http://chromiumcodereview.appspot.com/10051021/diff/8004/build/android/test_p...
build/android/test_package_apk.py:82: def StripAndCopyExecutable(self):
This function is not stripping anything.

http://chromiumcodereview.appspot.com/10051021/diff/8004/build/android/test_p...
build/android/test_package_apk.py:88: # Now install the new one.
nit: this comment doesn't add any value, especially given the logging.info() on
the next line.

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/Andr...
File testing/android/AndroidManifest.xml (right):

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/Andr...
testing/android/AndroidManifest.xml:13: <uses-sdk android:minSdkVersion="14" />
Now this refers to SDK 14 again. Since this is the baseline for ICS, we probably
want to update envsetup.sh to use 14 everywhere instead of the current mix of
14/15.

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/nati...
File testing/android/native_test_apk.xml (right):

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/nati...
testing/android/native_test_apk.xml:25: <property name="target"
value="android-14"/>
As before: we should use either 14 or 15, not both (the sdk.version var in this
ant build-script refers to 15). Is the target property used anywhere either way?

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/res/...
File testing/android/res/values/strings.xml (right):

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/res/...
testing/android/res/values/strings.xml:10: <string
name="native_library">replaceme</string>
Should this have a TODO? "replaceme" seems odd.

[John Grabowski, 2012-04-13 22:51:02]

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/java...
File
testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java
(right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/java...
testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java:41:
// triggers nativeTestFailed().
On 2012/04/13 09:42:51, bulach wrote:
> On 2012/04/13 01:11:37, John Grabowski wrote:
> > On 2012/04/12 15:51:42, bulach wrote:
> > > is it needed? the runner should be smart enough to detect all failures
> (i.e.,
> > > essentially pexpect there should capture a [RUN] without a corresponding
> [OK]
> > as
> > > an error...).
> > 
> > Yes, it is needed.  If we crash we have to wait for a "no activity in 60sec"
> > timeout which sucks.
> 
> right, sorry, I wasn't clear: rather than add yet-another failure case,
wouldn't
> it be sufficient to print something like:
> [RUN] Native Loader Failure / LOG / [FAIL] and reuse the existing mechanism?
> 
> that would simplify the upper layers..

I would prefer to make 'a test failed' distinct from 'failed to run any tests'. 
I've made the error more generic; is that OK?

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/nati...
File testing/android/native_test.gyp (right):

http://chromiumcodereview.appspot.com/10051021/diff/2001/testing/android/nati...
testing/android/native_test.gyp:50: # We must export JNI_OnLoad().
On 2012/04/13 09:42:51, bulach wrote:
> On 2012/04/13 01:11:37, John Grabowski wrote:
> > On 2012/04/12 15:51:42, bulach wrote:
> > > which option is doing this? maybe add a note?
> > 
> > I don't understand.  this option right here, the one we are removing,
prevents
> a
> > .so from re-exporting any symbol that comes in via .a.  But JNI_OnLoad()
comes
> > in from native_test_launcher.cc which is in a .a which is pulled in by
> > libbase_unittests.so
> 
> maybe just rephrase:
> # JNI_OnLoad is implemented in a .a and we need to
> # re-export in the .so.
> 
> (my confusion came from the fact that it is already mark as exported in the
.cc
> file..).

Done.

http://chromiumcodereview.appspot.com/10051021/diff/8004/build/android/buildb...
File build/android/buildbot_try_compile_test.sh (right):

http://chromiumcodereview.appspot.com/10051021/diff/8004/build/android/buildb...
build/android/buildbot_try_compile_test.sh:19: bb_native_test_compile_run_tests
On 2012/04/13 09:42:51, bulach wrote:
> should this be on the fyi bot too?

Not until it stabilizes a bit.

http://chromiumcodereview.appspot.com/10051021/diff/8004/build/android/test_p...
File build/android/test_package_apk.py (right):

http://chromiumcodereview.appspot.com/10051021/diff/8004/build/android/test_p...
build/android/test_package_apk.py:82: def StripAndCopyExecutable(self):
On 2012/04/13 10:01:49, Peter Beverloo wrote:
> This function is not stripping anything.

It is an override of a base class function.

http://chromiumcodereview.appspot.com/10051021/diff/8004/build/android/test_p...
build/android/test_package_apk.py:88: # Now install the new one.
On 2012/04/13 10:01:49, Peter Beverloo wrote:
> nit: this comment doesn't add any value, especially given the logging.info()
on
> the next line.

removed.

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/Andr...
File testing/android/AndroidManifest.xml (right):

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/Andr...
testing/android/AndroidManifest.xml:13: <uses-sdk android:minSdkVersion="14" />
On 2012/04/13 10:01:49, Peter Beverloo wrote:
> Now this refers to SDK 14 again. Since this is the baseline for ICS, we
probably
> want to update envsetup.sh to use 14 everywhere instead of the current mix of
> 14/15.

15ified

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/gene...
File testing/android/generate_native_test.py (right):

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/gene...
testing/android/generate_native_test.py:173: options, _ =
parser.parse_args(argv)  # pylint: disable=W0612
On 2012/04/13 09:42:51, bulach wrote:
> nit: remove the pylint annotation?

Done.

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/nati...
File testing/android/native_test.gyp (right):

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/nati...
testing/android/native_test.gyp:62: 'target_name':
'testing_android_jni_headers',
On 2012/04/13 09:42:51, bulach wrote:
> nit: how about native_test_jni_headers? the "testing" bit seems like this is
> running a test for the android_jni_headers..

Or even just "jni_headers" since it's already in the native_test.gyp namespace.

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/nati...
File testing/android/native_test_apk.xml (right):

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/nati...
testing/android/native_test_apk.xml:25: <property name="target"
value="android-14"/>
On 2012/04/13 10:01:49, Peter Beverloo wrote:
> As before: we should use either 14 or 15, not both (the sdk.version var in
this
> ant build-script refers to 15). Is the target property used anywhere either
way?

Unified.

The target property is used by ${sdk.dir}/tools/ant/build.xml

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/res/...
File testing/android/res/values/strings.xml (right):

http://chromiumcodereview.appspot.com/10051021/diff/8004/testing/android/res/...
testing/android/res/values/strings.xml:10: <string
name="native_library">replaceme</string>
On 2012/04/13 10:01:49, Peter Beverloo wrote:
> Should this have a TODO? "replaceme" seems odd.

No.  This is a template file.  The python included in this CL replaced
'replaceme' with the right solib name.

[John Grabowski, 2012-04-13 22:53:01]

+jam, +mark for ipc and base owners

[jam, 2012-04-15 22:19:19]

ipc lgtm

[Mark Mentovai, 2012-04-16 15:26:39]

LGTM

[bulach, 2012-04-16 15:44:30]

lgtm

thanks john, I agree with the new failure mode.. a few minor nits, feel free to
go ahead.

http://chromiumcodereview.appspot.com/10051021/diff/16001/build/android/build...
File build/android/buildbot_functions.sh (right):

http://chromiumcodereview.appspot.com/10051021/diff/16001/build/android/build...
build/android/buildbot_functions.sh:208: happy_failure=$(cat $tempfile | grep
RUNNER_FAILED | wc -l)
nit: grep -c

http://chromiumcodereview.appspot.com/10051021/diff/16001/build/android/test_...
File build/android/test_package.py (right):

http://chromiumcodereview.appspot.com/10051021/diff/16001/build/android/test_...
build/android/test_package.py:140: found = p.expect([re_run, pexpect.EOF,
re_runner_fail],
nit: would prefer to keep EOF at the end, so that we group all our failures,
then everything else.. or the other way round, keep it as the first one so we'll
never have to change it again?

http://chromiumcodereview.appspot.com/10051021/diff/16001/build/android/test_...
build/android/test_package.py:142: if found == 1:  # matched pexpect.EOF
whatever you decide, this will probably need changing..

[Ryan Sleevi, 2012-04-17 01:02:05]

Just a few drive-by nits that can probably be addressed as follow-ups. Just
wanted to answer some of your TODOs.

http://codereview.chromium.org/10051021/diff/16001/base/base.gyp
File base/base.gyp (right):

http://codereview.chromium.org/10051021/diff/16001/base/base.gyp#newcode668
base/base.gyp:668: # Hard-coding in the gypfile seems a poor choice.
Use <(SHARED_LIB_DIR) (and indicate that this target is only built for the
'target' cross-compile, not 'host')

http://codereview.chromium.org/10051021/diff/16001/base/base.gyp#newcode673
base/base.gyp:673: # 'shared_library_apk_wrapper' that does a lot of this
magically.
Or use a .gypi file.

{
  'targets': [
    {
       'target_name': 'base_unittests_apk',
       'type': 'none',
       'dependencies': [ ... ],
       'variables': {
         'apk_name': 'base_unittests',
         'jar_files': [
            '<(PRODUCT_DIR)/chromium_base.jar'
         ],
         'shared_libs': [
          
'<(SHARED_LIB_DIR)/<(SHARED_LIB_PREFIX)base_unittests<(SHARED_LIB_SUFFIX)',
         ],
       },
       'includes': [ 'build/android/unittests_apk.gypi' ]
    },
  ]
}

http://codereview.chromium.org/10051021/diff/16001/base/base.gyp#newcode680
base/base.gyp:680: 'base',  # So that android/java/java.gyp:base_java is built
You should depend on android/java/java.gyp:base_java in that case

http://codereview.chromium.org/10051021/diff/16001/testing/android/native_tes...
File testing/android/native_test_launcher.cc (right):

http://codereview.chromium.org/10051021/diff/16001/testing/android/native_tes...
testing/android/native_test_launcher.cc:131: FilePath
path("/data/user/0/org.chromium.native_tests/files/");
nit: Does this have to be hard-coded? It seems like this should be from an
environment variable itself.

[John Grabowski, 2012-04-18 21:31:09]

Thx for feedback Ryan; I'll apply the changes in follow-up CLs with Yaron.

http://codereview.chromium.org/10051021/diff/16001/testing/android/native_tes...
File testing/android/native_test_launcher.cc (right):

http://codereview.chromium.org/10051021/diff/16001/testing/android/native_tes...
testing/android/native_test_launcher.cc:131: FilePath
path("/data/user/0/org.chromium.native_tests/files/");
On 2012/04/17 01:02:05, Ryan Sleevi wrote:
> nit: Does this have to be hard-coded? It seems like this should be from an
> environment variable itself.

Let's be clear; this code is run inside an APK so we can't set env vars as such.
This will need to change since the current code assumes the device is rooted. 
We don't want to require that upstream; we'll need to package the test files in
the apk as well, so this becomes a jni call.  But we're not there yet.

[John Grabowski, 2012-04-18 21:45:37]

+ some media/OWNERS

[scherkus (not reviewing), 2012-04-18 22:43:19]

media lgtm w/ nit/question

http://codereview.chromium.org/10051021/diff/29001/media/base/android/OWNERS
File media/base/android/OWNERS (right):

http://codereview.chromium.org/10051021/diff/29001/media/base/android/OWNERS#...
media/base/android/OWNERS:2: bulach@chromium.org
OCD nit: it seems like other OWNERS files have a->z ordering, not sure if that's
a rule or not :)

[John Grabowski, 2012-04-19 00:36:39]

OCD accomodated (both files).
Thanks guys.

[commit-bot: I haz the power, 2012-04-19 00:45:21]

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

[commit-bot: I haz the power, 2012-04-19 02:03:26]

Try job failure for 10051021-40001 (retry) on mac_rel for step "update".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-04-19 18:20:30]

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

[commit-bot: I haz the power, 2012-04-19 21:02:12]

Change committed as 133053