Use Sync FakeServer in Android tests via custom APK

Create ChromeSyncShell and ChromeSyncShellTest

This CL enables usage of the Sync FakeServer in Android tests. This is
done by creating a new ChromeShell-based APK, ChromeSyncShell, and a
test APK that instruments it (ChromeSyncShellTest).

As part of this CL, previously-disabled tests (SyncTest.java) are
re-enabled. However, these tests will not run as part of the Chromium
waterfall/trybots as they are not configured to run ChromeSyncShellTest.

How to run the tests:
1) Build these targets:
   chrome_sync_shell_apk
   chrome_sync_shell_test_apk

2) Run test script
   build/android/adb_install_apk.py \
   --apk=ChromeSyncShell.apk && \
   build/android/test_runner.py instrumentation \
   --test-apk ChromeSyncShellTest

BUG=323265, 348951, 348117
TBR=cpu@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291190

[pval...(no longer on Chromium), 2014-08-09 00:48:22]

I've created a new issue for this.

As of mailing, this CL works in that ChromeShellForTest.apk is a separate
ChromeShell-based APK that contains test code. However, there are a few
remaining issues (mostly naming and code location I hope) that need to be
resolved. Please take a look at the comments I have made already.

When we're getting close to committing, I'll beef up the description.

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/DEPS
File chrome/android/shell/DEPS (right):

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/DEP...
chrome/android/shell/DEPS:12: "+sync/test/fake_server/android",
I'm not sure where this code (fake_server_android.{h,cc} exist in
sync/test/fake_server/android) should go. They need to depend on the base
FakeServer target (test_support_sync_fake_server).

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/for...
File chrome/android/shell/fortest/AndroidManifest.xml (right):

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/for...
chrome/android/shell/fortest/AndroidManifest.xml:10:
package="org.chromium.chrome.shellfortest">
I blindly copied the the manifest file from
chrome/android/shell/java/AndroidManifest.xml and made the appropriate
modifications here.

How should I proceed with this file? Also, where should it go?

https://codereview.chromium.org/457883002/diff/40001/chrome/chrome_android.gypi
File chrome/chrome_android.gypi (left):

https://codereview.chromium.org/457883002/diff/40001/chrome/chrome_android.gy...
chrome/chrome_android.gypi:16: 'chrome_android_paks.gypi', # Included for the
list of pak resources.
This can't be included here anymore because two APKs use it (see
chrome_shell.gypi) and it uses the package_name variable.

https://codereview.chromium.org/457883002/diff/40001/chrome/chrome_shell.gypi
File chrome/chrome_shell.gypi (right):

https://codereview.chromium.org/457883002/diff/40001/chrome/chrome_shell.gypi...
chrome/chrome_shell.gypi:91: 'manifest_package_name':
'org.chromium.chrome.shell',
Is this variable used anywhere? I can't find any usages of it.

https://codereview.chromium.org/457883002/diff/40001/chrome/chrome_shell.gypi...
chrome/chrome_shell.gypi:124: 'target_name': 'chrome_shell_for_test_apk',
I really don't like the name for this APK (ChromeShellForTest).

Any ideas on a good name?

[rlarocque, 2014-08-09 01:20:00]

It's not pretty, but I don't think there's much else you could have done given
the circumstances.

I'm sure Yaron will have a few questions.  Feel free to invite me to the
conversation if you end up discussing it over VC.

https://codereview.chromium.org/457883002/diff/40001/chrome/android/javatests...
File chrome/android/javatests/src/org/chromium/chrome/browser/sync/SyncTest.java
(right):

https://codereview.chromium.org/457883002/diff/40001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/sync/SyncTest.java:74:
ThreadUtils.runOnUiThreadBlocking(new Runnable() {
tearDown() is not called on the UI thread?  What thread do these unit tests run
on?  Is it safe for this thread to block during sync shutdown?

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/DEPS
File chrome/android/shell/DEPS (right):

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/DEP...
chrome/android/shell/DEPS:12: "+sync/test/fake_server/android",
On 2014/08/09 00:48:21, pvalenzuela wrote:
> I'm not sure where this code (fake_server_android.{h,cc} exist in
> sync/test/fake_server/android) should go. They need to depend on the base
> FakeServer target (test_support_sync_fake_server).

I think the tendency is to not put it in a directory. 
sync/test/fake_server/fake_serve_android.cc would be my guess as to where it
belongs.

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/chr...
File chrome/android/shell/chrome_main_delegate_chrome_shell_for_test_android.cc
(right):

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/chr...
chrome/android/shell/chrome_main_delegate_chrome_shell_for_test_android.cc:29:
return ChromeMainDelegateAndroid::RegisterApplicationNativeMethods(env) &&
I noticed the caller
(chromium/src/chrome/app/android/chrome_main_delegate_android.cc) ignores the
return value.

Not sure if that matters.  It does make your use of && seem a bit out of place,
though.

https://codereview.chromium.org/457883002/diff/40001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/457883002/diff/40001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_android.cc:486:
make_scoped_ptr<syncer::NetworkResources>(resources));
This is bad.  We might be able to get away with it, though.

