Upstream JavaBridge tests.

Move JavaBridge tests to content/public/android/javatests

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

[jam, 2012-09-06 22:05:05]

why are these tests under content/shell? in general, tests live alongside the
code they're testing, not with the target that they're built in. i.e. for a
browser test, it might be in content\browser, even though it's linked (and uses)
content shell which is in content\shell

[Yaron, 2012-09-06 22:15:56]

On 2012/09/06 22:05:05, John Abd-El-Malek wrote:
> why are these tests under content/shell? in general, tests live alongside the
> code they're testing, not with the target that they're built in. i.e. for a
> browser test, it might be in content\browser, even though it's linked (and
uses)
> content shell which is in content\shell

Haven't looked at code yet but some thigns:
1) In java, it's typical to have a java/ tree and a parallel javatests/ tree. 
2) As for why content shell: I believe these tests rely on having a full browser
context because we're actually executing javascript code and verifying that
certain callbacks are hooked up and the relevant marshaling of params/types
happens.

[jam, 2012-09-06 22:36:55]

On 2012/09/06 22:15:56, Yaron wrote:
> On 2012/09/06 22:05:05, John Abd-El-Malek wrote:
> > why are these tests under content/shell? in general, tests live alongside
the
> > code they're testing, not with the target that they're built in. i.e. for a
> > browser test, it might be in content\browser, even though it's linked (and
> uses)
> > content shell which is in content\shell
> 
> Haven't looked at code yet but some thigns:
> 1) In java, it's typical to have a java/ tree and a parallel javatests/ tree. 

do these realy have to be under content/shell though, as opposed to
content/browser?

> 2) As for why content shell: I believe these tests rely on having a full
browser
> context because we're actually executing javascript code and verifying that
> certain callbacks are hooked up and the relevant marshaling of params/types
> happens.

sure, just like content_browsertests depend on having a full browser running
(content_shell). however, the tests themselves aren't located in content/shell.
they're in content/browser for the most part, beside the code they're testing.

[Yaron, 2012-09-07 01:29:57]

I guess I'd like some clarity on your major contention. I really don't think we
should ever do:
content/browser/java/src/Foo.java and 
content/browser/java/src/FooTest.java
for java code.

Correct me if I'm wrong, but I think you want something like:
content/browser/java/src/Foo.java and
content/browser/javatests/src/Foo.java
whereas currently, we roll up tests that aren't strictly unit tests to the
nearest test runner directory (e.g. content/shell/android/javatests,
android_webview/javatests or chrome/android/javatests) and execute them from
there. Otherwise, there'd be a dependency violation as say
content/browser/javatests has to depend on content/shell/android/javatests in
order to get the test fixture for running integration tests that use content
shell.

I'm not sure about this but I think aside from deps we have an issue that if we
define tests say in the aforementioned directories, we'd currently only run
tests from content/shell/android because of how we find the actual tests to
execute from the test runner. Of course that could probably be fixed with a
change to our test runners. +Marcus to keep me honest

[bulach, 2012-09-07 08:00:48]

the way java tools / packaging work, it builds _everything_ under a directory
and puts in a .jar/.apk (which are an archive essentially).. there's no way (at
the moment) to segregate files into different archives..

please note that this is not something specific to chrome on android, google in
general uses this "javatest" top-level directory approach:
project
|__ java/foo/bar/Dee.java
|__ javatests/foo/bar/DeeTest.java

there was some discussion a while ago on what we'd consider "project" in our
context, and we decided that it's the parent of the "android" folder, so we
have:
base/android
|__ java/src/org/chromium/base
|__ javatests/src/org/chromium/base

content/public/android
|__ java/src/org/chromium/content
|__ javatests/src/org/chromium/content

amongst the arguments, is that the filesystem hierarchy from the file up has to
mimic somehow the java "package" (i.e., org.chromium.base,
org.chromium.content).. the alternative approach was to have a truly parallel
tree for java and its tests, starting at a really top-level:
chrome/src
|__ java (/base, /content, etc)
|__ javatests (/base, /content, etc..)

but that would make things even further from their C++ counterparts...

that's all I know :) I hope that helps, and I'm happy to provide more
information / add more people to the discussion.

[jam, 2012-09-07 15:04:39]

On 2012/09/07 01:29:57, Yaron wrote:
> I guess I'd like some clarity on your major contention. I really don't think
we
> should ever do:
> content/browser/java/src/Foo.java and 
> content/browser/java/src/FooTest.java
> for java code.
> 
> Correct me if I'm wrong, but I think you want something like:
> content/browser/java/src/Foo.java and
> content/browser/javatests/src/Foo.java

