[Cronet] Unit test refactoring and fixes

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

 // 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.net;
 
 import android.test.AndroidTestCase;
 
 import org.chromium.base.PathUtils;
 
 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
 import java.lang.reflect.Method;
 import java.net.URL;
 
 /**
  * Base test class for all CronetTest based tests.
  */
 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;
+            }
+        }
     }
 
     /**
      * Starts the CronetTest framework.
      */
     protected CronetTestFramework startCronetTestFramework() {
         return startCronetTestFrameworkWithUrlAndCronetEngineBuilder(null, null);
     }
 
     /**
      * Starts the CronetTest framework and loads the given URL. The URL can be
      * null.
      */
     protected CronetTestFramework startCronetTestFrameworkWithUrl(String url) {
         return startCronetTestFrameworkWithUrlAndCronetEngineBuilder(url, null);
     }
 
     /**
      * Starts the CronetTest framework using the provided CronetEngine.Builder
      * and loads the given URL. The URL can be null.
      */
     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;
     }
 
     /**
      * Starts the CronetTest framework appending the provided command line
      * arguments and loads the given URL. The URL can be null.
      */
     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);
     }
 
     /**
      * Starts the CronetTest framework for the legacy API.
      * @param url if non-null, a request will be made with that url.
      */
     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;
         }
         try {
             Method method = getClass().getMethod(getName(), (Class[]) null);
             if (method.isAnnotationPresent(CompareDefaultWithCronet.class)) {
                 // 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.
                 super.runTest();
             }
         } catch (Throwable e) {
             throw new Throwable("CronetTestBase#runTest failed.", e);
         }
     }
 
+    /**
+     * 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 {
     }
 
     @Target(ElementType.METHOD)
     @Retention(RetentionPolicy.RUNTIME)
     public @interface OnlyRunCronetHttpURLConnection {
     }
 
 }