Normally I'd be worried about these sorts of ownership handoffs.  How do we make
sure this memory gets freed, even if the caller (ie. Java) throws an exception
or otherwise gets into a bad state?

Since this is test code, I'm willing to leave it as is.

[pval...(no longer on Chromium), 2014-08-12 01:21:36]

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/DEPS
File chrome/android/shell/DEPS (right):

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/DEP...
chrome/android/shell/DEPS:12: "+sync/test/fake_server/android",
On 2014/08/09 01:19:59, rlarocque wrote:
> On 2014/08/09 00:48:21, pvalenzuela wrote:
> > I'm not sure where this code (fake_server_android.{h,cc} exist in
> > sync/test/fake_server/android) should go. They need to depend on the base
> > FakeServer target (test_support_sync_fake_server).
> 
> I think the tendency is to not put it in a directory. 
> sync/test/fake_server/fake_serve_android.cc would be my guess as to where it
> belongs.

This would introduce a dependency on a test directory though. I imagine this is
not a preferred situation.

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/chr...
File chrome/android/shell/chrome_main_delegate_chrome_shell_for_test_android.cc
(right):

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/chr...
chrome/android/shell/chrome_main_delegate_chrome_shell_for_test_android.cc:29:
return ChromeMainDelegateAndroid::RegisterApplicationNativeMethods(env) &&
On 2014/08/09 01:19:59, rlarocque wrote:
> I noticed the caller
> (chromium/src/chrome/app/android/chrome_main_delegate_android.cc) ignores the
> return value.
> 
> Not sure if that matters.  It does make your use of && seem a bit out of
place,
> though.

I feel like what I did here is in the spirit of how the function should work
(return true iff registration succeeded)

https://codereview.chromium.org/457883002/diff/40001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/457883002/diff/40001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_android.cc:486:
make_scoped_ptr<syncer::NetworkResources>(resources));
On 2014/08/09 01:20:00, rlarocque wrote:
> This is bad.  We might be able to get away with it, though.
> 
> Normally I'd be worried about these sorts of ownership handoffs.  How do we
make
> sure this memory gets freed, even if the caller (ie. Java) throws an exception
> or otherwise gets into a bad state?
> 
> Since this is test code, I'm willing to leave it as is.

I don't have a solution for this. We probably have to live with it.

[Yaron, 2014-08-12 17:22:04]

https://codereview.chromium.org/457883002/diff/40001/chrome/android/javatests...
File chrome/android/javatests/src/org/chromium/chrome/browser/sync/SyncTest.java
(right):

