Fix CronetUrlRequestTest#testFailures flake

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

Index: components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
diff --git a/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java b/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
index 9aaf35f3e2d1dd86c71b89e9f6371a3a187db62b..59940bef61febb95f30c569dcbec82e96bd19ade 100644
--- a/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
+++ b/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
@@ -627,7 +627,7 @@ public class CronetUrlRequestTest extends CronetTestBase {
         assertEquals(arbitraryNetError, callback.mError.getCronetInternalErrorCode());
         assertEquals(0, callback.mRedirectCount);
         assertTrue(callback.mOnErrorCalled);
-        assertEquals(callback.mResponseStep, ResponseStep.NOTHING);
+        assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep);
     }
 
     @SmallTest
@@ -644,7 +644,7 @@ public class CronetUrlRequestTest extends CronetTestBase {
         assertEquals(arbitraryNetError, callback.mError.getCronetInternalErrorCode());
         assertEquals(0, callback.mRedirectCount);
         assertTrue(callback.mOnErrorCalled);
-        assertEquals(callback.mResponseStep, ResponseStep.ON_RESPONSE_STARTED);
+        assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep);
     }
 
     @SmallTest
@@ -661,7 +661,7 @@ public class CronetUrlRequestTest extends CronetTestBase {
         assertEquals(arbitraryNetError, callback.mError.getCronetInternalErrorCode());
         assertEquals(0, callback.mRedirectCount);
         assertTrue(callback.mOnErrorCalled);
-        assertEquals(callback.mResponseStep, ResponseStep.ON_RESPONSE_STARTED);
+        assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep);
     }
 
     /**
@@ -696,7 +696,7 @@ public class CronetUrlRequestTest extends CronetTestBase {
         assertEquals(-201, callback.mError.getCronetInternalErrorCode());
         assertEquals("Exception in CronetUrlRequest: net::ERR_CERT_DATE_INVALID",
                 callback.mError.getMessage());
-        assertEquals(callback.mResponseStep, ResponseStep.NOTHING);
+        assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep);
     }
 
     /**
@@ -1572,7 +1572,11 @@ public class CronetUrlRequestTest extends CronetTestBase {
         urlRequest.start();
         callback.blockForDone();

[kapishnikov, 2016/08/31 18:10:27]

Same here. Is it possible that callback.blockForDone() can proceed before
cancel() has been handled?

[xunjieli, 2016/08/31 20:03:07]

Done.

         assertEquals(1, callback.mRedirectCount);
-        assertEquals(callback.mResponseStep, failureStep);
+        if (failureType == FailureType.CANCEL_SYNC || failureType == FailureType.CANCEL_ASYNC) {
+            assertEquals(ResponseStep.ON_CANCELED, callback.mResponseStep);
+        } else if (failureType == FailureType.THROW_SYNC) {
+            assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep);
+        }
         assertTrue(urlRequest.isDone());
         assertEquals(expectResponseInfo, callback.mResponseInfo != null);
         assertEquals(expectError, callback.mError != null);
@@ -1616,20 +1620,83 @@ public class CronetUrlRequestTest extends CronetTestBase {
 
     @SmallTest
     @Feature({"Cronet"})
-    public void testThrowON_SUCCEEDED() {
-        TestUrlRequestCallback callback = new TestUrlRequestCallback();
-        callback.setFailure(FailureType.THROW_SYNC, ResponseStep.ON_SUCCEEDED);
-        UrlRequest.Builder builder = new UrlRequest.Builder(NativeTestServer.getRedirectURL(),
-                callback, callback.getExecutor(), mTestFramework.mCronetEngine);
-        UrlRequest urlRequest = builder.build();
-        urlRequest.start();
-        callback.blockForDone();
-        assertEquals(1, callback.mRedirectCount);
-        assertEquals(callback.mResponseStep, ResponseStep.ON_SUCCEEDED);
-        assertTrue(urlRequest.isDone());
-        assertNotNull(callback.mResponseInfo);
-        assertNull(callback.mError);
-        assertFalse(callback.mOnErrorCalled);
+    public void testThrowOrCancelInOnSucceeded() {
+        FailureType[] testTypes = new FailureType[] {
+                FailureType.THROW_SYNC, FailureType.CANCEL_SYNC, FailureType.CANCEL_ASYNC};
+        for (FailureType type : testTypes) {
+            TestUrlRequestCallback callback = new TestUrlRequestCallback();
+            callback.setFailure(type, ResponseStep.ON_SUCCEEDED);
+            UrlRequest.Builder builder = new UrlRequest.Builder(NativeTestServer.getEchoMethodURL(),
+                    callback, callback.getExecutor(), mTestFramework.mCronetEngine);
+            UrlRequest urlRequest = builder.build();
+            urlRequest.start();
+            callback.blockForDone();
+            // Wait for all posted tasks to be executed to ensure there is no exception thrown.
+            callback.shutdownExecutor();

[kapishnikov, 2016/08/31 18:10:27]

Should we call awaitTermination() after shutdownExecutor()? Method
shutdownExecutor() does not wait for the submitted tasks to finish. Maybe we can
add a new method to the test callback: shutdownExecutorAndWait()

[xunjieli, 2016/08/31 20:03:07]

Done. Good idea!

+            assertNull(callback.mError);
+            assertEquals(ResponseStep.ON_SUCCEEDED, callback.mResponseStep);
+            assertTrue(urlRequest.isDone());
+            assertNotNull(callback.mResponseInfo);
+            assertFalse(callback.mOnErrorCalled);
+            assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
+            assertEquals("GET", callback.mResponseAsString);
+        }
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testThrowOrCancelInOnFailed() {
+        FailureType[] testTypes = new FailureType[] {
+                FailureType.THROW_SYNC, FailureType.CANCEL_SYNC, FailureType.CANCEL_ASYNC};
+        for (FailureType type : testTypes) {
+            String url = NativeTestServer.getEchoBodyURL();
+            // Shut down NativeTestServer so request will fail.
+            NativeTestServer.shutdownNativeTestServer();
+            TestUrlRequestCallback callback = new TestUrlRequestCallback();
+            callback.setFailure(type, ResponseStep.ON_FAILED);
+            UrlRequest.Builder builder = new UrlRequest.Builder(
+                    url, callback, callback.getExecutor(), mTestFramework.mCronetEngine);
+            UrlRequest urlRequest = builder.build();
+            urlRequest.start();
+            callback.blockForDone();
+            // Wait for all posted tasks to be executed to ensure there is no exception thrown.
+            callback.shutdownExecutor();

[kapishnikov, 2016/08/31 18:10:27]

Same: awaitTermination()

[xunjieli, 2016/08/31 20:03:07]

Done.

+            assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep);
+            assertTrue(callback.mOnErrorCalled);
+            assertNotNull(callback.mError);
+            assertTrue(urlRequest.isDone());
+            // Start NativeTestServer again to run the test for a second time.
+            assertTrue(NativeTestServer.startNativeTestServer(getContext()));
+        }
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testThrowOrCancelInOnCanceled() {
+        FailureType[] testTypes = new FailureType[] {
+                FailureType.THROW_SYNC, FailureType.CANCEL_SYNC, FailureType.CANCEL_ASYNC};
+        for (FailureType type : testTypes) {
+            TestUrlRequestCallback callback = new TestUrlRequestCallback() {
+                @Override
+                public void onResponseStarted(UrlRequest request, UrlResponseInfo info) {
+                    super.onResponseStarted(request, info);
+                    request.cancel();
+                }
+            };
+            callback.setFailure(type, ResponseStep.ON_CANCELED);
+            UrlRequest.Builder builder = new UrlRequest.Builder(NativeTestServer.getEchoBodyURL(),
+                    callback, callback.getExecutor(), mTestFramework.mCronetEngine);
+            UrlRequest urlRequest = builder.build();
+            urlRequest.start();
+            callback.blockForDone();
+            // Wait for all posted tasks to be executed to ensure there is no exception thrown.
+            callback.shutdownExecutor();

[kapishnikov, 2016/08/31 18:10:27]

Same: awaitTermination()

[xunjieli, 2016/08/31 20:03:07]

Done.

+            assertEquals(ResponseStep.ON_CANCELED, callback.mResponseStep);
+            assertTrue(urlRequest.isDone());
+            assertNotNull(callback.mResponseInfo);
+            assertNull(callback.mError);
+            assertTrue(callback.mOnCanceledCalled);
+        }
     }
 
     @SmallTest
@@ -1837,7 +1904,7 @@ public class CronetUrlRequestTest extends CronetTestBase {
                 "Exception in CronetUrlRequest: net::ERR_" + name, callback.mError.getMessage());
         assertEquals(0, callback.mRedirectCount);
         assertTrue(callback.mOnErrorCalled);
-        assertEquals(callback.mResponseStep, ResponseStep.NOTHING);
+        assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep);
     }
 
     // Returns the contents of byteBuffer, from its position() to its limit(),