[Android] Refactor the native test wrappers.

[Android] Refactor the native test wrappers.

BUG=476410
Committed: https://crrev.com/581d25e509e4a2b1e8927c6d8accc0c90ea86090
Cr-Commit-Position: refs/heads/master@{#330531}

[jbudorick, 2015-05-05 15:29:42]

jbudorick@chromium.org changed reviewers:
+ cjhopman@chromium.org, jaekyun@chromium.org, perezju@chromium.org,
tedchoc@chromium.org

[jbudorick, 2015-05-05 15:29:42]

Despite what it may look like, this is not just an attempt at CL language bingo.

perezju: build/android/
cjhopman: gyp + gn + anything else you want to look at
tedchoc: components/test/android/, content/shell/android/, testing/android/

jaekyun: FYI for components/test/android

[perezju, 2015-05-05 16:15:01]

mostly a rubber-stamp lgtm, just added a few comments on apk_helper

https://codereview.chromium.org/1126543009/diff/60001/build/android/pylib/uti...
File build/android/pylib/utils/apk_helper.py (right):

https://codereview.chromium.org/1126543009/diff/60001/build/android/pylib/uti...
build/android/pylib/utils/apk_helper.py:20: def GetPackageName(apk_path):
maybe add TODO notes to deprecate these? Or we expect to continue using them?

https://codereview.chromium.org/1126543009/diff/60001/build/android/pylib/uti...
build/android/pylib/utils/apk_helper.py:78: ['android:name'][0])
I guess only this line should be in the try-except block

https://codereview.chromium.org/1126543009/diff/60001/build/android/pylib/uti...
build/android/pylib/utils/apk_helper.py:92: return
manifest_info['manifest']['instrumentation']['android:name'][0]
same here, only this line in the try-except block

[Jaekyun Seok (inactive), 2015-05-05 22:40:35]

https://codereview.chromium.org/1126543009/diff/60001/testing/android/native_...
File testing/android/native_test/native_test_launcher.cc (right):

