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 252 matching lines @@
         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.

[xunjieli, 2016/05/24 18:26:38]

Since you are here, do you mind getting rid of this TODO and @SuppressWarnings
for this test and the three tests above this one?

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

Done.

     @SuppressWarnings("deprecation")
-    */
-    @FlakyTest(message = "http://crbug.com/614227")
     public void testRealTimeNetworkQualityObservations() throws Exception {
         mTestFramework = startCronetTestFramework();
         TestExecutor testExecutor = new TestExecutor();
         TestNetworkQualityListener networkQualityListener = new TestNetworkQualityListener();
         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 destroyed. Throughput observation corresponding to the first
+        // UrlRequest would post before the second UrlRequest starts.

[xunjieli, 2016/05/24 18:26:39]

I don't quite understand the threading model. The throughput observations are
posted to |testExecutor| which is different from the executors used by the
UrlRequests' callbacks. Do you mean that UrlRequest's throughput observation
will be posted from the *network thread* to the Java executor before the second
UrlRequest is started on the network thread?

Is it better then to explicitly wait until throughput observation is received?

Something like:

ConditionalVariable waitForThroughput = new ConditionalVariable();

TestNetworkQualityListener networkQualityListener = new
TestNetworkQualityListener() {
        @Override
        public void onThroughputObservation(int throughputKbps, long when, int
source) {
            super.onThroughputObservation(throughputKps, when, source);
            waitForThroughput.open();
        }
}

....

callback.blockForDone();
waitForThroughput.block();
....

[tbansal1, 2016/05/25 00:02:22]

Context on how NQE works: After the request completes, 
ThroughputAnalyzer posts the throughput observation to NQE on IO thread. NQE
then  notifies its observers. Cronet is one of the observer and when Cronet gets
the notification, it posts the observation to all ThroughputListeners on
NQEExecutor (including TestNetworkQualityListener).

So, in the test, we need a way to wait for the first IO post to execute. As soon
as that executes, we need to call testExecutor.runAllTasks(). 

The problem is that the post from ThroughputAnalyzer to NQE may execute after
callback.blockForDone().

I am not sure how to wait in the test to make sure that the first IO post is
executed.

[xunjieli, 2016/05/25 20:37:50]

I thought opening the conditional variable in onThroughputObservation will make
sure the IO is completed?


        @Override
        public void onThroughputObservation(int throughputKbps, long when, int
source) {
            super.onThroughputObservation(throughputKps, when, source);
            // Open the conditional variable here.
            waitForThroughput.open();
        }
}
callback.blockForDone();
// Wait for conditional variable to open
waitForThroughput.block();
testExecutor.runAllTasks();


Does this solve the flakiness? 

[tbansal1, 2016/05/25 21:04:13]

You are right, it makes sure that IO is completed. That pushes the observation
to Cronet. However, testExecutor (aka NQEExecutor) also needs to run so that the
observation is pushed from Cronet to the TestListener.
https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...
I tried this before. This would forever block on waitForThroughput  (in some
cases based on race) because it is waiting for the testExecutor to post the
observation from NQE to the TestListener.

One other really hacky fix is (just to illustrate what actually works):
callback.blockForDone();
waitForThroughput.block(5000); // Wait for 5 seconds, the IO task should be
posted in the meantime.
testExecutor.runAllTasks(); // This will post the observation from Cronet to
test listener and open waitForThroughput.
waitForThroughput.block(); // This should unblock immediately because
waitForThroughput would be open.

[xunjieli, 2016/05/26 13:54:47]

Ah, I see. I didn't realize TestExecutor runs tasks on current thread(which is
the test thread). Since we are blocking test thread, we will be essentially
blocking TestExecutor. 

It should be easy to solve this. Just create a executor that runs tasks on a
separate thread(you can use Executors.newSingleThreadExecutor()), and then
blocking test thread will be fine. Blocking test thread will ensure the IO
completes.

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

Acknowledged.

+        for (int i = 0; i < 2; ++i) {
+            TestUrlRequestCallback callback = new TestUrlRequestCallback();
+            UrlRequest urlRequest = mTestFramework.mCronetEngine.createRequest(
+                    mUrl, callback, callback.getExecutor());
+            urlRequest.start();
+            callback.blockForDone();
+            testExecutor.runAllTasks();
+        }
+
         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));
     }
 }