[Cronet] Initial implementation of HttpURLConnection

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

Index: components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java
diff --git a/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java b/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java
new file mode 100644
index 0000000000000000000000000000000000000000..e9557e91728a192dc957aebfbb95750ee0b41b3d
--- /dev/null
+++ b/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java
@@ -0,0 +1,188 @@
+// 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.urlconnection;
+
+import android.test.suitebuilder.annotation.SmallTest;
+
+import org.chromium.base.test.util.Feature;
+import org.chromium.cronet_test_apk.CronetTestActivity;
+import org.chromium.cronet_test_apk.CronetTestBase;
+import org.chromium.cronet_test_apk.UploadTestServer;
+import org.chromium.net.urlconnection.CronetURLStreamHandlerFactory;
+
+import java.io.BufferedReader;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.net.HttpURLConnection;
+import java.net.URL;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Example test that just starts the cronet sample.

[mmenke, 2014/11/21 16:53:07]

nit:  capitalize Cronet?

[xunjieli, 2014/11/21 20:30:56]

Done. Just realize that this is a stale comment.

+ */
+public class CronetHttpURLConnectionTest extends CronetTestBase {

[mmenke, 2014/11/21 16:53:07]

Do instrumentation tests support parameterizing test cases?  i.e. run every test
with a variable or enum having one value and another?  I'd really like to be
able to make a test suite that runs all tests with our wrapper and the default
implementation, to validate they behave in the same way.

[xunjieli, 2014/11/21 20:30:56]

I did some search, I think the standard way is to implement our own
instrumentation test runner. See
http://stackoverflow.com/questions/14820175/how-to-pass-an-argument-to-a-andr....
If we choose to do so, we will enable a command line flag on our buildbot to run
the same set of urlconnection tests for the default implementation right? 

+    private CronetTestActivity mActivity;
+
+    @Override
+    protected void setUp() throws Exception {
+        super.setUp();
+        mActivity = launchCronetTestApp();
+        waitForActiveShellToBeDoneLoading();
+        assertTrue(UploadTestServer.startUploadTestServer());
+        // Use Cronet's implementation of HttpURLConnection.
+        CronetURLStreamHandlerFactory factory =
+                new CronetURLStreamHandlerFactory(
+                        mActivity.getApplicationContext());
+        URL.setURLStreamHandlerFactory(factory);
+    }
+
+    @Override
+    protected void tearDown() {
+        UploadTestServer.shutdownUploadTestServer();
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testBasicGet() {
+        try {
+            URL url = new URL(UploadTestServer.getEchoMethodURL());
+            HttpURLConnection urlConnection =
+                    (HttpURLConnection) url.openConnection();
+            assertEquals(200, urlConnection.getResponseCode());

[mmenke, 2014/11/21 16:53:07]

Should have a non-2xx response test, too, with a body.

[xunjieli, 2014/11/21 20:30:56]

Done. Did a 404 response. But I used MockUrlRequestJobFactory, which won't work
for the default implementation.

+            assertEquals("GET\n", getResponseAsString(urlConnection));
+            urlConnection.disconnect();
+        } catch (Exception e) {

[mmenke, 2014/11/21 16:53:07]

Why are these needed?  Mother other tests just let the test fixture get the
exception.

[xunjieli, 2014/11/21 20:30:56]

Done. Yes, you are right.

+            e.printStackTrace();
+            fail();
+        }
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testAddRequestProperty() {
+        try {
+            URL url = new URL(UploadTestServer.getEchoAllHeadersURL());
+            HttpURLConnection urlConnection =
+                    (HttpURLConnection) url.openConnection();
+            urlConnection.addRequestProperty("header1", "foo");
+            urlConnection.addRequestProperty("header2", "bar");

[mmenke, 2014/11/21 16:53:07]

optional:  Using foo-header and bar-header instead of header1 and header2 may
make this test a little easier to read.

[xunjieli, 2014/11/21 20:30:56]

Done.

+            assertEquals(200, urlConnection.getResponseCode());
+            String headers = getResponseAsString(urlConnection);
+            List<String> header1Values =
+                    getRequestHeaderValues(headers, "header1");
+            List<String> header2Values =
+                    getRequestHeaderValues(headers, "header2");
+            assertEquals(1, header1Values.size());
+            assertEquals("foo", header1Values.get(0));
+            assertEquals(1, header1Values.size());
+            assertEquals("bar", header2Values.get(0));
+            urlConnection.disconnect();
+        } catch (Exception e) {
+            e.printStackTrace();
+            fail();
+        }
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testAddRequestPropertyWithSameKey() {
+        try {
+            URL url = new URL(UploadTestServer.getEchoAllHeadersURL());
+            HttpURLConnection urlConnection =
+                    (HttpURLConnection) url.openConnection();
+            urlConnection.addRequestProperty("header-name", "value1");
+            urlConnection.addRequestProperty("header-name", "value2");
+            assertEquals(200, urlConnection.getResponseCode());
+            String headers = getResponseAsString(urlConnection);
+            List<String> actualValues =
+                    getRequestHeaderValues(headers, "header-name");
+            // TODO(xunjieli): Currently Cronet only sends the last header value
+            // if there are multiple entries with the same header key, see
+            // crbug/432719. Fix this test once the bug is fixed.
+            assertEquals(1, actualValues.size());
+            assertEquals("value2", actualValues.get(0));
+            urlConnection.disconnect();
+        } catch (Exception e) {
+            e.printStackTrace();
+            fail();
+        }
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testSetRequestProperty() {
+        try {
+            URL url = new URL(UploadTestServer.getEchoAllHeadersURL());
+            HttpURLConnection urlConnection =
+                    (HttpURLConnection) url.openConnection();
+            urlConnection.setRequestProperty("header-name", "header-value");
+            assertEquals(200, urlConnection.getResponseCode());
+            String headers = getResponseAsString(urlConnection);
+            List<String> actualValues =
+                    getRequestHeaderValues(headers, "header-name");
+            assertEquals(1, actualValues.size());
+            assertEquals("header-value", actualValues.get(0));
+            urlConnection.disconnect();
+        } catch (Exception e) {
+            e.printStackTrace();
+            fail();
+        }
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testAddAndSetRequestProperty() {
+        try {
+            URL url = new URL(UploadTestServer.getEchoAllHeadersURL());
+            HttpURLConnection urlConnection =
+                    (HttpURLConnection) url.openConnection();
+            urlConnection.addRequestProperty("header-name", "value1");
+            urlConnection.setRequestProperty("header-name", "value2");
+            assertEquals(200, urlConnection.getResponseCode());
+            String headers = getResponseAsString(urlConnection);
+            List<String> actualValues =
+                    getRequestHeaderValues(headers, "header-name");
+            assertEquals(1, actualValues.size());
+            assertEquals("value2", actualValues.get(0));
+            urlConnection.disconnect();
+        } catch (Exception e) {
+            e.printStackTrace();
+            fail();
+        }
+    }

[mmenke, 2014/11/21 16:53:07]

Other test suggestion:  It's an issue with out other tests, too, but none of
these tests results in multiple body reads.  Fine to think about this in another
CL, since this may require some test changes.

[mmenke, 2014/11/21 16:53:07]

Other test suggestion:  connect -> disconnect -> try reading.

[xunjieli, 2014/11/21 20:30:56]

Done. Currently the default implementation throws an SocketException
(java.net.SocketException: sendto failed: EBADF Bad file number), and ours
proceed as if everything's fine. What is the best way to deal with this? Should
we add a boolean flag in CronetHttpURLConnection that basically says whether the
connection has been disconnected, if it is, throw an exception when user try to
read response code/message/body?

[xunjieli, 2014/11/21 20:30:56]

What do you mean by multiple body reads? read the inputstream after it has been
read once?

[mmenke, 2014/11/21 21:26:12]

I was thinking multiple reads at the CronetURLRequest, actually.

Multiple reads from the InputStream, and making sure we exercise both read
methods, is also something we should be doing (Not clear which BufferedReader
uses, or if it reads one byte at a time behind the scenes or not - best to test
them manually).

Reading with the single byte method, trying to read more bytes at once than are
currently available, reading fewer bytes than are currently available (And then
reading more, when we're at an offset), reading the exact number of bytes
currently available should all be tested.

ByteBuffer makes some of these variations less exciting than they'd be if we
didn't have it, but should still test them as if we were doing all that logic
ourselves.

+
+    /**
+     * Helper method to extract response body as a string for testing.
+     */
+    private String getResponseAsString(HttpURLConnection connection)
+            throws Exception {
+        InputStream in = connection.getInputStream();
+        BufferedReader reader = new BufferedReader(new InputStreamReader(in));
+        StringBuilder out = new StringBuilder();
+        String line;
+        while ((line = reader.readLine()) != null) {
+            out.append(line + "\n");

[mmenke, 2014/11/21 16:53:07]

Why append a line break here?

[xunjieli, 2014/11/21 20:30:56]

so reader.readLine() returns me a line without the original line breaks. I add
one back so i can parse headers. Otherwise, headers will be like
"header1:value1header2:value2".

[mmenke, 2014/11/21 21:26:12]

Ahh...Suggest just reading the data directly, so we don't have to muck with
that.  Checking for "GET\n", for instance, seems weird, and may have the
potential to overlook certain bugs.

+        }
+        return out.toString();
+    }
+
+    /**
+     * Helper method to extract a list of header values with the give header
+     * name.
+     */
+    private List<String> getRequestHeaderValues(String allHeaders,
+            String headerName) {
+        Pattern pattern = Pattern.compile(headerName + ":\\s(.*)\\n");
+        Matcher matcher = pattern.matcher(allHeaders);
+        List<String> headerValues = new ArrayList<String>();
+        while (matcher.find()) {
+            headerValues.add(matcher.group(1));
+        }
+        return headerValues;
+    }
+}