Fix flaky tests in CronetUrlRequestContextTest

components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.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.content.Context;
 import android.content.ContextWrapper;
 import android.os.ConditionVariable;
 import android.os.Handler;
 import android.os.Looper;
 import android.os.StrictMode;
 import android.test.suitebuilder.annotation.SmallTest;
 
 import org.chromium.base.FileUtils;
-import org.chromium.base.Log;
 import org.chromium.base.PathUtils;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.base.test.util.Feature;
 import org.chromium.net.TestUrlRequestCallback.ResponseStep;
 import org.chromium.net.impl.CronetLibraryLoader;
 import org.chromium.net.impl.CronetUrlRequestContext;
 import org.chromium.net.test.EmbeddedTestServer;
 
 import java.io.BufferedReader;
 import java.io.File;
 import java.io.FileReader;
 import java.util.Arrays;
 import java.util.LinkedList;
 import java.util.NoSuchElementException;
 import java.util.concurrent.Executor;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ThreadFactory;
 
 /**
  * Test CronetEngine.
  */
 @JNINamespace("cronet")
 public class CronetUrlRequestContextTest extends CronetTestBase {
     // URLs used for tests.
     private static final String MOCK_CRONET_TEST_FAILED_URL =
             "http://mock.failed.request/-2";
     private static final String MOCK_CRONET_TEST_SUCCESS_URL =
             "http://mock.http/success.txt";
-    private static final String TAG = "RequestContextTest";
     private static final int MAX_FILE_SIZE = 1000000000;
     private static final int NUM_EVENT_FILES = 10;
 
     private EmbeddedTestServer mTestServer;
     private String mUrl;
     private String mUrl404;
     private String mUrl500;
     CronetTestFramework mTestFramework;

[pauljensen, 2016/08/18 15:05:29]

Please remove this variable.  It's the root cause of this how this bug slipped
in.  It's no longer used in a class-wide scope and should be replaced by
method-scoped variables so as to avoid this bug in the future, and also better
follow our coding style of keeping variables in the narrowest scope.

[kapishnikov, 2016/08/18 16:18:33]

Done.

 
     // Thread on which network quality listeners should be notified.
     private Thread mNetworkQualityThread;
 
     @Override
     protected void setUp() throws Exception {
         super.setUp();
         mTestServer = EmbeddedTestServer.createAndStartDefaultServer(getContext());
         mUrl = mTestServer.getURL("/echo?status=200");
         mUrl404 = mTestServer.getURL("/echo?status=404");
@@ skipping 29 matching lines @@
                     new UrlRequest.Builder(mUrl, mCallback, mCallback.getExecutor(), cronetEngine);
             urlRequestBuilder.build().start();
             mCallback.blockForDone();
         }
     }
 
     /**
      * Callback that shutdowns the request context when request has succeeded
      * or failed.
      */
