Re-enable WebApkServiceImplTest

Re-enable WebApkServiceImplTest

This CL moves the TestWebApkServiceImplWrapper service out of
chrome_public_test_apk and into a new APK - apk_with_webapk_service.

TestWebApkServiceImplWrapper is launched by the test via Context#bindService().
Due to the super-special-instrumentation-test-ClassLoader a
ClassNotFoundException was being thrown for IWebApkApi.Stub in
TestWebApkServiceImplWrapper#onBind().

apk_with_webapk_service does not use a super-special-ClassLoader.

BUG=634390
TEST=WebApkServiceImplTest.*
NOPRESUBMIT=true

Committed: https://crrev.com/2db0238573cd2cbdaa858d8e51a55baa589df6e6
Cr-Commit-Position: refs/heads/master@{#412292}

[pkotwicz, 2016-08-08 00:59:50]

pkotwicz@chromium.org changed reviewers:
+ smaier@chromium.org

[pkotwicz, 2016-08-08 00:59:50]

smaier@ can you please take a look? Is my CL description accurate?

[pkotwicz, 2016-08-08 01:02:45]

Description was changed from

==========
Re-enable WebApkServiceImplTest

This CL moves the TestWebApkServiceImplWrapper service out of
chrome_public_test_apk and into a new APK - apk_with_webapk_service.

TestWebApkServiceImplWrapper is launched by the test via Context#bindService().
Due to the super-special-instrumentation-test-ClassLoader a
ClassNotFoundException was being thrown for IWebApkApi.Stub in
TestWebApkServiceImplWrapper.

apk_with_webapk_service does not use a super-special-ClassLoader.

BUG=634390
==========

to

==========
Re-enable WebApkServiceImplTest

This CL moves the TestWebApkServiceImplWrapper service out of
chrome_public_test_apk and into a new APK - apk_with_webapk_service.

TestWebApkServiceImplWrapper is launched by the test via Context#bindService().
Due to the super-special-instrumentation-test-ClassLoader a
ClassNotFoundException was being thrown for IWebApkApi.Stub in
TestWebApkServiceImplWrapper#onBind().

apk_with_webapk_service does not use a super-special-ClassLoader.

BUG=634390
==========

[smaier, 2016-08-08 18:34:11]

On 2016/08/08 00:59:50, pkotwicz wrote:
> smaier@ can you please take a look? Is my CL description accurate?

That makes sense - lgtm. Have you run these tests with both is_debug=true and
is_debug=false?

[smaier, 2016-08-08 18:34:23]

https://codereview.chromium.org/2225683002/diff/1/chrome/android/webapk/libs/...
File chrome/android/webapk/libs/runtime_library/javatests/DEPS (right):

https://codereview.chromium.org/2225683002/diff/1/chrome/android/webapk/libs/...
chrome/android/webapk/libs/runtime_library/javatests/DEPS:2: "+base",
If you're already killing the dependency on chrome/test, does it also make sense
to remove the base dependency? It should be okay to remove.

https://codereview.chromium.org/2225683002/diff/1/chrome/android/webapk/libs/...
File
chrome/android/webapk/libs/runtime_library/javatests/src/org/chromium/webapk/lib/runtime_library/WebApkServiceImplTest.java
(right):

https://codereview.chromium.org/2225683002/diff/1/chrome/android/webapk/libs/...
chrome/android/webapk/libs/runtime_library/javatests/src/org/chromium/webapk/lib/runtime_library/WebApkServiceImplTest.java:26:
"org.chromium.webapk.lib.runtime_library.test.TestWebApkServiceImplWrapper";
nit: I'd prefer something along the lines of
TestWebApkServiceImplWrapper.class.getName(), since this won't break if ProGuard
renames this class. Perhaps something similar would be nice for the package name
above, but I'm not sure if that makes sense.

[pkotwicz, 2016-08-09 15:54:09]

pkotwicz@chromium.org changed reviewers:
+ yfriedman@chromium.org

[pkotwicz, 2016-08-09 15:54:09]

Yaron for OWNERS

https://codereview.chromium.org/2225683002/diff/1/chrome/android/webapk/libs/...
File
chrome/android/webapk/libs/runtime_library/javatests/src/org/chromium/webapk/lib/runtime_library/WebApkServiceImplTest.java
(right):

https://codereview.chromium.org/2225683002/diff/1/chrome/android/webapk/libs/...
chrome/android/webapk/libs/runtime_library/javatests/src/org/chromium/webapk/lib/runtime_library/WebApkServiceImplTest.java:26:
"org.chromium.webapk.lib.runtime_library.test.TestWebApkServiceImplWrapper";
Unfortunately, the way this code is written chrome_public_test_apk cannot have
any dependencies on apk_with_webapk_service

Otherwise, we get back to the mess we had before

[Yaron, 2016-08-09 19:12:33]

Seems like a lot of overhead for this one change - what about a special-case
proguard rule? Presumably this is because of renaming things

[smaier, 2016-08-09 19:24:35]

On 2016/08/09 19:12:33, Yaron (back but slow) wrote:
> Seems like a lot of overhead for this one change - what about a special-case
> proguard rule? Presumably this is because of renaming things

This change is required specifically for the tests when they are not being
ProGuarded (works fine with ProGuard right now). We have logic when making our
instrumentation tests with is_debug=true that removes any .dex files from the
test_apk that exist in both the apk_under_test and the test_apk. This works just
fine, since whenever the test_apk looks for any of these classes that sit in the
union of apk_under_test and test_apk, it will first look for the class in the
test_apk, fail, and then fall back on the apk_under_test (which is in its
classloader) to provide a class.

However, the tests in the cl bind to a new service. This new service gets booted
up, and does not get the same classloader as the test_apk. It only has the
test_apk dex files in it, and does not get the apk_under_test's dex files. Thus,
any classes that used to exist in a dex file in both the test_apk and the
under_test_apk will not be found, since the classes in the union only appear in
the apk_under_test.

The reason ProGuard doesn't run into this is because we merge everything in the
apk_under_test directly into the test_apk. Thus, all classes in the union of
test_apk and apk_under_test will exist in the test_apk (which is the only dex
file in the classloader for this bound service).

Let me know if this makes sense.

[Yaron, 2016-08-10 01:23:53]

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

[Yaron, 2016-08-10 01:23:54]

+jbudorick

Yep, makes perfect sense - thanks!
Hmm and IIRC that logic to avoid duplicate classes was because otherwise the
apk-under-test would blow through the method limit because it had all of chrome
code plus some unit tests which put it over. 
Now that we have multidex on for debug, maybe we don't need to drop clases.

Or I'm not remember correctly and possibly BadThings happen if classes are
duplicated?

[jbudorick, 2016-08-10 16:30:21]

On 2016/08/10 01:23:54, Yaron (back but slow) wrote:
> +jbudorick
> 
> Yep, makes perfect sense - thanks!
> Hmm and IIRC that logic to avoid duplicate classes was because otherwise the
> apk-under-test would blow through the method limit because it had all of
chrome
> code plus some unit tests which put it over. 

Well, the test_apk would blow through the method limit. (I think that several of
the apks_under_test are now well over the dex limit on their own.)

> Now that we have multidex on for debug, maybe we don't need to drop clases.
> 
> Or I'm not remember correctly and possibly BadThings happen if classes are
> duplicated?

I think you're right, and I think we'd eventually like to move (back?) to a
state where the test apk doesn't care about an APK under test because it
contains everything it needs to run tests in all cases. The current
configuration -- particularly how we merge the stuff in apk_under_test into
test_apk in release builds but still install everything -- is, imo, a compromise
state that let us realize some gains in our product APKs while not needing to
jump through the various hoops, particularly on the build+infra side, required
to drop to a single (test code + code under test) APK.

[pkotwicz, 2016-08-11 16:59:32]

Yaron ping!

[Yaron, 2016-08-11 17:10:10]

On 2016/08/11 16:59:32, pkotwicz wrote:
> Yaron ping!

So given John's confirmation are you saying that you want to do this
temporarily? I'd prefer we avoided adding yet another apk :(

[pkotwicz, 2016-08-11 18:13:37]

As far as I know there is no way of fixing this test without adding yet another
APK. We could just delete the test if you prefer

[Yaron, 2016-08-12 21:00:03]

On 2016/08/11 18:13:37, pkotwicz wrote:
> As far as I know there is no way of fixing this test without adding yet
another
> APK. We could just delete the test if you prefer

My thinking was that if we fix the issue discussed with him, we could make this
test work without an additional apk.

[Yaron, 2016-08-15 20:26:18]

lgtm - I'm ok with this as a temporary measure but would like to see us follow
up on the multidex. I'll file a bug for that.

[jbudorick, 2016-08-15 20:39:41]

On 2016/08/15 20:26:18, Yaron (limited availability) wrote:
> lgtm - I'm ok with this as a temporary measure but would like to see us follow
> up on the multidex. I'll file a bug for that.

I'm fine with this plan.

[pkotwicz, 2016-08-16 15:05:29]

Description was changed from

==========
Re-enable WebApkServiceImplTest

This CL moves the TestWebApkServiceImplWrapper service out of
chrome_public_test_apk and into a new APK - apk_with_webapk_service.

TestWebApkServiceImplWrapper is launched by the test via Context#bindService().
Due to the super-special-instrumentation-test-ClassLoader a
ClassNotFoundException was being thrown for IWebApkApi.Stub in
TestWebApkServiceImplWrapper#onBind().

apk_with_webapk_service does not use a super-special-ClassLoader.

BUG=634390
==========

to

==========
Re-enable WebApkServiceImplTest

This CL moves the TestWebApkServiceImplWrapper service out of
chrome_public_test_apk and into a new APK - apk_with_webapk_service.

TestWebApkServiceImplWrapper is launched by the test via Context#bindService().
Due to the super-special-instrumentation-test-ClassLoader a
ClassNotFoundException was being thrown for IWebApkApi.Stub in
TestWebApkServiceImplWrapper#onBind().

apk_with_webapk_service does not use a super-special-ClassLoader.

BUG=634390
TEST=WebApkServiceImplTest.*
==========

[pkotwicz, 2016-08-16 15:26:12]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-08-16 15:26:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org, smaier@chromium.org
Link to the patchset: https://codereview.chromium.org/2225683002/#ps20001
(title: "Merge branch 'master' into webapk_disabled_test")

[commit-bot: I haz the power, 2016-08-16 15:26:23]

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-08-16 15:32:27]

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