https://codereview.chromium.org/457883002/diff/40001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/sync/SyncTest.java:74:
ThreadUtils.runOnUiThreadBlocking(new Runnable() {
On 2014/08/09 01:19:59, rlarocque wrote:
> tearDown() is not called on the UI thread?  What thread do these unit tests
run
> on?  Is it safe for this thread to block during sync shutdown?

These are called on a separate test thread (android tests by default run on this
thread unless you annotate the test with @UIThreadTest).

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/DEPS
File chrome/android/shell/DEPS (right):

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/DEP...
chrome/android/shell/DEPS:12: "+sync/test/fake_server/android",
On 2014/08/12 01:21:35, pvalenzuela wrote:
> On 2014/08/09 01:19:59, rlarocque wrote:
> > On 2014/08/09 00:48:21, pvalenzuela wrote:
> > > I'm not sure where this code (fake_server_android.{h,cc} exist in
> > > sync/test/fake_server/android) should go. They need to depend on the base
> > > FakeServer target (test_support_sync_fake_server).
> > 
> > I think the tendency is to not put it in a directory. 
> > sync/test/fake_server/fake_serve_android.cc would be my guess as to where it
> > belongs.
> 
> This would introduce a dependency on a test directory though. I imagine this
is
> not a preferred situation.

I think we should keep these changes in a separate package/folder that depends
on chrome shell.
So move all of the new code to:
chrome/android/sync_test_shell ? (strawman) and you can add that dep there.

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/chr...
File chrome/android/shell/chrome_main_delegate_chrome_shell_for_test_android.cc
(right):

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/chr...
chrome/android/shell/chrome_main_delegate_chrome_shell_for_test_android.cc:29:
return ChromeMainDelegateAndroid::RegisterApplicationNativeMethods(env) &&
On 2014/08/12 01:21:35, pvalenzuela wrote:
> On 2014/08/09 01:19:59, rlarocque wrote:
> > I noticed the caller
> > (chromium/src/chrome/app/android/chrome_main_delegate_android.cc) ignores
the
> > return value.
> > 
It probably shouldn't. :(
 
> > Not sure if that matters.  It does make your use of && seem a bit out of
> place,
> > though.
> 
> I feel like what I did here is in the spirit of how the function should work
> (return true iff registration succeeded)

Agreed

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/chr...
File chrome/android/shell/chrome_main_delegate_chrome_shell_for_test_android.h
(right):

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/chr...
chrome/android/shell/chrome_main_delegate_chrome_shell_for_test_android.h:10:
class ChromeMainDelegateChromeShellForTestAndroid
move and rename per above.

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/for...
File chrome/android/shell/fortest/AndroidManifest.xml (right):

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/for...
chrome/android/shell/fortest/AndroidManifest.xml:10:
package="org.chromium.chrome.shellfortest">
On 2014/08/09 00:48:21, pvalenzuela wrote:
> I blindly copied the the manifest file from
> chrome/android/shell/java/AndroidManifest.xml and made the appropriate
> modifications here.
> 
> How should I proceed with this file? Also, where should it go?

Yes, copy is unfortunately the way to go. For a while we had a manifest merger
working but it's currently unsupported. Please in
chrome/android/shell/sync_test_shell/java/

https://codereview.chromium.org/457883002/diff/40001/chrome/chrome_shell.gypi
File chrome/chrome_shell.gypi (right):

https://codereview.chromium.org/457883002/diff/40001/chrome/chrome_shell.gypi...
chrome/chrome_shell.gypi:91: 'manifest_package_name':
'org.chromium.chrome.shell',
On 2014/08/09 00:48:21, pvalenzuela wrote:
> Is this variable used anywhere? I can't find any usages of it.

Looks to be dead code.

https://codereview.chromium.org/457883002/diff/40001/chrome/chrome_shell.gypi...
chrome/chrome_shell.gypi:104: 'target_name': 'chrome_shell_paks',
Why do you have two separate paks targets? Seems like it can be shared.

[rlarocque, 2014-08-12 22:02:27]

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/chr...
File chrome/android/shell/chrome_main_delegate_chrome_shell_for_test_android.cc
(right):

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/chr...
chrome/android/shell/chrome_main_delegate_chrome_shell_for_test_android.cc:29:
return ChromeMainDelegateAndroid::RegisterApplicationNativeMethods(env) &&
On 2014/08/12 01:21:35, pvalenzuela wrote:
> On 2014/08/09 01:19:59, rlarocque wrote:
> > I noticed the caller
> > (chromium/src/chrome/app/android/chrome_main_delegate_android.cc) ignores
the
> > return value.
> > 
> > Not sure if that matters.  It does make your use of && seem a bit out of
> place,
> > though.
> 
> I feel like what I did here is in the spirit of how the function should work
> (return true iff registration succeeded)

It also leaves things half-initialized if the second call fails.  That's the
part that seems bad, but probably doesn't matter.

If it did happen, it would probably result in a fatal error sooner or later. 
But it wouldn't be caught by the caller, since it's not checking the return
value.

[pval...(no longer on Chromium), 2014-08-14 00:35:39]

I've addressed some of the comments. PTAL.

Some remaining issues to resolve:

1) My first trybot runs for this CL failed because the bots don't know about the
new APK. Is there any way I can make this switch in one CL? It looks like the
files I need to change are in
tools/build/scripts/slave/recipe_modules/chromium/chromium_linux.py

2) Should FakeServerHelper.java be moved to the chrome/android/shell_for_test
dir? This file is unique in that it needs to be included by
ChromeShellForTest.apk (so it doesn't crash on startup) and ChromeShellTest.apk
(so it can be used in tests).

3) Speaking of the ChromeShellForTest.apk naming, any additional ideas? I feel
like this might be the least bad option.

