[Cronet] Unit test refactoring and fixes

components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java

Index: components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java
diff --git a/components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java b/components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java
index bd22a6ddc26056642d82ef604690ac9b5246a509..d542d2d21cd3e67ef7d4c2ea1c9687b4ef884e3d 100644
--- a/components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java
+++ b/components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java
@@ -21,12 +21,41 @@ import java.net.URL;
 public class CronetTestBase extends AndroidTestCase {
     private static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "cronet_test";
 
-    private CronetTestFramework mCronetTestFramework;
+    /**
+     * Package name that contains Cronet URLConnection tests.
+     */
+    private static final String URLCONNECTION_PACKAGE = "org.chromium.net.urlconnection";
+
+    /**
+     * Indicates that the test should run with CronetHttpURLConnection. If the flag is
+     * <code>true</code>, the next call to any <code>startCronetTestFramework...()</code> method
+     * will replace the global URLStreamHandlerFactory factory with the Cronet one.
+     */
+    private boolean mTestingCronetHttpURLConnection = false;
+
+    /**
+     * <code>URL.setURLStreamHandlerFactory()</code> method can only be called once. This flag

[xunjieli, 2015/12/07 23:09:10]

nit: maybe use {@code true} ? That's how it was used elsewhere, so we should
probably stick to one style.

[kapishnikov, 2015/12/08 01:09:37]

Will fix it.

+     * indicates that the method has already been called and should not be called again. The flag
+     * is needed for tests that test CronetHttpURLConnection and start the Cronet Test Framework
+     * multiple times.
+     */
+    private boolean mURLStreamHandlerFactoryReplaced = false;
 
     @Override
     protected void setUp() throws Exception {
         super.setUp();
         PathUtils.setPrivateDataDirectorySuffix(PRIVATE_DATA_DIRECTORY_SUFFIX, getContext());
+        CronetTestFramework.prepareTestStorage(getContext());
+
+        // If the test method has the OnlyRunCronetHttpURLConnection annotation then
+        // set mTestingCronetHttpURLConnection to true, so the next startCronetTestFramework...()
+        // method will replace URLStreamHandlerFactory.
+        if (getClass().getPackage().getName().equals(URLCONNECTION_PACKAGE)) {
+            Method method = getClass().getMethod(getName(), (Class[]) null);
+            if (method.isAnnotationPresent(OnlyRunCronetHttpURLConnection.class)) {
+                mTestingCronetHttpURLConnection = true;
+            }
+        }
     }
 
     /**
@@ -50,8 +79,9 @@ public class CronetTestBase extends AndroidTestCase {
      */
     protected CronetTestFramework startCronetTestFrameworkWithUrlAndCronetEngineBuilder(
             String url, CronetEngine.Builder builder) {
-        mCronetTestFramework = new CronetTestFramework(url, null, getContext(), builder);
-        return mCronetTestFramework;
+        CronetTestFramework framework = new CronetTestFramework(url, null, getContext(), builder);
+        switchToCronetHttpURLConnectionTestingIfNeeded(framework);
+        return framework;
     }
 
     /**
@@ -60,17 +90,17 @@ public class CronetTestBase extends AndroidTestCase {
      */
     protected CronetTestFramework startCronetTestFrameworkWithUrlAndCommandLineArgs(
             String url, String[] commandLineArgs) {
-        mCronetTestFramework = new CronetTestFramework(url, commandLineArgs, getContext(), null);
-        return mCronetTestFramework;
+        CronetTestFramework framework =
+                new CronetTestFramework(url, commandLineArgs, getContext(), null);
+        switchToCronetHttpURLConnectionTestingIfNeeded(framework);
+        return framework;
     }
 
     // Helper method to tell the framework to skip library init during construction.
     protected CronetTestFramework startCronetTestFrameworkAndSkipLibraryInit() {
         String[] commandLineArgs = {
                 CronetTestFramework.LIBRARY_INIT_KEY, CronetTestFramework.LibraryInitType.NONE};
-        mCronetTestFramework =
-                startCronetTestFrameworkWithUrlAndCommandLineArgs(null, commandLineArgs);
-        return mCronetTestFramework;
+        return startCronetTestFrameworkWithUrlAndCommandLineArgs(null, commandLineArgs);
     }
 
     /**
@@ -80,15 +110,12 @@ public class CronetTestBase extends AndroidTestCase {
     protected CronetTestFramework startCronetTestFrameworkForLegacyApi(String url) {
         String[] commandLineArgs = {
                 CronetTestFramework.LIBRARY_INIT_KEY, CronetTestFramework.LibraryInitType.LEGACY};
-        mCronetTestFramework =
-                startCronetTestFrameworkWithUrlAndCommandLineArgs(url, commandLineArgs);
-        return mCronetTestFramework;
+        return startCronetTestFrameworkWithUrlAndCommandLineArgs(url, commandLineArgs);
     }
 
     @Override
     protected void runTest() throws Throwable {
-        if (!getClass().getPackage().getName().equals(
-                "org.chromium.net.urlconnection")) {
+        if (!getClass().getPackage().getName().equals(URLCONNECTION_PACKAGE)) {
             super.runTest();
             return;
         }
@@ -98,12 +125,9 @@ public class CronetTestBase extends AndroidTestCase {
                 // Run with the default HttpURLConnection implementation first.
                 super.runTest();
                 // Use Cronet's implementation, and run the same test.
-                URL.setURLStreamHandlerFactory(mCronetTestFramework.mStreamHandlerFactory);
-                super.runTest();
-            } else if (method.isAnnotationPresent(
-                    OnlyRunCronetHttpURLConnection.class)) {
-                // Run only with Cronet's implementation.
-                URL.setURLStreamHandlerFactory(mCronetTestFramework.mStreamHandlerFactory);
+                this.tearDown();
+                mTestingCronetHttpURLConnection = true;
+                this.setUp();
                 super.runTest();
             } else {

[xunjieli, 2015/12/07 23:09:10]

I'd prefer to keep CronetHttpURLConnection logic all in one place here. It's
slightly cleaner and easier to reason about.  We can avoid having to call
switchToCronetHttpURLConnectionTestingIfNeeded, and set
mTestingCronetHttpURLConnection. 

[kapishnikov, 2015/12/08 01:09:37]

The problem with keeping all logic inside runTest() method is that the method is
called before the actual test is run; therefore, CronetTestFramework must be
created in the setUp() method. Otherwise, mCronetTestFramework will be null and
URL.setURLStreamHandlerFactory(.mStreamHandlerFactory) will fail with
NullPointerException. Instantiating CronetEngin in setUp() imposes limitation
that all tests have to share the same Cronet configuration. Also, two engines
cannot be instantiated in the same test, e.g. for persistence testing.

[xunjieli, 2015/12/08 13:40:25]

I don't really like this. The @CompareDefaultWithCronet or
@OnlyRunCronetHttpURLConnection will only ever need one CronetEngine instance
anyway. The tests don't share CronetEngines, since the default stack doesn't use
CronetEngine.

I think this CL tries to do too many things at once, which makes reviewing hard.
Could you do a change list that's only enough to solve the host resolver
refactoring first? And do other changes in small chunks in different CLs? 

                 // For all other tests.
@@ -114,6 +138,44 @@ public class CronetTestBase extends AndroidTestCase {
         }
     }
 
+    /**
+     * Registers test host resolver for testing with the new API.
+     */
+    protected void registerHostResolver(CronetTestFramework framework) {
+        registerHostResolver(framework, false);
+    }
+
+    /**
+     * Registers test host resolver.
+     *
+     * @param isLegacyAPI true if the test should use the legacy API.
+     */
+    protected void registerHostResolver(CronetTestFramework framework, boolean isLegacyAPI) {
+        long urlRequestContextAdapter;
+        if (isLegacyAPI) {
+            urlRequestContextAdapter = ((ChromiumUrlRequestFactory) framework.mRequestFactory)
+                                               .getRequestContext()
+                                               .getUrlRequestContextAdapter();
+        } else {
+            urlRequestContextAdapter = ((CronetUrlRequestContext) framework.mCronetEngine)
+                                               .getUrlRequestContextAdapter();
+        }
+        NativeTestServer.registerHostResolverProc(urlRequestContextAdapter, isLegacyAPI);
+    }
+
+    /**
+     * Replaces global URLStreamHandlerFactory with the Cronet one if it is required by the
+     * running test.
+     *
+     * @param framework the Cronet Test Framework to use for StreamHandlerFactory retreival.
+     */
+    private void switchToCronetHttpURLConnectionTestingIfNeeded(CronetTestFramework framework) {
+        if (mTestingCronetHttpURLConnection && !mURLStreamHandlerFactoryReplaced) {
+            URL.setURLStreamHandlerFactory(framework.mStreamHandlerFactory);
+            mURLStreamHandlerFactoryReplaced = true;
+        }
+    }
+
     @Target(ElementType.METHOD)
     @Retention(RetentionPolicy.RUNTIME)
     public @interface CompareDefaultWithCronet {