[commit-bot: I haz the power, 2016-08-16 15:32:28]

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...)

[pkotwicz, 2016-08-16 17:25:53]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-08-16 17:26:07]

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-08-16 17:29:34]

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

[commit-bot: I haz the power, 2016-08-16 17:29:35]

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...)

[pkotwicz, 2016-08-16 17:57:52]

Description was changed from

==========
Re-enable WebApkServiceImplTest

This CL moves the TestWebApkServiceImplWrapper service out of
chrome_public_test_apk and into a new APK - apk_with_webapk_service.

TestWebApkServiceImplWrapper is launched by the test via Context#bindService().
Due to the super-special-instrumentation-test-ClassLoader a
ClassNotFoundException was being thrown for IWebApkApi.Stub in
TestWebApkServiceImplWrapper#onBind().

apk_with_webapk_service does not use a super-special-ClassLoader.

BUG=634390
TEST=WebApkServiceImplTest.*
==========

to

==========
Re-enable WebApkServiceImplTest

This CL moves the TestWebApkServiceImplWrapper service out of
chrome_public_test_apk and into a new APK - apk_with_webapk_service.

TestWebApkServiceImplWrapper is launched by the test via Context#bindService().
Due to the super-special-instrumentation-test-ClassLoader a
ClassNotFoundException was being thrown for IWebApkApi.Stub in
TestWebApkServiceImplWrapper#onBind().