4) There are lint failures (presumably disabled for ChromeShell?) that are
simply a result of another target depending on some of the code. How can/Should
I suppress these?

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/DEPS
File chrome/android/shell/DEPS (right):

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/DEP...
chrome/android/shell/DEPS:12: "+sync/test/fake_server/android",
On 2014/08/12 17:22:03, Yaron wrote:
> On 2014/08/12 01:21:35, pvalenzuela wrote:
> > On 2014/08/09 01:19:59, rlarocque wrote:
> > > On 2014/08/09 00:48:21, pvalenzuela wrote:
> > > > I'm not sure where this code (fake_server_android.{h,cc} exist in
> > > > sync/test/fake_server/android) should go. They need to depend on the
base
> > > > FakeServer target (test_support_sync_fake_server).
> > > 
> > > I think the tendency is to not put it in a directory. 
> > > sync/test/fake_server/fake_serve_android.cc would be my guess as to where
it
> > > belongs.
> > 
> > This would introduce a dependency on a test directory though. I imagine this
> is
> > not a preferred situation.
> 
> I think we should keep these changes in a separate package/folder that depends
> on chrome shell.
> So move all of the new code to:
> chrome/android/sync_test_shell ? (strawman) and you can add that dep there.

Done.

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/chr...
File chrome/android/shell/chrome_main_delegate_chrome_shell_for_test_android.cc
(right):

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/chr...
chrome/android/shell/chrome_main_delegate_chrome_shell_for_test_android.cc:29:
return ChromeMainDelegateAndroid::RegisterApplicationNativeMethods(env) &&
On 2014/08/12 22:02:27, rlarocque wrote:
> On 2014/08/12 01:21:35, pvalenzuela wrote:
> > On 2014/08/09 01:19:59, rlarocque wrote:
> > > I noticed the caller
> > > (chromium/src/chrome/app/android/chrome_main_delegate_android.cc) ignores
> the
> > > return value.
> > > 
> > > Not sure if that matters.  It does make your use of && seem a bit out of
> > place,
> > > though.
> > 
> > I feel like what I did here is in the spirit of how the function should work
> > (return true iff registration succeeded)
> 
> It also leaves things half-initialized if the second call fails.  That's the
> part that seems bad, but probably doesn't matter.
> 
> If it did happen, it would probably result in a fatal error sooner or later. 
> But it wouldn't be caught by the caller, since it's not checking the return
> value.

I think we should keep this code as-is and add a TODO/file a bug for actually
using the return value. How does that sound?

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/chr...
File chrome/android/shell/chrome_main_delegate_chrome_shell_for_test_android.h
(right):

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/chr...
chrome/android/shell/chrome_main_delegate_chrome_shell_for_test_android.h:10:
class ChromeMainDelegateChromeShellForTestAndroid
On 2014/08/12 17:22:03, Yaron wrote:
> move and rename per above. 

Moved, but kept the name (for now) since we haven't decided on a good name.

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/for...
File chrome/android/shell/fortest/AndroidManifest.xml (right):

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/for...
chrome/android/shell/fortest/AndroidManifest.xml:10:
package="org.chromium.chrome.shellfortest">
On 2014/08/12 17:22:03, Yaron wrote:
> On 2014/08/09 00:48:21, pvalenzuela wrote:
> > I blindly copied the the manifest file from
> > chrome/android/shell/java/AndroidManifest.xml and made the appropriate
> > modifications here.
> > 
> > How should I proceed with this file? Also, where should it go?
> 
> Yes, copy is unfortunately the way to go. For a while we had a manifest merger
> working but it's currently unsupported. Please in
> chrome/android/shell/sync_test_shell/java/

Moved

https://codereview.chromium.org/457883002/diff/40001/chrome/chrome_android.gypi
File chrome/chrome_android.gypi (left):

https://codereview.chromium.org/457883002/diff/40001/chrome/chrome_android.gy...
chrome/chrome_android.gypi:16: 'chrome_android_paks.gypi', # Included for the
list of pak resources.
On 2014/08/09 00:48:21, pvalenzuela wrote:
> This can't be included here anymore because two APKs use it (see
> chrome_shell.gypi) and it uses the package_name variable.

I've added this back as we can just reuse the resources from ChromeShell.apk

https://codereview.chromium.org/457883002/diff/40001/chrome/chrome_shell.gypi
File chrome/chrome_shell.gypi (right):

https://codereview.chromium.org/457883002/diff/40001/chrome/chrome_shell.gypi...
chrome/chrome_shell.gypi:104: 'target_name': 'chrome_shell_paks',
On 2014/08/12 17:22:04, Yaron wrote:
> Why do you have two separate paks targets? Seems like it can be shared.

ah, good point. fixed.

[rlarocque, 2014-08-14 00:44:02]

I don't have much more to offer on this review.  I'll stick around and comment
as necessary, but I'll hand off the rights to approve to the one reviewer who
knows things about Android.

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/chr...
File chrome/android/shell/chrome_main_delegate_chrome_shell_for_test_android.cc
(right):

