Set up initial MockURLRequestJob tests for Cronet

Set up initial MockURLRequestJob tests for Cronet

When we add the new Async APIs, we will need to implement tests for these
APIs. MockURLRequestJob is a nice way, as suggested by mmenke, to simulate
failed jobs during different stage of url requests.

BUG=

[xunjieli, 2014-09-02 13:23:25]

xunjieli@chromium.org changed reviewers:
+ mef@chromium.org, mmenke@chromium.org

[xunjieli, 2014-09-02 13:23:25]

Hi Matt and Misha, I just would like a CL that sets things up for writing tests
later.Let me know what you think. Thanks!

https://codereview.chromium.org/525613002/diff/20001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java
(left):

https://codereview.chromium.org/525613002/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java:24:
System.loadLibrary("cronet");
Is this needed?
It seems unnecessary, since clients will load the library anyway. Also for
testing, "cronet_tests" should be loaded by the tests. If we have this line,
"cronet" will be loaded additionally, which we do not want.

[mmenke, 2014-09-02 20:49:39]

On 2014/09/02 13:23:25, xunjieli1 wrote:
> Hi Matt and Misha, I just would like a CL that sets things up for writing
tests
> later.Let me know what you think. Thanks!
> 
>
https://codereview.chromium.org/525613002/diff/20001/components/cronet/androi...
> File
>
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java
> (left):
> 
>
https://codereview.chromium.org/525613002/diff/20001/components/cronet/androi...
>
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java:24:
> System.loadLibrary("cronet");
> Is this needed?
> It seems unnecessary, since clients will load the library anyway. Also for
> testing, "cronet_tests" should be loaded by the tests. If we have this line,
> "cronet" will be loaded additionally, which we do not want.

Sorry for not getting to this today.  Saved it for last because of its size,
should get to it tomorrow.

[mef, 2014-09-02 21:35:55]

couple of nits / questions, will have more tomorrow.

https://codereview.chromium.org/525613002/diff/20001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/525613002/diff/20001/components/cronet.gypi#n...
components/cronet.gypi:340: 
nit: spurious nl.

https://codereview.chromium.org/525613002/diff/20001/components/cronet.gypi#n...
components/cronet.gypi:351: 'native_lib_target': 'libcronet_tests',
why _tests?

https://codereview.chromium.org/525613002/diff/20001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java
(left):

https://codereview.chromium.org/525613002/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java:24:
System.loadLibrary("cronet");
On 2014/09/02 13:23:25, xunjieli1 wrote:
> Is this needed?
> It seems unnecessary, since clients will load the library anyway. Also for
> testing, "cronet_tests" should be loaded by the tests. If we have this line,
> "cronet" will be loaded additionally, which we do not want.
That makes sense if clients do load it explicitly, which I'm not sure about, but
I agree that it probably shouldn't be here.

[xunjieli, 2014-09-03 15:04:48]

https://codereview.chromium.org/525613002/diff/20001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/525613002/diff/20001/components/cronet.gypi#n...
components/cronet.gypi:340: 
On 2014/09/02 21:35:55, mef wrote:
> nit: spurious nl.

Done. Thanks!

https://codereview.chromium.org/525613002/diff/20001/components/cronet.gypi#n...
components/cronet.gypi:351: 'native_lib_target': 'libcronet_tests',
On 2014/09/02 21:35:55, mef wrote:
> why _tests?

Done. Oops sorry, forgot to change it back.

https://codereview.chromium.org/525613002/diff/20001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java
(left):

https://codereview.chromium.org/525613002/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java:24:
System.loadLibrary("cronet");
On 2014/09/02 21:35:55, mef wrote:
> On 2014/09/02 13:23:25, xunjieli1 wrote:
> > Is this needed?
> > It seems unnecessary, since clients will load the library anyway. Also for
> > testing, "cronet_tests" should be loaded by the tests. If we have this line,
> > "cronet" will be loaded additionally, which we do not want.
> That makes sense if clients do load it explicitly, which I'm not sure about,
but
> I agree that it probably shouldn't be here.

Acknowledged. I guess I will just remove it. I think they should load it
explicitly like the cronet sample app does.

[mmenke, 2014-09-03 18:02:35]

I just skimmed the Java part of this CL.  I need to take the time to get up to
speed on Java activities and the like.

https://codereview.chromium.org/525613002/diff/40001/components/cronet/androi...
File components/cronet/android/test/cronet_tests_jni.cc (right):

https://codereview.chromium.org/525613002/diff/40001/components/cronet/androi...
components/cronet/android/test/cronet_tests_jni.cc:23: 
nit:  Remove extra newline.