right

> whereas currently, we roll up tests that aren't strictly unit tests to the
> nearest test runner directory (e.g. content/shell/android/javatests,
> android_webview/javatests or chrome/android/javatests) and execute them from
> there. Otherwise, there'd be a dependency violation as say
> content/browser/javatests has to depend on content/shell/android/javatests in
> order to get the test fixture for running integration tests that use content
> shell.

what i've been trying to point out is that this isn't a dependency violation.
i.e. a content_browsertest (say, in content/browser), by definition, has to
include stuff from content/shell so that it can manipulate its host browser.
this is fine.

content/shell can have tests that are specifically testing Content Shell. but
tests that are generic tests over "content" should be under content/browser or
content/renderer or content/common etc...
> 
> I'm not sure about this but I think aside from deps we have an issue that if
we
> define tests say in the aforementioned directories, we'd currently only run
> tests from content/shell/android because of how we find the actual tests to
> execute from the test runner. Of course that could probably be fixed with a
> change to our test runners. +Marcus to keep me honest

[klundberg, 2012-09-07 16:15:29]

Some general comments.

I will address the discussion wrt location next.

http://codereview.chromium.org/10911131/diff/2001/content/shell/android/javat...
File
content/shell/android/javatests/src/org/chromium/content_shell/ContentShellTestBase.java
(right):

http://codereview.chromium.org/10911131/diff/2001/content/shell/android/javat...
content/shell/android/javatests/src/org/chromium/content_shell/ContentShellTestBase.java:26:
= new HashMap<ContentView, TestContentViewClient>();
I don't think you should add this to ContentShellTestBase as not all sub-classes
will need this.

Instead, create another ContentViewTestBase class and add the ContentView
specific things to this class.

Otherwise, we will just end up in the same situation as MainActivityTestBase is
in.

http://codereview.chromium.org/10911131/diff/2001/content/shell/android/javat...
content/shell/android/javatests/src/org/chromium/content_shell/ContentShellTestBase.java:85:
TimeUnit.SECONDS);
This should be in a utility class, not in a test base class.