https://codereview.chromium.org/457883002/diff/40001/chrome/android/shell/chr...
chrome/android/shell/chrome_main_delegate_chrome_shell_for_test_android.cc:29:
return ChromeMainDelegateAndroid::RegisterApplicationNativeMethods(env) &&
On 2014/08/14 00:35:39, pvalenzuela wrote:
> On 2014/08/12 22:02:27, rlarocque wrote:
> > On 2014/08/12 01:21:35, pvalenzuela wrote:
> > > On 2014/08/09 01:19:59, rlarocque wrote:
> > > > I noticed the caller
> > > > (chromium/src/chrome/app/android/chrome_main_delegate_android.cc)
ignores
> > the
> > > > return value.
> > > > 
> > > > Not sure if that matters.  It does make your use of && seem a bit out of
> > > place,
> > > > though.
> > > 
> > > I feel like what I did here is in the spirit of how the function should
work
> > > (return true iff registration succeeded)
> > 
> > It also leaves things half-initialized if the second call fails.  That's the
> > part that seems bad, but probably doesn't matter.
> > 
> > If it did happen, it would probably result in a fatal error sooner or later.

> > But it wouldn't be caught by the caller, since it's not checking the return
> > value.
> 
> I think we should keep this code as-is and add a TODO/file a bug for actually
> using the return value. How does that sound?

SGTM

[Yaron, 2014-08-14 22:49:30]

https://codereview.chromium.org/457883002/diff/60001/chrome/android/javatests...
File chrome/android/javatests/src/org/chromium/chrome/browser/sync/SyncTest.java
(right):

https://codereview.chromium.org/457883002/diff/60001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/sync/SyncTest.java:170:
// unncessary once we have other enabled tests.
So do the other tests work? Can you remove this?

https://codereview.chromium.org/457883002/diff/60001/chrome/android/shell/jav...
File chrome/android/shell/javatests/AndroidManifest.xml (right):

https://codereview.chromium.org/457883002/diff/60001/chrome/android/shell/jav...
chrome/android/shell/javatests/AndroidManifest.xml:24:
android:targetPackage="org.chromium.chrome.shellfortest"
This changes all instrumentation tests to use your new test apk. What if you
move SyncTest.py into chrome/android/shell_for_test/java/. I think you can add
an <instrumentation> declaration to that apk and have it test itself.

https://codereview.chromium.org/457883002/diff/60001/chrome/android/shell_for...
File chrome/android/shell_for_test/DEPS (right):

https://codereview.chromium.org/457883002/diff/60001/chrome/android/shell_for...
chrome/android/shell_for_test/DEPS:1: include_rules = [
Can you add a README in chrome/android/shell_for_test briefly describing it and
the difference from chrome/android/shell

https://codereview.chromium.org/457883002/diff/60001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/457883002/diff/60001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:2765: 'chrome_shell_for_test_apk_java',
This won't be necessary if my other suggestion works

https://codereview.chromium.org/457883002/diff/60001/chrome/test/android/java...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java
(right):

https://codereview.chromium.org/457883002/diff/60001/chrome/test/android/java...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java:407:
public static void useFakeServer(final Context context) {
This probably shouldn't be in this class because it wouldn't work when tests run
against the regular chromeshell

[pval...(no longer on Chromium), 2014-08-15 19:12:41]

Yaron,

I think I had a misunderstanding in the plan here. My direction was creating a
new ChromeShell-like APK w/ test libraries built in that all instrumentation
tests would use (as you pointed out) via ChromeShellTest. However, I do think
that your suggestion of a single APK for ChromeShell+test hooks+tests for Sync
is a good idea. Let's go in that direction.

I've been trying to put that solution together this morning but am struggling to
do so. How should this new APK look in terms of which directories it includes
sources from? Specifically, what should we set for the java_in_dir and
additional_src_dirs GYP variables?

Any other variables that might need to be set?

Another confusing snag I hit was that we can't use the is_test_apk = 1 GYP
variable since it suppresses resource inclusion in the APK (but in *this* test
APK, we need them). I think simply not setting that variable does the trick
here.

Thanks,
-Patrick.

[Yaron, 2014-08-15 20:09:46]

On Fri, Aug 15, 2014 at 12:12 PM, <pvalenzuela@chromium.org> wrote:

> Yaron,
>
> I think I had a misunderstanding in the plan here. My direction was
> creating a
> new ChromeShell-like APK w/ test libraries built in that all
> instrumentation
> tests would use (as you pointed out) via ChromeShellTest. However, I do
> think
> that your suggestion of a single APK for ChromeShell+test hooks+tests for
> Sync
> is a good idea. Let's go in that direction.
>
> Sorry, ya. I kind of realized it along the way, I think it'll be simpler.
Glad to hear we agree.


> I've been trying to put that solution together this morning but am
> struggling to
> do so. How should this new APK look in terms of which directories it
> includes
> sources from? Specifically, what should we set for the java_in_dir and
> additional_src_dirs GYP variables?
>

I think it should include only chrome/android/shell_for_test. The rest
would be picked up through dependencies, right?

>
> Any other variables that might need to be set?
>
> Another confusing snag I hit was that we can't use the is_test_apk = 1 GYP
> variable since it suppresses resource inclusion in the APK (but in *this*
> test
> APK, we need them). I think simply not setting that variable does the trick
> here.
>
Ya, I think you're right on this one.

>
> Thanks,
> -Patrick.
>
>
>
> https://codereview.chromium.org/457883002/
>

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

[pval...(no longer on Chromium), 2014-08-19 04:01:12]

+cpu and +stevet for OWNERS approval for the DEPS change in
chrome/android/sync_shell/DEPS. The sync_shell directory is a very similar
solution to chrome/android/shell, but we need a new DEPS file here.

yfriedman and rlarocque, 

PTAL.

I have modified this CL to create ChromeSyncShell (and ChromeSyncShellTest). I
could not get the single APK approach (ChromeSyncShell instruments and tests
itself) working, so this is the next best approach to get something checked in
ASAP.

Notes and questions:
1) ChromeShellUrlTest is included in the new test APK due to its code path. This
adds three sanity tests to the suite. Is it ok to leave it here? I feel like
it's nice to have some smoke tests for ChromeSyncShell.

2) Where should I add a TODO for running these tests on waterfall/trybot?

