Fix a pair of Cronet upload bugs and add tests.

components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java

Index: components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java
diff --git a/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java b/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java
new file mode 100644
index 0000000000000000000000000000000000000000..e9582d3f75dbd544ca6c54cc548dfefdb9cc0010
--- /dev/null
+++ b/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java
@@ -0,0 +1,229 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.cronet_test_apk;
+
+import android.test.suitebuilder.annotation.SmallTest;
+
+import org.chromium.base.test.util.Feature;
+import org.chromium.net.HttpUrlRequest;
+import org.chromium.net.HttpUrlRequestListener;
+
+import java.io.ByteArrayInputStream;
+import java.io.InputStream;
+
+import java.nio.channels.Channels;
+import java.nio.channels.ReadableByteChannel;
+
+import java.util.HashMap;
+
+/**
+ * Example test that just starts the cronet sample.

[xunjieli, 2014/10/17 20:56:45]

nit: stale doc.

[mmenke, 2014/10/17 21:27:15]

Done.

+ */
+public class UploadTest extends CronetTestBase {
+    private static final String UPLOAD_DATA = "Nifty upload data!";
+    private static final String UPLOAD_CHANNEL_DATA = "Upload channel data";
+
+    private HttpUrlRequest createRequest(
+            String url, HttpUrlRequestListener listener,
+            CronetTestActivity activity) {
+        HashMap<String, String> headers = new HashMap<String, String>();
+        return activity.mChromiumRequestFactory.createRequest(

[xunjieli, 2014/10/17 20:56:45]

I removed |mChromiumRequestFactory|. You can use |mRequestFactory| instead.

[mmenke, 2014/10/17 21:27:15]

Done.

+                url, HttpUrlRequest.REQUEST_PRIORITY_MEDIUM, headers, listener);
+    }
+
+    // Sets request to have an upload channel containing the given data.
+    // uploadDataLength should generally be uploadData.length(), unless a test
+    // needs to get a read error.
+    private void setUploadChannel(HttpUrlRequest request,
+                                  String contentType,
+                                  String uploadData,
+                                  int uploadDataLength) {
+        InputStream uploadDataStream = new ByteArrayInputStream(
+                uploadData.getBytes());
+        ReadableByteChannel uploadDataChannel =
+                Channels.newChannel(uploadDataStream);
+        request.setUploadChannel(
+                contentType, uploadDataChannel, uploadDataLength);
+    }
+
+    // Tests uploading an in-memory string.
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testUploadData() throws Exception {
+        CronetTestActivity activity = launchCronetTestApp();

[mef, 2014/10/17 20:47:27]

Does it make sense to combine boilerplate to launch app and add interceptors
into helper method?
Or maybe that should go into launchCronetTestApp()?

[mmenke, 2014/10/17 21:27:15]

Reluctant to add this to launchCronetTestApp, since the test server isn't used
by other tests.  Open to doing that if we continue to use it with the new API,
and use the server more broadly.

I've gone ahead and created a separate method.  Also made activity a class
variable.

+        assertNotNull(activity);
+        assertTrue(UploadTestServer.startUploadTestServer());
+        MockUrlRequestJobUtil.addUrlInterceptors();
+
+        TestHttpUrlRequestListener listener = new TestHttpUrlRequestListener();
+        HttpUrlRequest request = createRequest(
+                UploadTestServer.getEchoBodyURL(), listener, activity);
+        request.setUploadData("text/plain", UPLOAD_DATA.getBytes("UTF8"));
+        request.start();
+        listener.blockForComplete();
+
+        assertEquals(200, listener.mHttpStatusCode);
+        assertEquals(UPLOAD_DATA, listener.mResponseAsString);
+    }
+
+    // Tests uploading an in-memory string with a redirect that preserves the
+    // POST body.  This makes sure the body is correctly sent again.
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testUploadDataWithRedirect() throws Exception {
+        CronetTestActivity activity = launchCronetTestApp();
+        assertNotNull(activity);
+        assertTrue(UploadTestServer.startUploadTestServer());
+        MockUrlRequestJobUtil.addUrlInterceptors();
+
+        TestHttpUrlRequestListener listener = new TestHttpUrlRequestListener();
+        HttpUrlRequest request = createRequest(
+                UploadTestServer.getRedirectToEchoBody(), listener, activity);
+        request.setUploadData("text/plain", UPLOAD_DATA.getBytes("UTF8"));
+        request.start();
+        listener.blockForComplete();
+
+        assertEquals(200, listener.mHttpStatusCode);
+        assertEquals(UPLOAD_DATA, listener.mResponseAsString);
+    }
+
+    // Tests Content-Type can be set when uploading an in-memory string.
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testUploadDataContentType() throws Exception {
+        String contentTypes[] = {"text/plain", "chicken/spicy"};
+        CronetTestActivity activity = launchCronetTestApp();
+        assertNotNull(activity);
+        assertTrue(UploadTestServer.startUploadTestServer());
+        MockUrlRequestJobUtil.addUrlInterceptors();
+
+        for (String contentType : contentTypes) {
+            TestHttpUrlRequestListener listener = new TestHttpUrlRequestListener();

[mef, 2014/10/17 20:47:27]

nit: 80 chars, although I've heard a suggestion to adjust column width for Java
in Rietveld.

[mmenke, 2014/10/17 21:27:15]

Done.

+            HttpUrlRequest request = createRequest(
+                    UploadTestServer.getEchoHeaderURL("Content-Type"), listener,
+                    activity);
+            request.setUploadData(contentType, UPLOAD_DATA.getBytes("UTF8"));
+            request.start();
+            listener.blockForComplete();
+
+            assertEquals(200, listener.mHttpStatusCode);
+            assertEquals(contentType, listener.mResponseAsString);
+        }
+    }
+
+    // Tests methods can be set when uploading.
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testUploadMethods() throws Exception {
+        String uploadMethods[] = {"POST", "PUT"};
+        CronetTestActivity activity = launchCronetTestApp();
+        assertNotNull(activity);
+        assertTrue(UploadTestServer.startUploadTestServer());
+        MockUrlRequestJobUtil.addUrlInterceptors();
+
+        for (String uploadMethod : uploadMethods) {
+            TestHttpUrlRequestListener listener = new TestHttpUrlRequestListener();
+            HttpUrlRequest request = createRequest(
+                    UploadTestServer.getEchoMethodURL(), listener, activity);
+            request.setHttpMethod(uploadMethod);
+            request.setUploadData("text/plain", UPLOAD_DATA.getBytes("UTF8"));
+            request.start();
+            listener.blockForComplete();
+
+            assertEquals(200, listener.mHttpStatusCode);
+            assertEquals(uploadMethod, listener.mResponseAsString);
+        }
+    }
+
+    // Tests uploading from a channel.
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testUploadChannel() throws Exception {
+        CronetTestActivity activity = launchCronetTestApp();
+        assertNotNull(activity);
+        assertTrue(UploadTestServer.startUploadTestServer());
+        MockUrlRequestJobUtil.addUrlInterceptors();
+
+        TestHttpUrlRequestListener listener = new TestHttpUrlRequestListener();
+        HttpUrlRequest request = createRequest(
+                UploadTestServer.getEchoBodyURL(), listener, activity);
+        setUploadChannel(request, "text/plain", UPLOAD_CHANNEL_DATA,
+                         UPLOAD_CHANNEL_DATA.length());
+        request.start();
+        listener.blockForComplete();
+
+        assertEquals(200, listener.mHttpStatusCode);
+        assertEquals(UPLOAD_CHANNEL_DATA, listener.mResponseAsString);
+    }
+
+    // Tests uploading from a channel in the case a redirect preserves the post
+    // body.  Since channels can't be rewound, the request fails when we try to
+    // rewind it to send the second request.
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testUploadChannelWithRedirect() throws Exception {
+        CronetTestActivity activity = launchCronetTestApp();
+        assertNotNull(activity);
+        assertTrue(UploadTestServer.startUploadTestServer());
+        MockUrlRequestJobUtil.addUrlInterceptors();
+
+        TestHttpUrlRequestListener listener = new TestHttpUrlRequestListener();
+        HttpUrlRequest request = createRequest(
+                UploadTestServer.getRedirectToEchoBody(), listener, activity);
+        setUploadChannel(request, "text/plain", UPLOAD_CHANNEL_DATA,
+                         UPLOAD_CHANNEL_DATA.length());
+        request.start();
+        listener.blockForComplete();
+
+        assertEquals(0, listener.mHttpStatusCode);

[mef, 2014/10/17 20:47:27]

can we get error code here?

[mmenke, 2014/10/17 21:27:15]

I don't think we expose the error code, currently.  Or am I missing something?

Should definitely add a way to do that to the new API, but don't think it's
worth adding to the old one, if we don't already have one.

[mef, 2014/10/17 22:01:34]

In old API there is a method GetErrorCode, which unfortunately does some fuzzy
mapping.
There is also a method GetErrorString which generates a string, which includes
error code, so that may be a decent check.

In new API I've added net error code to UrlRequestException that gets passed to
UrlRequestListener.

[mmenke, 2014/10/20 17:19:37]

Thanks, done!  The test currently depends on both the string error name and the
raw numeric error code. The new API should make it possible for tests to only
depend on one of them - doesn't seem worth updating the old API to allow that,
though.

+    }
+
+    // Tests uploading from a channel when there's a read error.  The body
+    // should be 0-padded.
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testUploadChannelWithReadError() throws Exception {
+        CronetTestActivity activity = launchCronetTestApp();
+        assertNotNull(activity);
+        assertTrue(UploadTestServer.startUploadTestServer());
+        MockUrlRequestJobUtil.addUrlInterceptors();
+
+        TestHttpUrlRequestListener listener = new TestHttpUrlRequestListener();
+        HttpUrlRequest request = createRequest(
+                UploadTestServer.getEchoBodyURL(), listener, activity);
+        setUploadChannel(request, "text/plain", UPLOAD_CHANNEL_DATA,
+                         UPLOAD_CHANNEL_DATA.length() + 2);
+        request.start();
+        listener.blockForComplete();
+
+        assertEquals(200, listener.mHttpStatusCode);
+        assertEquals(UPLOAD_CHANNEL_DATA + "\0\0", listener.mResponseAsString);
+    }
+
+    // Tests Content-Type can be set when uploading from a channel.
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testUploadChannelContentType() throws Exception {
+        String contentTypes[] = {"text/plain", "chicken/spicy"};
+        CronetTestActivity activity = launchCronetTestApp();
+        assertNotNull(activity);
+        assertTrue(UploadTestServer.startUploadTestServer());
+        MockUrlRequestJobUtil.addUrlInterceptors();
+
+        for (String contentType : contentTypes) {
+            TestHttpUrlRequestListener listener = new TestHttpUrlRequestListener();
+            HttpUrlRequest request = createRequest(
+                    UploadTestServer.getEchoHeaderURL("Content-Type"), listener,
+                    activity);
+            setUploadChannel(request, contentType, UPLOAD_CHANNEL_DATA,
+                             UPLOAD_CHANNEL_DATA.length());
+            request.start();
+            listener.blockForComplete();
+
+            assertEquals(200, listener.mHttpStatusCode);
+            assertEquals(contentType, listener.mResponseAsString);
+        }
+    }
+}