[Cronet] Make metrics reporting happen after terminal callbacks.

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

Index: components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java
diff --git a/components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java b/components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java
index dab3af9725da13d0c8a56ee622b54f0564caa238..c71559b5308225692f75b873ae55a4d234fad0c2 100644
--- a/components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java
+++ b/components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java
@@ -30,6 +30,24 @@ public class RequestFinishedInfoTest extends CronetTestBase {
     private EmbeddedTestServer mTestServer;
     private String mUrl;
 
+    // A subclass of TestRequestFinishedListener to additionally assert that UrlRequest.Callback's

[mgersh, 2017/05/01 15:29:22]

What do you think about modifying TestRequestFinishedListener to do this instead
of having a subclass and separate tests that use it? Some of these new tests are
essentially copies of existing ones with an extra assert, and in those cases I
don't think we need two separate tests.

[xunjieli, 2017/05/01 19:17:16]

This subclass associates with exactly on UrlRequest.callback, but a
RequestFinishedListener can be used to report metrics for many UrlRequests. I
think it's better to keep them separate so we can use
TestRequestFinishedListener for multiple requests scenario.
I started by modifying the existing tests, but found myself adding tests for
scenarios that weren't covered (e.g. error from Java). I think it's slightly
more self-contained to do it this way, so we do not modify some test cases to
use AssertCallbackDoneRequestFinishedListener and but not others?

+    // terminal callbacks have been invoked at the time of onRequestFinished(). Note that this
+    // listener runs callbacks on a separated thread, so tests use this is racy by design. See

[mef, 2017/04/28 21:16:55]

nit: could you rephrase 'so tests use this is racy by design'.

[xunjieli, 2017/05/01 19:17:16]

Done.

+    // crbug.com/710877.
+    private static class AssertCallbackDoneRequestFinishedListener
+            extends TestRequestFinishedListener {
+        private final TestUrlRequestCallback mCallback;
+        public AssertCallbackDoneRequestFinishedListener(TestUrlRequestCallback callback) {
+            mCallback = callback;
+        }
+
+        @Override
+        public void onRequestFinished(RequestFinishedInfo requestInfo) {
+            assertTrue(mCallback.isDone());
+            super.onRequestFinished(requestInfo);
+        }
+    };
+
     @Override
     protected void setUp() throws Exception {
         super.setUp();
@@ -367,4 +385,137 @@ public class RequestFinishedInfoTest extends CronetTestBase {
         assertEquals(sentByteCount, (long) metrics.getSentByteCount());
         assertEquals(receivedByteCount, (long) metrics.getReceivedByteCount());
     }
+
+    @SmallTest
+    @OnlyRunNativeCronet
+    @Feature({"Cronet"})
+    @SuppressWarnings("deprecation")
+    public void testOrderSuccessfulRequest() throws Exception {
+        mTestFramework = startCronetTestFramework();
+        final TestUrlRequestCallback callback = new TestUrlRequestCallback();
+        TestRequestFinishedListener requestFinishedListener =
+                new AssertCallbackDoneRequestFinishedListener(callback);
+        mTestFramework.mCronetEngine.addRequestFinishedListener(requestFinishedListener);
+        ExperimentalUrlRequest.Builder urlRequestBuilder =
+                (ExperimentalUrlRequest.Builder) mTestFramework.mCronetEngine.newUrlRequestBuilder(
+                        mUrl, callback, callback.getExecutor());
+        Date startTime = new Date();
+        urlRequestBuilder.addRequestAnnotation("request annotation")
+                .addRequestAnnotation(this)
+                .build()
+                .start();
+        callback.blockForDone();
+        requestFinishedListener.blockUntilDone();
+        Date endTime = new Date();
+
+        RequestFinishedInfo requestInfo = requestFinishedListener.getRequestInfo();
+        MetricsTestUtil.checkRequestFinishedInfo(requestInfo, mUrl, startTime, endTime);
+        assertEquals(RequestFinishedInfo.SUCCEEDED, requestInfo.getFinishedReason());
+        MetricsTestUtil.checkHasConnectTiming(requestInfo.getMetrics(), startTime, endTime, false);
+        assertEquals(newHashSet("request annotation", this), // Use sets for unordered comparison.
+                new HashSet<Object>(requestInfo.getAnnotations()));
+        mTestFramework.mCronetEngine.shutdown();
+    }
+
+    @SmallTest
+    @OnlyRunNativeCronet
+    @Feature({"Cronet"})
+    // Tests a failed request where the error originates from Java.
+    public void testOrderFailedRequestJava() throws Exception {
+        mTestFramework = startCronetTestFramework();
+        final TestUrlRequestCallback callback = new TestUrlRequestCallback() {
+            @Override
+            public void onResponseStarted(UrlRequest request, UrlResponseInfo info) {
+                throw new RuntimeException("make this request fail");
+            }
+        };
+        TestRequestFinishedListener requestFinishedListener =
+                new AssertCallbackDoneRequestFinishedListener(callback);
+        mTestFramework.mCronetEngine.addRequestFinishedListener(requestFinishedListener);
+        UrlRequest.Builder urlRequestBuilder = mTestFramework.mCronetEngine.newUrlRequestBuilder(
+                mUrl, callback, callback.getExecutor());
+        urlRequestBuilder.build().start();
+        callback.blockForDone();
+        assertTrue(callback.mOnErrorCalled);
+        requestFinishedListener.blockUntilDone();
+        RequestFinishedInfo requestInfo = requestFinishedListener.getRequestInfo();
+        assertNotNull("RequestFinishedInfo.Listener must be called", requestInfo);
+        assertEquals(mUrl, requestInfo.getUrl());
+        assertTrue(requestInfo.getAnnotations().isEmpty());
+        assertEquals(RequestFinishedInfo.FAILED, requestInfo.getFinishedReason());
+        assertNotNull(requestInfo.getException());
+        assertEquals("Exception received from UrlRequest.Callback",
+                requestInfo.getException().getMessage());
+        RequestFinishedInfo.Metrics metrics = requestInfo.getMetrics();
+        assertNotNull("RequestFinishedInfo.getMetrics() must not be null", metrics);
+        mTestFramework.mCronetEngine.shutdown();
+    }
+
+    @SmallTest
+    @OnlyRunNativeCronet
+    @Feature({"Cronet"})
+    // Tests a failed request where the error originates from native code.
+    public void testOrderFailedRequestNative() throws Exception {
+        String connectionRefusedUrl = "http://127.0.0.1:3";
+        mTestFramework = startCronetTestFramework();
+        final TestUrlRequestCallback callback = new TestUrlRequestCallback();
+        TestRequestFinishedListener requestFinishedListener =
+                new AssertCallbackDoneRequestFinishedListener(callback);
+        mTestFramework.mCronetEngine.addRequestFinishedListener(requestFinishedListener);
+        UrlRequest.Builder urlRequestBuilder = mTestFramework.mCronetEngine.newUrlRequestBuilder(
+                connectionRefusedUrl, callback, callback.getExecutor());
+        urlRequestBuilder.build().start();
+        callback.blockForDone();
+        assertTrue(callback.mOnErrorCalled);
+        requestFinishedListener.blockUntilDone();
+        RequestFinishedInfo requestInfo = requestFinishedListener.getRequestInfo();
+        assertNotNull("RequestFinishedInfo.Listener must be called", requestInfo);
+        assertEquals(connectionRefusedUrl, requestInfo.getUrl());
+        assertTrue(requestInfo.getAnnotations().isEmpty());
+        assertEquals(RequestFinishedInfo.FAILED, requestInfo.getFinishedReason());
+        assertNotNull(requestInfo.getException());
+        assertEquals(NetworkException.ERROR_CONNECTION_REFUSED,
+                ((NetworkException) requestInfo.getException()).getErrorCode());
+        RequestFinishedInfo.Metrics metrics = requestInfo.getMetrics();
+        assertNotNull("RequestFinishedInfo.getMetrics() must not be null", metrics);
+        mTestFramework.mCronetEngine.shutdown();
+    }
+
+    @SmallTest
+    @OnlyRunNativeCronet
+    @Feature({"Cronet"})
+    public void testOrderCanceledRequest() throws Exception {
+        mTestFramework = startCronetTestFramework();
+        final TestUrlRequestCallback callback = new TestUrlRequestCallback() {
+            @Override
+            public void onResponseStarted(UrlRequest request, UrlResponseInfo info) {
+                super.onResponseStarted(request, info);
+                request.cancel();
+            }
+        };
+
+        TestRequestFinishedListener requestFinishedListener =
+                new AssertCallbackDoneRequestFinishedListener(callback);
+        mTestFramework.mCronetEngine.addRequestFinishedListener(requestFinishedListener);
+        ExperimentalUrlRequest.Builder urlRequestBuilder =
+                mTestFramework.mCronetEngine.newUrlRequestBuilder(
+                        mUrl, callback, callback.getExecutor());
+        Date startTime = new Date();
+        urlRequestBuilder.addRequestAnnotation("request annotation")
+                .addRequestAnnotation(this)
+                .build()
+                .start();
+        callback.blockForDone();
+        requestFinishedListener.blockUntilDone();
+        Date endTime = new Date();
+
+        RequestFinishedInfo requestInfo = requestFinishedListener.getRequestInfo();
+        MetricsTestUtil.checkRequestFinishedInfo(requestInfo, mUrl, startTime, endTime);
+        assertEquals(RequestFinishedInfo.CANCELED, requestInfo.getFinishedReason());
+        MetricsTestUtil.checkHasConnectTiming(requestInfo.getMetrics(), startTime, endTime, false);
+
+        assertEquals(newHashSet("request annotation", this), // Use sets for unordered comparison.
+                new HashSet<Object>(requestInfo.getAnnotations()));
+        mTestFramework.mCronetEngine.shutdown();
+    }
 }