3) Let me know if there are any technical details you'd like me to add in the
sync_shell README file.

https://codereview.chromium.org/457883002/diff/60001/chrome/android/javatests...
File chrome/android/javatests/src/org/chromium/chrome/browser/sync/SyncTest.java
(right):

https://codereview.chromium.org/457883002/diff/60001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/sync/SyncTest.java:170:
// unncessary once we have other enabled tests.
On 2014/08/14 22:49:30, Yaron wrote:
> So do the other tests work? Can you remove this?

removed as of patchset 3

https://codereview.chromium.org/457883002/diff/60001/chrome/android/shell_for...
File chrome/android/shell_for_test/DEPS (right):

https://codereview.chromium.org/457883002/diff/60001/chrome/android/shell_for...
chrome/android/shell_for_test/DEPS:1: include_rules = [
On 2014/08/14 22:49:30, Yaron wrote:
> Can you add a README in chrome/android/shell_for_test briefly describing it
and
> the difference from chrome/android/shell

https://codereview.chromium.org/457883002/diff/60001/chrome/test/android/java...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java
(right):

https://codereview.chromium.org/457883002/diff/60001/chrome/test/android/java...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java:407:
public static void useFakeServer(final Context context) {
On 2014/08/14 22:49:30, Yaron wrote:
> This probably shouldn't be in this class because it wouldn't work when tests
run
> against the regular chromeshell

I've moved this file to chrome/android/sync_shell/javatests because it is only
used by SyncTest.java and those tests only run against ChromeSyncShell. This
solves the problem that motivated your comment.

https://codereview.chromium.org/457883002/diff/140001/chrome/android/sync_she...
File chrome/android/sync_shell/README (right):

https://codereview.chromium.org/457883002/diff/140001/chrome/android/sync_she...
chrome/android/sync_shell/README:1: ChromeSyncShell (and ChromeSyncShellTest)
exists because we don't want to
Please let me know if there are any particular items that you'd like me to note
here.

[SteveT, 2014-08-19 12:36:51]

Sorry - don't think I am an OWNER for any of these things.

[rlarocque, 2014-08-19 18:04:19]

https://codereview.chromium.org/457883002/diff/140001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestUtil.java
(right):

https://codereview.chromium.org/457883002/diff/140001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestUtil.java:5:
package org.chromium.chrome.browser.sync;
Just making sure I understand this:

This code used to be in the test APK, ChromeShellTest.apk.  Now it's being moved
in to Chrome Shell + Fake Server APK, which is ChromeSyncShell.apk.  Is that
correct?

https://codereview.chromium.org/457883002/diff/140001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/457883002/diff/140001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:2770: # move?
Remove this comment before committing?

And, to answer your question, I don't think the tests belong in either place.

These tests include:
./android/javatests/src/org/chromium/sync/notifier/InvalidationPreferencesTest.java
./android/javatests/src/org/chromium/sync/notifier/InvalidationClientNameProviderTest.java
./android/javatests/src/org/chromium/sync/notifier/TestableInvalidationService.java
./android/javatests/src/org/chromium/sync/notifier/signin/SyncStatusHelperTest.java
./android/javatests/src/org/chromium/sync/notifier/InvalidationServiceTest.java
./test/android/javatests/src/org/chromium/sync/test/util/MockGrantCredentialsPermissionActivity.java
./test/android/javatests/src/org/chromium/sync/test/util/MockSyncContentResolverDelegate.java
./test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java
./test/android/javatests/src/org/chromium/sync/test/util/AccountHolder.java

On non-Android, the invalidations and signin components have been pulled out of
sync.  There's no reason why the Android tests for these things should remain
part of sync.

[pval...(no longer on Chromium), 2014-08-19 18:13:25]

On 2014/08/19 12:36:51, SteveT wrote:
> Sorry - don't think I am an OWNER for any of these things.

@stevet: you're listed as an owner in
https://code.google.com/p/chromium/codesearch#chromium/src/components/search_...
and I added a DEPS addition which adds that directory (requires approval). Shall
I add pkasting@ instead?

[SteveT, 2014-08-19 18:36:34]

Oh I see that now. Sorry, was looking around for the directory itself earlier.

LGTM for that specific DEPS change.

[pval...(no longer on Chromium), 2014-08-19 18:43:22]

https://codereview.chromium.org/457883002/diff/140001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestUtil.java
(right):

https://codereview.chromium.org/457883002/diff/140001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestUtil.java:5:
package org.chromium.chrome.browser.sync;
On 2014/08/19 18:04:19, rlarocque wrote:
> Just making sure I understand this:
> 
> This code used to be in the test APK, ChromeShellTest.apk.  Now it's being
moved
> in to Chrome Shell + Fake Server APK, which is ChromeSyncShell.apk (the test
APK that instruments the new ChromeSyncShell).  Is that
> correct?

It used to be in ChromeShellTest.apk and now it is in ChromeSyncShellTest.apk.

The APK that it's included in is decided by the GYP definitions for each APK. 

The code paths line up like this (modeled after ChromeShell).
ChromeSyncShell: chrome/android/sync_shell/java
ChromeSyncShellTest: chrome/android/sync_shell/javatests 

As for the Java package, I moved it to be next to SyncTest.java for convenience
(also it's only used there).

https://codereview.chromium.org/457883002/diff/140001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/457883002/diff/140001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:2770: # move?
On 2014/08/19 18:04:19, rlarocque wrote:
> Remove this comment before committing?
> 
> And, to answer your question, I don't think the tests belong in either place.
> 
> These tests include:
>
./android/javatests/src/org/chromium/sync/notifier/InvalidationPreferencesTest.java
>
./android/javatests/src/org/chromium/sync/notifier/InvalidationClientNameProviderTest.java
>
./android/javatests/src/org/chromium/sync/notifier/TestableInvalidationService.java
>
./android/javatests/src/org/chromium/sync/notifier/signin/SyncStatusHelperTest.java
>
./android/javatests/src/org/chromium/sync/notifier/InvalidationServiceTest.java
>
./test/android/javatests/src/org/chromium/sync/test/util/MockGrantCredentialsPermissionActivity.java
>
./test/android/javatests/src/org/chromium/sync/test/util/MockSyncContentResolverDelegate.java
>
./test/android/javatests/src/org/chromium/sync/test/util/MockAccountManager.java
> ./test/android/javatests/src/org/chromium/sync/test/util/AccountHolder.java
> 
> On non-Android, the invalidations and signin components have been pulled out
of
> sync.  There's no reason why the Android tests for these things should remain
> part of sync.

Removed; thanks for catching this comment.

I haven't touched any of the tests you listed. They will continue to run in
ChromeShellTest.apk as they do today. I think you concern has more to do with
the packages of files like InvalidationPreferencesTest. We can resolve this in
another CL at a later point.

[pval...(no longer on Chromium), 2014-08-19 19:08:00]

FYI

We had some discussions recently about how to get this CL in quicker before I go
out of office for a while. The consensus opinion was submit the
ChromeShell+FakeServer APK (now called ChromeSyncShell) ASAP and leave the tests
for later.

It turned out to be really easy to add the test APK (ChromeSyncShellTest), so I
did that here. I felt it was easier than teasing apart the test code and leaving
it in an unsubmitted CL.

[Yaron, 2014-08-19 21:21:22]

I think we're close. One issue is that I don't believe these apks will be
installed/used by the bots. We'll have to follow-up with infra to add them.

https://codereview.chromium.org/457883002/diff/160001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestUtil.java
(right):

https://codereview.chromium.org/457883002/diff/160001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestUtil.java:5:
package org.chromium.chrome.browser.sync;
This can't all move (at least not yet). It's used by the internal bots for some
sync tests. When we've defined the downstream equivalent to this ChromeSyncShell
then it can mvoe. For now, can you move the FakeServer code to FakeSErver.java
and leave this here?

[Yaron, 2014-08-19 21:23:19]

https://codereview.chromium.org/457883002/diff/160001/chrome/android/sync_she...
File chrome/android/sync_shell/DEPS (right):

https://codereview.chromium.org/457883002/diff/160001/chrome/android/sync_she...
chrome/android/sync_shell/DEPS:1: include_rules = [
let's add an OWNERS file too here. That way if someone tries to add a dep we can
verify it's kosher (i.e chromeshell shouldn't depend on these)

[pval...(no longer on Chromium), 2014-08-20 01:55:09]

PTAL

https://codereview.chromium.org/457883002/diff/160001/chrome/android/sync_she...
File chrome/android/sync_shell/DEPS (right):

https://codereview.chromium.org/457883002/diff/160001/chrome/android/sync_she...
chrome/android/sync_shell/DEPS:1: include_rules = [
On 2014/08/19 21:23:19, Yaron wrote:
> let's add an OWNERS file too here. That way if someone tries to add a dep we
can
> verify it's kosher (i.e chromeshell shouldn't depend on these)

done. I've added you and zea (for sync stuff)

https://codereview.chromium.org/457883002/diff/160001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestUtil.java
(right):

https://codereview.chromium.org/457883002/diff/160001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestUtil.java:5:
package org.chromium.chrome.browser.sync;
On 2014/08/19 21:21:21, Yaron wrote:
> This can't all move (at least not yet). It's used by the internal bots for
some
> sync tests. When we've defined the downstream equivalent to this
ChromeSyncShell
> then it can mvoe. For now, can you move the FakeServer code to FakeSErver.java
> and leave this here?

Done. I have also moved FakeServerHelper so that it is only contained in
ChromeSyncShellTest

[Yaron, 2014-08-20 05:06:25]

lgtm

https://codereview.chromium.org/457883002/diff/180001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/FakeServerHelper.java
(right):

https://codereview.chromium.org/457883002/diff/180001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/FakeServerHelper.java:31:
if (sFakeServerHelper == null) {
This should probably assert on ui thread. (you are initing it there anyway). You
only need to update deleteFakeServer to also use ui thread.

[pval...(no longer on Chromium), 2014-08-20 18:20:42]

https://codereview.chromium.org/457883002/diff/180001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/FakeServerHelper.java
(right):

https://codereview.chromium.org/457883002/diff/180001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/FakeServerHelper.java:31:
if (sFakeServerHelper == null) {
On 2014/08/20 05:06:25, Yaron wrote:
> This should probably assert on ui thread. (you are initing it there anyway).
You
> only need to update deleteFakeServer to also use ui thread.

Done.

[rlarocque, 2014-08-20 18:23:27]

lgtm

[pval...(no longer on Chromium), 2014-08-20 19:23:57]

The CQ bit was checked by pvalenzuela@chromium.org

[commit-bot: I haz the power, 2014-08-20 19:25:42]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/pvalenzuela@chromium.org/457883002/220001

[commit-bot: I haz the power, 2014-08-20 21:27:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-20 21:30:12]

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

[commit-bot: I haz the power, 2014-08-20 21:30:13]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[pval...(no longer on Chromium), 2014-08-20 21:50:10]

The CQ bit was checked by pvalenzuela@chromium.org

[commit-bot: I haz the power, 2014-08-20 21:52:36]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/pvalenzuela@chromium.org/457883002/240001

[commit-bot: I haz the power, 2014-08-21 00:36:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[commit-bot: I haz the power, 2014-08-21 00:40:34]

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

[commit-bot: I haz the power, 2014-08-21 00:40:35]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[rlarocque, 2014-08-21 00:43:49]

The trybots are having a bad day.  Their credentials are being rejected by the
SVN server when they try to update.

The troopers are investigating.  It's been like this all day, so it could be a
while before it gets fixed.

[rlarocque, 2014-08-21 00:52:22]

On 2014/08/21 00:43:49, rlarocque wrote:
> The trybots are having a bad day.  Their credentials are being rejected by the
> SVN server when they try to update.
> 
> The troopers are investigating.  It's been like this all day, so it could be a
> while before it gets fixed.

There's a bug for it now: http://crbug.com/405783

[pval...(no longer on Chromium), 2014-08-21 18:27:13]

The CQ bit was checked by pvalenzuela@chromium.org

[commit-bot: I haz the power, 2014-08-21 18:31:56]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/pvalenzuela@chromium.org/457883002/240001

[commit-bot: I haz the power, 2014-08-21 21:26:50]

Committed patchset #9 (240001) as 291190