https://codereview.chromium.org/1126543009/diff/60001/testing/android/native_...
testing/android/native_test/native_test_launcher.cc:124: if
(!base::android::RedirectStream(stdout, stdout_file_path, "w")) {
"w+" is being used for content_browsertests and components_browsertests; you can
see the details in https://codereview.chromium.org/1003073002 .

[Ted C, 2015-05-05 23:11:17]

https://codereview.chromium.org/1126543009/diff/60001/build/android/pylib/gte...
File build/android/pylib/gtest/gtest_test_instance.py (right):

https://codereview.chromium.org/1126543009/diff/60001/build/android/pylib/gte...
build/android/pylib/gtest/gtest_test_instance.py:22: '--single_process',
Any reason we don't add these in the java side activities at this point?

https://codereview.chromium.org/1126543009/diff/60001/components/test/android...
File
components/test/android/browsertests_apk/components_browser_tests_android.cc
(left):

https://codereview.chromium.org/1126543009/diff/60001/components/test/android...
components/test/android/browsertests_apk/components_browser_tests_android.cc:41:
static const char kCommandLineFilePath[] =
is the test runner now going to pass this in via a command line?

https://codereview.chromium.org/1126543009/diff/60001/components/test/android...
components/test/android/browsertests_apk/components_browser_tests_android.cc:55:
static const char* const kInitialArgv[] = {"ComponentsBrowserTestsActivity"};
And it looks like this is lost in the conversion to native_test_launcher.cc.  I
think now they all share the same kInitialArgv, which I don't actually know if
it matters just pointing it out.

https://codereview.chromium.org/1126543009/diff/60001/components/test/android...
components/test/android/browsertests_apk/components_browser_tests_android.cc:81:
"components_browsertests_devtools_remote");
It seems like we lost this (or at least I can't find it).  Same in the other
instances.

https://codereview.chromium.org/1126543009/diff/60001/testing/android/native_...
File
testing/android/native_test/java/src/org/chromium/native_test/NativeBrowserTestActivity.java
(right):

https://codereview.chromium.org/1126543009/diff/60001/testing/android/native_...
testing/android/native_test/java/src/org/chromium/native_test/NativeBrowserTestActivity.java:18:
loadLibraries();
what libraries is it loading here?  this looks backwards to me...shouldn't the
libraries be loaded before the browser is started?

The browser starting also will load a bunch of libraries, so is this duplicated
work?

Looking at the next file it seems like maybe these are the libraries that
contain the test code (maybe)?  Seems like we should figure out a clearer name
here since libraries is pretty overloaded.

https://codereview.chromium.org/1126543009/diff/60001/testing/android/native_...
testing/android/native_test/java/src/org/chromium/native_test/NativeBrowserTestActivity.java:21:
protected abstract void startBrowser();
Add some javadoc here.  Also, does the browser need to be fully running after
returning from this call?

https://codereview.chromium.org/1126543009/diff/60001/testing/android/native_...
File
testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java
(right):

https://codereview.chromium.org/1126543009/diff/60001/testing/android/native_...
testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java:116:
protected void loadLibraries() {
javadoc here as well.

[jbudorick, 2015-05-06 18:00:00]

https://codereview.chromium.org/1126543009/diff/60001/build/android/pylib/gte...
File build/android/pylib/gtest/gtest_test_instance.py (right):

https://codereview.chromium.org/1126543009/diff/60001/build/android/pylib/gte...
build/android/pylib/gtest/gtest_test_instance.py:22: '--single_process',
On 2015/05/05 23:11:17, Ted C wrote:
> Any reason we don't add these in the java side activities at this point?

Nope, and it's cleaner to do so.

https://codereview.chromium.org/1126543009/diff/60001/build/android/pylib/uti...
File build/android/pylib/utils/apk_helper.py (right):

https://codereview.chromium.org/1126543009/diff/60001/build/android/pylib/uti...
build/android/pylib/utils/apk_helper.py:20: def GetPackageName(apk_path):
On 2015/05/05 16:15:01, perezju wrote:
> maybe add TODO notes to deprecate these? Or we expect to continue using them?

I definitely expect to keep using this one. Not sure about
GetInstrumentationName, so I'm adding a TODO there.

https://codereview.chromium.org/1126543009/diff/60001/build/android/pylib/uti...
build/android/pylib/utils/apk_helper.py:78: ['android:name'][0])
On 2015/05/05 16:15:01, perezju wrote:
> I guess only this line should be in the try-except block

Done.

https://codereview.chromium.org/1126543009/diff/60001/build/android/pylib/uti...
build/android/pylib/utils/apk_helper.py:92: return
manifest_info['manifest']['instrumentation']['android:name'][0]
On 2015/05/05 16:15:01, perezju wrote:
> same here, only this line in the try-except block

Done.

https://codereview.chromium.org/1126543009/diff/60001/components/test/android...
File
components/test/android/browsertests_apk/components_browser_tests_android.cc
(left):

https://codereview.chromium.org/1126543009/diff/60001/components/test/android...
components/test/android/browsertests_apk/components_browser_tests_android.cc:41:
static const char kCommandLineFilePath[] =
On 2015/05/05 23:11:17, Ted C wrote:
> is the test runner now going to pass this in via a command line?

The existing test runner uses /data/local/tmp/chrome-native-tests-command-line
across the board.

The new test runner ("platform mode" in the help text) passes the command-line
flags as an extra to NativeTestInstrumentationTestRunner, which handles passing
them in.

https://codereview.chromium.org/1126543009/diff/60001/components/test/android...
components/test/android/browsertests_apk/components_browser_tests_android.cc:55:
static const char* const kInitialArgv[] = {"ComponentsBrowserTestsActivity"};
On 2015/05/05 23:11:17, Ted C wrote:
> And it looks like this is lost in the conversion to native_test_launcher.cc. 
I
> think now they all share the same kInitialArgv, which I don't actually know if
> it matters just pointing it out.

No, it doesn't. The program name gets ignored.

https://codereview.chromium.org/1126543009/diff/60001/components/test/android...
components/test/android/browsertests_apk/components_browser_tests_android.cc:81:
"components_browsertests_devtools_remote");
On 2015/05/05 23:11:17, Ted C wrote:
> It seems like we lost this (or at least I can't find it).  Same in the other
> instances.

Added to (Content|Components)BrowserTestsActivity.

https://codereview.chromium.org/1126543009/diff/60001/testing/android/native_...
File
testing/android/native_test/java/src/org/chromium/native_test/NativeBrowserTestActivity.java
(right):

https://codereview.chromium.org/1126543009/diff/60001/testing/android/native_...
testing/android/native_test/java/src/org/chromium/native_test/NativeBrowserTestActivity.java:18:
loadLibraries();
On 2015/05/05 23:11:17, Ted C wrote:
> what libraries is it loading here?  this looks backwards to me...shouldn't the
> libraries be loaded before the browser is started?

For both browser tests and unit tests, it just appears to be the one .so. (In
the case of, e.g., content_browsertests, that's libcontent_browsertests.so.)

> 
> The browser starting also will load a bunch of libraries, so is this
duplicated
> work?

It is. The browser tests do this in LibraryLoader.ensureInitialized.

> 
> Looking at the next file it seems like maybe these are the libraries that
> contain the test code (maybe)?  Seems like we should figure out a clearer name
> here since libraries is pretty overloaded.

Moved down to NativeUnitTestActivity.

As it's loading "NativeLibraries.LIBRARIES", I'm not sure how to name it more
specifically...

https://codereview.chromium.org/1126543009/diff/60001/testing/android/native_...
testing/android/native_test/java/src/org/chromium/native_test/NativeBrowserTestActivity.java:21:
protected abstract void startBrowser();
On 2015/05/05 23:11:17, Ted C wrote:
> Add some javadoc here.  Also, does the browser need to be fully running after
> returning from this call?

No. After looking at this more, startBrowser is a misnomer. Renamed to
initializeBrowserProcess, since that more accurately reflects what it does.

https://codereview.chromium.org/1126543009/diff/60001/testing/android/native_...
File
testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java
(right):

https://codereview.chromium.org/1126543009/diff/60001/testing/android/native_...
testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java:116:
protected void loadLibraries() {
On 2015/05/05 23:11:17, Ted C wrote:
> javadoc here as well.

this is now private in NativeUnitTestActivity

https://codereview.chromium.org/1126543009/diff/60001/testing/android/native_...
File testing/android/native_test/native_test_launcher.cc (right):

https://codereview.chromium.org/1126543009/diff/60001/testing/android/native_...
testing/android/native_test/native_test_launcher.cc:124: if
(!base::android::RedirectStream(stdout, stdout_file_path, "w")) {
On 2015/05/05 22:40:34, Jaekyun Seok wrote:
> "w+" is being used for content_browsertests and components_browsertests; you
can
> see the details in https://codereview.chromium.org/1003073002 .

Oh neat, I didn't realize you had done that. Switched.

[Ted C, 2015-05-14 18:07:20]

lgtm

[jbudorick, 2015-05-14 22:43:18]

cjhopman, any thoughts about the gyp/gn in here?

[cjhopman, 2015-05-15 20:11:27]

lgtm

[jbudorick, 2015-05-15 20:57:11]

jbudorick@chromium.org changed reviewers:
+ jochen@chromium.org

[jbudorick, 2015-05-15 20:57:12]

+jochen for owners review for:
 - components/BUILD.gn
 - content/test/BUILD.gn

[jochen (gone - plz use gerrit), 2015-05-19 11:03:12]

lgtm

[jbudorick, 2015-05-19 13:50:20]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-05-19 13:50:20]

The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org
Link to the patchset: https://codereview.chromium.org/1126543009/#ps100001
(title: "removing stray file")

[commit-bot: I haz the power, 2015-05-19 13:50:31]

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

[jbudorick, 2015-05-19 13:54:47]

The CQ bit was unchecked by jbudorick@chromium.org

[jbudorick, 2015-05-19 15:26:22]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-05-19 15:26:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, cjhopman@chromium.org, jochen@chromium.org,
perezju@chromium.org
Link to the patchset: https://codereview.chromium.org/1126543009/#ps120001
(title: "fix gn")

[commit-bot: I haz the power, 2015-05-19 15:26:40]

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

[commit-bot: I haz the power, 2015-05-19 17:09:23]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-19 17:10:14]

Patchset 7 (id:??) landed as
https://crrev.com/581d25e509e4a2b1e8927c6d8accc0c90ea86090
Cr-Commit-Position: refs/heads/master@{#330531}

[jbudorick, 2015-05-19 20:03:30]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1138993009/ by jbudorick@chromium.org.

The reason for reverting is: broke
http://build.chromium.org/p/chromium.mojo/builders/Chromium%20Mojo%20Android

e.g.
http://build.chromium.org/p/chromium.mojo/builders/Chromium%20Mojo%20Android/....