http://codereview.chromium.org/10911131/diff/2001/content/shell/android/javat...
content/shell/android/javatests/src/org/chromium/content_shell/ContentShellTestBase.java:94:
throws Throwable {
Same comment as above.

[klundberg, 2012-09-07 16:22:16]

On 2012/09/07 15:04:39, John Abd-El-Malek wrote:
> On 2012/09/07 01:29:57, Yaron wrote:
> > I guess I'd like some clarity on your major contention. I really don't think
> we
> > should ever do:
> > content/browser/java/src/Foo.java and 
> > content/browser/java/src/FooTest.java
> > for java code.
> > 
> > Correct me if I'm wrong, but I think you want something like:
> > content/browser/java/src/Foo.java and
> > content/browser/javatests/src/Foo.java
> 
> right
> 
> > whereas currently, we roll up tests that aren't strictly unit tests to the
> > nearest test runner directory (e.g. content/shell/android/javatests,
> > android_webview/javatests or chrome/android/javatests) and execute them from
> > there. Otherwise, there'd be a dependency violation as say
> > content/browser/javatests has to depend on content/shell/android/javatests
in
> > order to get the test fixture for running integration tests that use content
> > shell.
> 
> what i've been trying to point out is that this isn't a dependency violation.
> i.e. a content_browsertest (say, in content/browser), by definition, has to
> include stuff from content/shell so that it can manipulate its host browser.
> this is fine.
> 
> content/shell can have tests that are specifically testing Content Shell. but
> tests that are generic tests over "content" should be under content/browser or
> content/renderer or content/common etc...

I agree that this should be the approach.

The dependency on content/shell/android/javatests is only because we are trying
to use ContentShell to get the ContentView.

Originally, we tried to use a separate Activity for these kind of tests but that
had some dependencies on the chrome layer when we tried to run these as part of
the ContentShellTest APK. But it should be possible to have a similar approach
without the dependency on the chrome layer. Maybe we should discuss more in
person. 

> > 
> > I'm not sure about this but I think aside from deps we have an issue that if
> we
> > define tests say in the aforementioned directories, we'd currently only run
> > tests from content/shell/android because of how we find the actual tests to
> > execute from the test runner. Of course that could probably be fixed with a
> > change to our test runners. +Marcus to keep me honest

[Yaron, 2012-09-07 16:46:43]

On 2012/09/07 16:22:16, klundberg wrote:
> On 2012/09/07 15:04:39, John Abd-El-Malek wrote:
> > On 2012/09/07 01:29:57, Yaron wrote:
> > > I guess I'd like some clarity on your major contention. I really don't
think
> > we
> > > should ever do:
> > > content/browser/java/src/Foo.java and 
> > > content/browser/java/src/FooTest.java
> > > for java code.
> > > 
> > > Correct me if I'm wrong, but I think you want something like:
> > > content/browser/java/src/Foo.java and
> > > content/browser/javatests/src/Foo.java
> > 
> > right
> > 
> > > whereas currently, we roll up tests that aren't strictly unit tests to the
> > > nearest test runner directory (e.g. content/shell/android/javatests,
> > > android_webview/javatests or chrome/android/javatests) and execute them
from
> > > there. Otherwise, there'd be a dependency violation as say
> > > content/browser/javatests has to depend on content/shell/android/javatests
> in
> > > order to get the test fixture for running integration tests that use
content
> > > shell.
> > 
> > what i've been trying to point out is that this isn't a dependency
violation.
> > i.e. a content_browsertest (say, in content/browser), by definition, has to
> > include stuff from content/shell so that it can manipulate its host browser.
> > this is fine.
> > 
> > content/shell can have tests that are specifically testing Content Shell.
but
> > tests that are generic tests over "content" should be under content/browser
or
> > content/renderer or content/common etc...
> 
> I agree that this should be the approach.
> 
> The dependency on content/shell/android/javatests is only because we are
trying
> to use ContentShell to get the ContentView.
> 
> Originally, we tried to use a separate Activity for these kind of tests but
that
> had some dependencies on the chrome layer when we tried to run these as part
of
> the ContentShellTest APK. But it should be possible to have a similar approach
> without the dependency on the chrome layer. Maybe we should discuss more in
> person. 
> 
> > > 
> > > I'm not sure about this but I think aside from deps we have an issue that
if
> > we
> > > define tests say in the aforementioned directories, we'd currently only
run
> > > tests from content/shell/android because of how we find the actual tests
to
> > > execute from the test runner. Of course that could probably be fixed with
a
> > > change to our test runners. +Marcus to keep me honest

Happy to meet in person if necessary but I think I understand now. It seems to
me like these specific tests can live in:
content/android/javatests/src/org/chromium/content/browser
They're testing browser-level code in the content layer.
Note that this differs from:
content/public/android/javatests/src which contains content-level test utils
that are shared between content/chrome. As Marcus poitned out, due to how java
packages are managed, we need to separate the shared test utils and the
instrumentation tests that run in the content layer. From jam's comment, it also
sounds like content/android/javatests -> content/shell/android/javatests is not
a deps issue for these tests.

[Ramya, 2012-09-19 21:31:57]

PTAL

http://codereview.chromium.org/10911131/diff/2001/content/shell/android/javat...
File
content/shell/android/javatests/src/org/chromium/content_shell/ContentShellTestBase.java
(right):

http://codereview.chromium.org/10911131/diff/2001/content/shell/android/javat...
content/shell/android/javatests/src/org/chromium/content_shell/ContentShellTestBase.java:26:
= new HashMap<ContentView, TestContentViewClient>();
On 2012/09/07 16:15:29, klundberg wrote:
> I don't think you should add this to ContentShellTestBase as not all
sub-classes
> will need this.
> 
> Instead, create another ContentViewTestBase class and add the ContentView
> specific things to this class.
> 
> Otherwise, we will just end up in the same situation as MainActivityTestBase
is
> in.

Done.

http://codereview.chromium.org/10911131/diff/2001/content/shell/android/javat...
content/shell/android/javatests/src/org/chromium/content_shell/ContentShellTestBase.java:85:
TimeUnit.SECONDS);
On 2012/09/07 16:15:30, klundberg wrote:
> This should be in a utility class, not in a test base class.

As discussed, leaving it here till the Util files are moved to base.