apk_with_webapk_service does not use a super-special-ClassLoader.

BUG=634390
TEST=WebApkServiceImplTest.*
NOPRESUBMIT=1
==========

[pkotwicz, 2016-08-16 17:57:56]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-08-16 17:58:09]

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-08-16 18:05:22]

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

[commit-bot: I haz the power, 2016-08-16 18:05:23]

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...)

[pkotwicz, 2016-08-16 18:05:33]

Description was changed from

==========
Re-enable WebApkServiceImplTest

This CL moves the TestWebApkServiceImplWrapper service out of
chrome_public_test_apk and into a new APK - apk_with_webapk_service.

TestWebApkServiceImplWrapper is launched by the test via Context#bindService().
Due to the super-special-instrumentation-test-ClassLoader a
ClassNotFoundException was being thrown for IWebApkApi.Stub in
TestWebApkServiceImplWrapper#onBind().

apk_with_webapk_service does not use a super-special-ClassLoader.

BUG=634390
TEST=WebApkServiceImplTest.*
NOPRESUBMIT=1
==========

to

==========
Re-enable WebApkServiceImplTest

This CL moves the TestWebApkServiceImplWrapper service out of
chrome_public_test_apk and into a new APK - apk_with_webapk_service.

TestWebApkServiceImplWrapper is launched by the test via Context#bindService().
Due to the super-special-instrumentation-test-ClassLoader a
ClassNotFoundException was being thrown for IWebApkApi.Stub in
TestWebApkServiceImplWrapper#onBind().

