Fix flaky NQE cronet test

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 static org.chromium.base.CollectionUtil.newHashSet;
 
 import android.content.Context;
 import android.content.ContextWrapper;
@@ skipping 11 matching lines @@
 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.HashSet;
 import java.util.LinkedList;
 import java.util.NoSuchElementException;
 import java.util.concurrent.Executor;
+import java.util.concurrent.Executors;
 
 /**
  * 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 =
@@ skipping 79 matching lines @@
                 }
             } catch (NoSuchElementException e) {
             }
         }
     }
 
     static class TestNetworkQualityListener
             implements NetworkQualityRttListener, NetworkQualityThroughputListener {
         int mRttObservationCount;
         int mThroughputObservationCount;
+        ConditionVariable mWaitForThroughput = null;

[xunjieli, 2016/05/26 17:55:30]

nit: maybe get rid of "=null", and add "final" keyword. And move the final
member above the non-final ones.

[tbansal1, 2016/05/26 18:02:59]

Done.

+
+        TestNetworkQualityListener(ConditionVariable waitForThroughput) {
+            mWaitForThroughput = waitForThroughput;
+        }
 
         @Override
         public void onRttObservation(int rttMs, long when, int source) {
             mRttObservationCount++;
         }
 
         @Override
         public void onThroughputObservation(int throughputKbps, long when, int source) {
+            if (mWaitForThroughput != null) {
+                mWaitForThroughput.open();
+            }
             mThroughputObservationCount++;
         }
 
         public int rttObservationCount() {
             return mRttObservationCount;
         }
 
         public int throughputObservationCount() {
             return mThroughputObservationCount;
         }
@@ skipping 64 matching lines @@
         // Verify that the request is successful and that the Data Reduction
         // Proxy logic configured to use the test server as its proxy.
         assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
         assertEquals(serverHostPort, callback.mResponseInfo.getProxyServer());
         assertEquals("http://DomainThatDoesnt.Resolve/datareductionproxysuccess.txt",
                 callback.mResponseInfo.getUrl());
     }
 
     @SmallTest
     @Feature({"Cronet"})
-    // TODO(xunjieli): Remove annotation after crbug.com/539519 is fixed.
-    @SuppressWarnings("deprecation")
     public void testRealTimeNetworkQualityObservationsNotEnabled() throws Exception {
         mTestFramework = startCronetTestFramework();
-        TestNetworkQualityListener networkQualityListener = new TestNetworkQualityListener();
+        TestNetworkQualityListener networkQualityListener = new TestNetworkQualityListener(null);
         try {
             mTestFramework.mCronetEngine.addRttListener(networkQualityListener);
             fail("Should throw an exception.");
         } catch (IllegalStateException e) {
         }
         try {
             mTestFramework.mCronetEngine.addThroughputListener(networkQualityListener);
             fail("Should throw an exception.");
         } catch (IllegalStateException e) {
         }
         TestUrlRequestCallback callback = new TestUrlRequestCallback();
         UrlRequest urlRequest =
                 mTestFramework.mCronetEngine.createRequest(mUrl, callback, callback.getExecutor());
         urlRequest.start();
         callback.blockForDone();
         assertEquals(0, networkQualityListener.rttObservationCount());
         assertEquals(0, networkQualityListener.throughputObservationCount());
         mTestFramework.mCronetEngine.shutdown();
     }
 
     @SmallTest
     @Feature({"Cronet"})
-    // TODO(xunjieli): Remove annotation after crbug.com/539519 is fixed.
-    @SuppressWarnings("deprecation")
     public void testRealTimeNetworkQualityObservationsListenerRemoved() throws Exception {
         mTestFramework = startCronetTestFramework();
         TestExecutor testExecutor = new TestExecutor();
-        TestNetworkQualityListener networkQualityListener = new TestNetworkQualityListener();
+        TestNetworkQualityListener networkQualityListener = new TestNetworkQualityListener(null);
         mTestFramework.mCronetEngine.enableNetworkQualityEstimatorForTesting(
                 true, true, testExecutor);
         mTestFramework.mCronetEngine.addRttListener(networkQualityListener);
         mTestFramework.mCronetEngine.addThroughputListener(networkQualityListener);
         mTestFramework.mCronetEngine.removeRttListener(networkQualityListener);
         mTestFramework.mCronetEngine.removeThroughputListener(networkQualityListener);
         TestUrlRequestCallback callback = new TestUrlRequestCallback();
         UrlRequest urlRequest =
                 mTestFramework.mCronetEngine.createRequest(mUrl, callback, callback.getExecutor());
         urlRequest.start();
         callback.blockForDone();
         testExecutor.runAllTasks();
         assertEquals(0, networkQualityListener.rttObservationCount());
         assertEquals(0, networkQualityListener.throughputObservationCount());
         mTestFramework.mCronetEngine.shutdown();
     }
 
-    /*
     @SmallTest
     @Feature({"Cronet"})
-    // TODO(xunjieli): Remove annotation after crbug.com/539519 is fixed.
-    @SuppressWarnings("deprecation")
-    */
-    @FlakyTest(message = "http://crbug.com/614227")
     public void testRealTimeNetworkQualityObservations() throws Exception {
         mTestFramework = startCronetTestFramework();
-        TestExecutor testExecutor = new TestExecutor();
-        TestNetworkQualityListener networkQualityListener = new TestNetworkQualityListener();
+        Executor testExecutor = Executors.newSingleThreadExecutor();
+        ConditionVariable waitForThroughput = new ConditionVariable();
+        TestNetworkQualityListener networkQualityListener =
+                new TestNetworkQualityListener(waitForThroughput);
         mTestFramework.mCronetEngine.enableNetworkQualityEstimatorForTesting(
                 true, true, testExecutor);
         mTestFramework.mCronetEngine.addRttListener(networkQualityListener);
         mTestFramework.mCronetEngine.addThroughputListener(networkQualityListener);
         TestUrlRequestCallback callback = new TestUrlRequestCallback();
         UrlRequest urlRequest =
                 mTestFramework.mCronetEngine.createRequest(mUrl, callback, callback.getExecutor());
         urlRequest.start();
         callback.blockForDone();
-        testExecutor.runAllTasks();
+
+        // Throughput observation is posted to the network quality estimator and its observers
+        // after the UrlRequest is completed.
+        waitForThroughput.block();

[xunjieli, 2016/05/26 17:55:30]

Is throughtput observation always going to be posted after the RTT observation?

[tbansal1, 2016/05/26 18:02:59]

Yes.

[xunjieli, 2016/05/26 18:06:49]

Please add a comment above in "public void onThroughputObservation" of
TestNetworkQualityListener to make a note of this. It is not obvious why you
want to wait for throughput instead of both rtt and throughput.

[tbansal1, 2016/05/26 18:21:40]

I expanded on the comment here, instead of in onThroughputObservation. Let me
know if this is still vague.

         assertTrue(networkQualityListener.rttObservationCount() > 0);
         assertTrue(networkQualityListener.throughputObservationCount() > 0);
         mTestFramework.mCronetEngine.shutdown();
     }
 
     private static class TestRequestFinishedListener
             implements CronetEngine.RequestFinishedListener {
         private UrlRequestInfo mRequestInfo = null;
 
         @Override
@@ skipping 845 matching lines @@
                     }
                 }.start();
                 otherThreadDone.block();
                 builder.build().shutdown();
                 uiThreadDone.open();
             }
         });
         assertTrue(uiThreadDone.block(1000));
     }
 }