https://codereview.chromium.org/525613002/diff/40001/components/cronet/androi...
components/cronet/android/test/cronet_tests_jni.cc:39: JNI_EXPORT jint
JNI_OnLoad(JavaVM* vm, void* reserved) {
This duplicates a lot of code.  We should make public functions for startup and
shutdown of cronet, and then just have different OnLoadHandlers

https://codereview.chromium.org/525613002/diff/40001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetTestBase.java
(right):

https://codereview.chromium.org/525613002/diff/40001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetTestBase.java:16:

nit:  I don't believe blank lines are generally used after the start of classes.

https://codereview.chromium.org/525613002/diff/40001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java
(right):

https://codereview.chromium.org/525613002/diff/40001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:27:
+ request.getContentLength());
nit:  Binary operators should be put on the same line as their first argument.

https://codereview.chromium.org/525613002/diff/40001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:39:
+ request.getHttpStatusCode());
We should have the http status code in onResponseStarted, as it's sent before
the headers are.

https://codereview.chromium.org/525613002/diff/40001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:39:
+ request.getHttpStatusCode());
nit:  Binary operators should be put on the same line as their first argument.

https://codereview.chromium.org/525613002/diff/40001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/MockURLRequestJobTest.java:62:
assertEquals(200, request.getHttpStatusCode());
Are we talking to a real server here?  If so, seems a little weird in a
"MockURLRequestJobTest".

If that's the case, fine with either renaming the test class, or mocking out a
successful request.

https://codereview.chromium.org/525613002/diff/40001/components/cronet/androi...
File components/cronet/android/test/mock_url_request_job.cc (right):

https://codereview.chromium.org/525613002/diff/40001/components/cronet/androi...
components/cronet/android/test/mock_url_request_job.cc:33:
GURL(ConvertJavaStringToUTF8(env, jurl)), &FailedJobFactory);
Suggest just using URLRequestFailedJob::AddUrlHandler(), and always invoking it
on test startup - can actually move the call into JNI_OnLoad in
cronet_tests_jni, and you won't even need any new JNI calls, just the extra
initialization line.

Then your test can just navigate to, say, "http://mock.failed.request/101", and
requests for that URL will fail with ERR_CONNECTION_RESET (That's error 101. 
See net/base/net_error_list.h for a list of error codes, though not sure we
currently really care about which code a request fails with).

https://codereview.chromium.org/525613002/diff/40001/net/test/url_request_fai...
File net/test/url_request_failed_job.h (right):

https://codereview.chromium.org/525613002/diff/40001/net/test/url_request_fai...
net/test/url_request_failed_job.h:6: #define NET_TEST_URL_REQUEST_FAILED_JOB_H_
Rather than duplicate the old file, we should move it.  This will affect a lot
of test files, so you should probably do it in another CL.

It's a pain, but I suggest doing that for all job types in content/net/test.

May be a little cleaner to move them into net/test/url_request than net/test,
just to keep them all together, and separate from the other test code.

[mef, 2014-09-03 22:03:19]

After this CL we would probably want to move all the tests from cronet_sample
into this, so cronet_sample would be a) simpler and b) proguardable.

https://codereview.chromium.org/525613002/diff/40001/components/cronet/androi...
File components/cronet/android/test/res/layout/cronet_sample_activity.xml
(right):

https://codereview.chromium.org/525613002/diff/40001/components/cronet/androi...
components/cronet/android/test/res/layout/cronet_sample_activity.xml:9:
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
Suggest to rename this to "cronet_test_activity.xml"

[mef, 2014-09-03 22:04:04]

On 2014/09/03 22:03:19, mef wrote:
> After this CL we would probably want to move all the tests from cronet_sample
> into this, so cronet_sample would be a) simpler and b) proguardable.
> 
>
https://codereview.chromium.org/525613002/diff/40001/components/cronet/androi...
> File components/cronet/android/test/res/layout/cronet_sample_activity.xml
> (right):
> 
>
https://codereview.chromium.org/525613002/diff/40001/components/cronet/androi...
> components/cronet/android/test/res/layout/cronet_sample_activity.xml:9:
> <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
> Suggest to rename this to "cronet_test_activity.xml"

I've created bug 409926 for Async API.

[mmenke, 2014-09-03 22:06:00]

On 2014/09/03 22:04:04, mef wrote:
> On 2014/09/03 22:03:19, mef wrote:
> > After this CL we would probably want to move all the tests from
cronet_sample
> > into this, so cronet_sample would be a) simpler and b) proguardable.
> > 
> >
>
https://codereview.chromium.org/525613002/diff/40001/components/cronet/androi...
> > File components/cronet/android/test/res/layout/cronet_sample_activity.xml
> > (right):
> > 
> >
>
https://codereview.chromium.org/525613002/diff/40001/components/cronet/androi...
> > components/cronet/android/test/res/layout/cronet_sample_activity.xml:9:
> > <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
> > Suggest to rename this to "cronet_test_activity.xml"
> 
> I've created bug 409926 for Async API.

Misha: Just FYI, I talked to Helen about moving the mock URL requests from
content/ to net/.  It's a pretty painful refactoring for mock_url_request_job,
in particular, but we're going to go ahead and do that first.