apk_with_webapk_service does not use a super-special-ClassLoader.

BUG=634390
TEST=WebApkServiceImplTest.*
NOPRESUBMIT=true
==========

[pkotwicz, 2016-08-16 18:05:37]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-08-16 18:05:51]

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-08-16 18:08:07]

Description was changed from

==========
Re-enable WebApkServiceImplTest

This CL moves the TestWebApkServiceImplWrapper service out of
chrome_public_test_apk and into a new APK - apk_with_webapk_service.

TestWebApkServiceImplWrapper is launched by the test via Context#bindService().
Due to the super-special-instrumentation-test-ClassLoader a
ClassNotFoundException was being thrown for IWebApkApi.Stub in
TestWebApkServiceImplWrapper#onBind().

apk_with_webapk_service does not use a super-special-ClassLoader.

BUG=634390
TEST=WebApkServiceImplTest.*
NOPRESUBMIT=true
==========

to

==========
Re-enable WebApkServiceImplTest

This CL moves the TestWebApkServiceImplWrapper service out of
chrome_public_test_apk and into a new APK - apk_with_webapk_service.

TestWebApkServiceImplWrapper is launched by the test via Context#bindService().
Due to the super-special-instrumentation-test-ClassLoader a
ClassNotFoundException was being thrown for IWebApkApi.Stub in
TestWebApkServiceImplWrapper#onBind().

apk_with_webapk_service does not use a super-special-ClassLoader.

BUG=634390
TEST=WebApkServiceImplTest.*
NOPRESUBMIT=true
==========

[commit-bot: I haz the power, 2016-08-16 18:08:08]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-16 18:09:21]

Description was changed from

==========
Re-enable WebApkServiceImplTest

This CL moves the TestWebApkServiceImplWrapper service out of
chrome_public_test_apk and into a new APK - apk_with_webapk_service.

TestWebApkServiceImplWrapper is launched by the test via Context#bindService().
Due to the super-special-instrumentation-test-ClassLoader a
ClassNotFoundException was being thrown for IWebApkApi.Stub in
TestWebApkServiceImplWrapper#onBind().

apk_with_webapk_service does not use a super-special-ClassLoader.

BUG=634390
TEST=WebApkServiceImplTest.*
NOPRESUBMIT=true
==========

to

==========
Re-enable WebApkServiceImplTest

This CL moves the TestWebApkServiceImplWrapper service out of
chrome_public_test_apk and into a new APK - apk_with_webapk_service.

TestWebApkServiceImplWrapper is launched by the test via Context#bindService().
Due to the super-special-instrumentation-test-ClassLoader a
ClassNotFoundException was being thrown for IWebApkApi.Stub in
TestWebApkServiceImplWrapper#onBind().

apk_with_webapk_service does not use a super-special-ClassLoader.

BUG=634390
TEST=WebApkServiceImplTest.*
NOPRESUBMIT=true

Committed: https://crrev.com/2db0238573cd2cbdaa858d8e51a55baa589df6e6
Cr-Commit-Position: refs/heads/master@{#412292}
==========

[commit-bot: I haz the power, 2016-08-16 18:09:23]

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