Use a test specific macro to call JNI registrars during APK tests.

Use a test specific macro to call JNI registrars during APK tests.

TBR=rsleevi@chromium.org,jam@chromium.org
(Using TBR as this is a trivial gyp change)
BUG=125059
TEST=


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

[John Grabowski, 2012-06-25 21:04:18]

LGTM

Like MUCH better then the other solution.

http://codereview.chromium.org/10658017/diff/1/testing/android/native_test_la...
File testing/android/native_test_launcher.cc (right):

http://codereview.chromium.org/10658017/diff/1/testing/android/native_test_la...
testing/android/native_test_launcher.cc:188: #if
defined(REGISTER_NET_UNITTESTS_JNI)
Explain how/when/where these are defined in a comment.

[Yaron, 2012-06-25 21:06:11]

lgtm as well fwiw

[commit-bot: I haz the power, 2012-06-25 21:40:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/10658017/1005

[nilesh, 2012-06-25 21:40:39]

http://codereview.chromium.org/10658017/diff/1/testing/android/native_test_la...
File testing/android/native_test_launcher.cc (right):

http://codereview.chromium.org/10658017/diff/1/testing/android/native_test_la...
testing/android/native_test_launcher.cc:188: #if
defined(REGISTER_NET_UNITTESTS_JNI)
On 2012/06/25 21:04:18, John Grabowski wrote:
> Explain how/when/where these are defined in a comment.

Done.

[commit-bot: I haz the power, 2012-06-25 23:39:33]

Change committed as 144057

[Ryan Sleevi, 2012-06-25 23:49:29]

When TBRing, please make sure to add reviewers to the reviewer list and publish
comments. I didn't even see this go by until I saw my name on the waterfall.

This feels very much like a layering violation (having testing/ code have to
have specific knowledge of net/ and content/ ) - so much so I would have nack'd
and asked for further review.

It seems like you should instead be doing the same thing we do for upstream
tests - either deriving from a base class (provided by testing/) within each
relevant folder (content/, net/) and registering in those subclasses - or having
basic test harnesses within those folders that call the base implementations.

For example, see src/base/net_test_suite.h

I don't think this is as simple as described/appropriate for a TBR.

[nilesh, 2012-06-26 17:00:19]

Hi Ryan,

Thanks for your feedback. I am sorry for incorrectly categorizing this as a
simple gyp change. I am looking into base/net_test_suite.h and how I can use a
similar strategy for other test targets too.

(Other approaches I tried: http://codereview.chromium.org/10591005/)

Do let me know if you want this reverted rightaway.

On 2012/06/25 23:49:29, Ryan Sleevi wrote:
> When TBRing, please make sure to add reviewers to the reviewer list and
publish
> comments. I didn't even see this go by until I saw my name on the waterfall.
> 
> This feels very much like a layering violation (having testing/ code have to
> have specific knowledge of net/ and content/ ) - so much so I would have
nack'd
> and asked for further review.
> 
> It seems like you should instead be doing the same thing we do for upstream
> tests - either deriving from a base class (provided by testing/) within each
> relevant folder (content/, net/) and registering in those subclasses - or
having
> basic test harnesses within those folders that call the base implementations.
> 
> For example, see src/base/net_test_suite.h
> 
> I don't think this is as simple as described/appropriate for a TBR.

[Ryan Sleevi, 2012-06-26 17:53:53]

On 2012/06/26 17:00:19, nileshagrawal1 wrote:
> Hi Ryan,
> 
> Thanks for your feedback. I am sorry for incorrectly categorizing this as a
> simple gyp change. I am looking into base/net_test_suite.h and how I can use a
> similar strategy for other test targets too.
> 
> (Other approaches I tried: http://codereview.chromium.org/10591005/)
> 
> Do let me know if you want this reverted rightaway.

Hi Nilesh,

If the Android infrastructure has not already adapted to this CL (and thus
cannot be easily reverted without master restarts), I do think reverting would
be appropriate. I personally don't feel strongly about this, especially if this
will be resolved within the next day or two, but since I suspect that it may
take a bit longer, and because the Chromium-side prefers reviews first, rather
than expediency, reverting seems more appropriate.

I certainly agree that this is a great improvement over the other CL, but I
think we can do better without the layering concern.

You should also look at base/base.gyp:run_all_unittests

For test suites that do not need to do any special (global) initialization, we
have the test suite target depend on run_all_unittests, which provides the
main() entry point. For tests that do (such as net_unittests or crypto_unittests
on the native side, for example), we have them depend on test_support_base, that
provides all the Chromium-specific helpers, but they're responsible for
implementing main() and doing any extra setup themselves (eg: via a
run_all_unittests.cc file in their respective subdirectories).

I think with appropriate restructuring, you should be able to accomplish this
for your apk needs, since the APKs are already dedicated targets.

> 
> On 2012/06/25 23:49:29, Ryan Sleevi wrote:
> > When TBRing, please make sure to add reviewers to the reviewer list and
> publish
> > comments. I didn't even see this go by until I saw my name on the waterfall.
> > 
> > This feels very much like a layering violation (having testing/ code have to
> > have specific knowledge of net/ and content/ ) - so much so I would have
> nack'd
> > and asked for further review.
> > 
> > It seems like you should instead be doing the same thing we do for upstream
> > tests - either deriving from a base class (provided by testing/) within each
> > relevant folder (content/, net/) and registering in those subclasses - or
> having
> > basic test harnesses within those folders that call the base
implementations.
> > 
> > For example, see src/base/net_test_suite.h
> > 
> > I don't think this is as simple as described/appropriate for a TBR.

[nilesh, 2012-06-26 18:24:55]

Thanks again for the detailed explaination.

Revert CL: http://codereview.chromium.org/10662056/
I will wait for atleast one LGTM before TBR :)