-    class ShutdownTestUrlRequestCallback extends TestUrlRequestCallback {
+    static class ShutdownTestUrlRequestCallback extends TestUrlRequestCallback {
+        private final CronetEngine mCronetEngine;
+        private ConditionVariable mCallbackCompletionBlock = new ConditionVariable();

[pauljensen, 2016/08/18 15:05:29]

final

[kapishnikov, 2016/08/18 16:18:33]

Done.

+
+        ShutdownTestUrlRequestCallback(CronetEngine cronetEngine) {
+            mCronetEngine = cronetEngine;
+        }
+
         @Override
         public void onSucceeded(UrlRequest request, UrlResponseInfo info) {
-            // TODO: Remove logging when http://crbug.com/596929 & http://crbug.com/635025 is fixed.
-            Log.d(TAG, "ShutdownTestUrlRequestCallback.onSucceeded() has started");
             super.onSucceeded(request, info);
-            Log.d(TAG, "ShutdownTestUrlRequestCallback.onSucceeded() before calling shutdown");
-            mTestFramework.mCronetEngine.shutdown();
-            Log.d(TAG, "ShutdownTestUrlRequestCallback.hasFinished() has finished");
+            mCronetEngine.shutdown();
+            mCallbackCompletionBlock.open();
         }
 
         @Override
         public void onFailed(UrlRequest request, UrlResponseInfo info, UrlRequestException error) {
-            // TODO: Remove logging when http://crbug.com/596929 & http://crbug.com/635025 is fixed.
-            Log.d(TAG, "ShutdownTestUrlRequestCallback.onFailed() has started");
             super.onFailed(request, info, error);
-            Log.d(TAG, "ShutdownTestUrlRequestCallback.onFailed() before calling shutdown");
-            mTestFramework.mCronetEngine.shutdown();
-            Log.d(TAG, "ShutdownTestUrlRequestCallback.onFailed() has finished.");
+            mCronetEngine.shutdown();
+            mCallbackCompletionBlock.open();
+        }
+
+        void blockForCallbackToComplete() {

[pauljensen, 2016/08/18 15:05:29]

add: // Wait for request completion callback.

[kapishnikov, 2016/08/18 16:18:33]

Done.

+            mCallbackCompletionBlock.block();
         }
     }
 
     static class TestExecutor implements Executor {
         private final LinkedList<Runnable> mTaskQueue = new LinkedList<Runnable>();
 
         @Override
         public void execute(Runnable task) {
             mTaskQueue.add(task);
         }
@@ skipping 212 matching lines @@
                 != EffectiveConnectionType.EFFECTIVE_CONNECTION_TYPE_UNKNOWN);
 
         mTestFramework.mCronetEngine.shutdown();
     }
 
     @SmallTest
     @Feature({"Cronet"})
     // TODO: Remove the annotation after fixing http://crbug.com/637979 & http://crbug.com/637972
     @OnlyRunNativeCronet
     public void testShutdown() throws Exception {
-        Log.i(TAG, "testShutdown() has started");
         mTestFramework = startCronetTestFramework();
-        TestUrlRequestCallback callback = new ShutdownTestUrlRequestCallback();
+        ShutdownTestUrlRequestCallback callback =
+                new ShutdownTestUrlRequestCallback(mTestFramework.mCronetEngine);
         // Block callback when response starts to verify that shutdown fails
         // if there are active requests.
         callback.setAutoAdvance(false);
         UrlRequest.Builder urlRequestBuilder = new UrlRequest.Builder(
                 mUrl, callback, callback.getExecutor(), mTestFramework.mCronetEngine);
         UrlRequest urlRequest = urlRequestBuilder.build();
         urlRequest.start();
         try {
             mTestFramework.mCronetEngine.shutdown();
             fail("Should throw an exception");
@@ skipping 21 matching lines @@
         } catch (Exception e) {
             assertEquals("Cannot shutdown with active requests.",
                          e.getMessage());
         }
 
         // May not have read all the data, in theory. Just enable auto-advance
         // and finish the request.
         callback.setAutoAdvance(true);
         callback.startNextRead(urlRequest);
         callback.blockForDone();
-        // TODO: Remove sleep when http://crbug.com/596929 is fixed.
-        // The sleep gives the thread that shuts down the engine time to complete.
-        // See http://crbug.com/596929
-        Thread.sleep(3000);
-        Log.i(TAG, "testShutdown() has finished");
+        callback.blockForCallbackToComplete();
+        callback.shutdownExecutor();
     }
 
     @SmallTest
     @Feature({"Cronet"})
     @OnlyRunNativeCronet
     public void testShutdownDuringInit() throws Exception {
         final CronetTestFramework testFramework = startCronetTestFrameworkAndSkipLibraryInit();
         final ConditionVariable block = new ConditionVariable(false);
 
         // Post a task to main thread to block until shutdown is called to test
@@ skipping 73 matching lines @@
         } catch (Exception e) {
             assertEquals("Engine is shut down.", e.getMessage());
         }
     }
 
     @SmallTest
     @Feature({"Cronet"})
     // TODO: Remove the annotation after fixing http://crbug.com/637972
     @OnlyRunNativeCronet
     public void testShutdownAfterError() throws Exception {
-        Log.i(TAG, "testShutdownAfterError() has started");
         mTestFramework = startCronetTestFramework();
-        TestUrlRequestCallback callback = new ShutdownTestUrlRequestCallback();
+        ShutdownTestUrlRequestCallback callback =
+                new ShutdownTestUrlRequestCallback(mTestFramework.mCronetEngine);
         UrlRequest.Builder urlRequestBuilder = new UrlRequest.Builder(MOCK_CRONET_TEST_FAILED_URL,
                 callback, callback.getExecutor(), mTestFramework.mCronetEngine);
         urlRequestBuilder.build().start();
         callback.blockForDone();
         assertTrue(callback.mOnErrorCalled);
-        // TODO: Remove sleep when http://crbug.com/635025 is fixed.
-        // The sleep gives the thread that shuts down the engine time to complete.
-        // See http://crbug.com/637986
-        Thread.sleep(3000);
-        Log.i(TAG, "testShutdownAfterError() has finished");
+        callback.blockForCallbackToComplete();
+        callback.shutdownExecutor();
     }
 
     @SmallTest
     @Feature({"Cronet"})
     public void testShutdownAfterCancel() throws Exception {
         mTestFramework = startCronetTestFramework();
         TestUrlRequestCallback callback = new TestUrlRequestCallback();
         // Block callback when response starts to verify that shutdown fails
         // if there are active requests.
         callback.setAutoAdvance(false);
@@ skipping 857 matching lines @@
                     }
                 }.start();
                 otherThreadDone.block();
                 builder.build().shutdown();
                 uiThreadDone.open();
             }
         });
         assertTrue(uiThreadDone.block(1000));
     }
 }