http://codereview.chromium.org/10911131/diff/2001/content/shell/android/javat...
content/shell/android/javatests/src/org/chromium/content_shell/ContentShellTestBase.java:94:
throws Throwable {
On 2012/09/07 16:15:30, klundberg wrote:
> Same comment as above.

See comment above.

[Yaron, 2012-09-20 04:13:53]

+Yusuf

I've love to just see this land but it's a waste given the refactorings in
cotent_view_core that Jay and Yusuf are doing. Can you work with Yusuf to sort
that out?

http://codereview.chromium.org/10911131/diff/11001/content/browser/android/co...
File content/browser/android/content_view_core_impl.cc (right):

http://codereview.chromium.org/10911131/diff/11001/content/browser/android/co...
content/browser/android/content_view_core_impl.cc:711: CheckException(env);
Should be done automatically by jni-generator

http://codereview.chromium.org/10911131/diff/11001/content/browser/android/co...
File content/browser/android/content_view_core_impl.h (right):

http://codereview.chromium.org/10911131/diff/11001/content/browser/android/co...
content/browser/android/content_view_core_impl.h:62: // Called when page loading
begins.
Most of this is being refactored by Yusuf:
https://gerrit-int.chromium.org/#/c/25017/

http://codereview.chromium.org/10911131/diff/11001/content/shell/android/java...
File content/shell/android/javatests/content_shell_test_apk.xml (right):

http://codereview.chromium.org/10911131/diff/11001/content/shell/android/java...
content/shell/android/javatests/content_shell_test_apk.xml:30: <path
id="javac.custom.sourcepath">
Nit: fix indentation

http://codereview.chromium.org/10911131/diff/11001/content/shell/android/java...
File
content/shell/android/javatests/src/org/chromium/content_shell/ContentShellTestBase.java
(right):

http://codereview.chromium.org/10911131/diff/11001/content/shell/android/java...
content/shell/android/javatests/src/org/chromium/content_shell/ContentShellTestBase.java:12:
import org.chromium.content.browser.ContentView;
It doesn't seem like any of these new imports are necessary. Please remove.

[Yusuf, 2012-09-20 04:37:57]

Jay's change is in review and ready to land downstream. If we can push the
upstream version tomorrow, I have both the downstream and upstream CLs uploaded.
So all pieces are actually in place.

We can talk with Ramya more tomorrow, but to me it seems if we can rid of the
content_view_core related changes, this actually won't conflict with my CLs and
we would be able to run the tests after my CLs land.

[Ramya, 2012-09-24 23:33:26]

content_view_core.h, content_view_core_impl.* and ContentViewCore.java changes
have been removed. These are no longer needed.

[Yaron, 2012-09-24 23:45:56]

On 2012/09/24 23:33:26, Ramya wrote:
> content_view_core.h, content_view_core_impl.* and ContentViewCore.java changes
> have been removed. These are no longer needed.

Seems like this is blocked on Tommy's refactor (you need the
content_java_test_support target) which should land later today. Let's try
tomorrow.

[Ramya, 2012-09-25 22:37:12]

PTAL

[Yaron, 2012-09-25 22:43:16]

lgtm

http://codereview.chromium.org/10911131/diff/23001/content/content_tests.gypi
File content/content_tests.gypi (right):

http://codereview.chromium.org/10911131/diff/23001/content/content_tests.gypi...
content/content_tests.gypi:881: 'target_name': 'content_shell_javatests',
Just discussed with Chris. We don't actually need this right now. Eventually we
will, but additional_input_paths in content_shell_test_apk does the same thing.
So please remove.

[Ramya, 2012-09-25 22:49:54]

PTAL

http://codereview.chromium.org/10911131/diff/23001/content/content_tests.gypi
File content/content_tests.gypi (right):

http://codereview.chromium.org/10911131/diff/23001/content/content_tests.gypi...
content/content_tests.gypi:881: 'target_name': 'content_shell_javatests',
On 2012/09/25 22:43:16, Yaron wrote:
> Just discussed with Chris. We don't actually need this right now. Eventually
we
> will, but additional_input_paths in content_shell_test_apk does the same
thing.
> So please remove.

Done.

[Yaron, 2012-09-25 22:51:37]

lgtm

Great, that's cleaner.

[klundberg, 2012-09-25 22:59:56]

lgtm

[cjhopman, 2012-09-25 23:26:06]

lgtm

[jam, 2012-09-25 23:34:10]

lgtm

[commit-bot: I haz the power, 2012-09-25 23:35:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cramya@chromium.org/10911131/27001

[commit-bot: I haz the power, 2012-09-26 00:09:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cramya@chromium.org/10911131/27001

[commit-bot: I haz the power, 2012-09-26 01:26:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cramya@chromium.org/10911131/19015

[commit-bot: I haz the power, 2012-09-26 03:50:38]

Change committed as 158743