Fix CronetUrlRequestTest#testFailures flake

components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.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.os.ConditionVariable;
 import android.os.StrictMode;
 import android.test.MoreAsserts;
 import android.test.suitebuilder.annotation.SmallTest;
@@ skipping 44 matching lines @@
         super.tearDown();
     }
 
     private TestUrlRequestCallback startAndWaitForComplete(String url) throws Exception {
         TestUrlRequestCallback callback = new TestUrlRequestCallback();
         // Create request.
         UrlRequest.Builder builder = new UrlRequest.Builder(
                 url, callback, callback.getExecutor(), mTestFramework.mCronetEngine);
         UrlRequest urlRequest = builder.build();
         urlRequest.start();
         callback.blockForDone();

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

Should we also wait for the executor to finish all tasks here?  openDone() is
called before maybeThrowCancelOrPause() in onSucceeded(), onFailed() and
onCanceled() methods. I am not sure if it is important to give
maybeThrowCancelOrPause() method chance to complete before finishing the test.

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

Done. I think it makes sense to wait for a bit and make sure there's nothing bad
happens afterwards. thanks for the suggestion.

         assertTrue(urlRequest.isDone());
         return callback;
     }
 
     private void checkResponseInfo(UrlResponseInfo responseInfo, String expectedUrl,
             int expectedHttpStatusCode, String expectedHttpStatusText) {
         assertEquals(expectedUrl, responseInfo.getUrl());
         assertEquals(
                 expectedUrl, responseInfo.getUrlChain().get(responseInfo.getUrlChain().size() - 1));
         assertEquals(expectedHttpStatusCode, responseInfo.getHttpStatusCode());
@@ skipping 544 matching lines @@
     public void testMockStartAsyncError() throws Exception {
         final int arbitraryNetError = -3;
         TestUrlRequestCallback callback =
                 startAndWaitForComplete(MockUrlRequestJobFactory.getMockUrlWithFailure(
                         FailurePhase.START, arbitraryNetError));
         assertNull(callback.mResponseInfo);
         assertNotNull(callback.mError);
         assertEquals(arbitraryNetError, callback.mError.getCronetInternalErrorCode());
         assertEquals(0, callback.mRedirectCount);
         assertTrue(callback.mOnErrorCalled);
-        assertEquals(callback.mResponseStep, ResponseStep.NOTHING);
+        assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep);
     }
 
     @SmallTest
     @Feature({"Cronet"})
     @OnlyRunNativeCronet // Java impl doesn't support MockUrlRequestJobFactory
     public void testMockReadDataSyncError() throws Exception {
         final int arbitraryNetError = -4;
         TestUrlRequestCallback callback =
                 startAndWaitForComplete(MockUrlRequestJobFactory.getMockUrlWithFailure(
                         FailurePhase.READ_SYNC, arbitraryNetError));
         assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
         assertEquals(0, callback.mResponseInfo.getReceivedBytesCount());
         assertNotNull(callback.mError);
         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
     @Feature({"Cronet"})
     @OnlyRunNativeCronet // Java impl doesn't support MockUrlRequestJobFactory
     public void testMockReadDataAsyncError() throws Exception {
         final int arbitraryNetError = -5;
         TestUrlRequestCallback callback =
                 startAndWaitForComplete(MockUrlRequestJobFactory.getMockUrlWithFailure(
                         FailurePhase.READ_ASYNC, arbitraryNetError));
         assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
         assertEquals(0, callback.mResponseInfo.getReceivedBytesCount());
         assertNotNull(callback.mError);
         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);
     }
 
     /**
      * Tests that request continues when client certificate is requested.
      */
     @SmallTest
     @Feature({"Cronet"})
     @OnlyRunNativeCronet
     public void testMockClientCertificateRequested() throws Exception {
         TestUrlRequestCallback callback = startAndWaitForComplete(
@@ skipping 14 matching lines @@
     @OnlyRunNativeCronet // Java impl doesn't support MockUrlRequestJobFactory
     public void testMockSSLCertificateError() throws Exception {
         TestUrlRequestCallback callback = startAndWaitForComplete(
                 MockUrlRequestJobFactory.getMockUrlForSSLCertificateError());
         assertNull(callback.mResponseInfo);
         assertNotNull(callback.mError);
         assertTrue(callback.mOnErrorCalled);
         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);
     }
 
     /**
      * Checks that the buffer is updated correctly, when starting at an offset.
      */
     @SmallTest
     @Feature({"Cronet"})
     public void testSimpleGetBufferUpdates() throws Exception {
         TestUrlRequestCallback callback = new TestUrlRequestCallback();
         callback.setAutoAdvance(false);
@@ skipping 853 matching lines @@
             boolean expectResponseInfo, boolean expectError) {
         if (Log.isLoggable("TESTING", Log.VERBOSE)) {
             Log.v("TESTING", "Testing " + failureType + " during " + failureStep);
         }
         TestUrlRequestCallback callback = new TestUrlRequestCallback();
         callback.setFailure(failureType, failureStep);
         UrlRequest.Builder builder = new UrlRequest.Builder(NativeTestServer.getRedirectURL(),
                 callback, callback.getExecutor(), mTestFramework.mCronetEngine);
         UrlRequest urlRequest = builder.build();
         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);
         assertEquals(expectError, callback.mOnErrorCalled);
         assertEquals(failureType == FailureType.CANCEL_SYNC
                         || failureType == FailureType.CANCEL_ASYNC
                         || failureType == FailureType.CANCEL_ASYNC_WITHOUT_PAUSE,
                 callback.mOnCanceledCalled);
     }
 
@@ skipping 23 matching lines @@
         throwOrCancel(FailureType.CANCEL_ASYNC, ResponseStep.ON_READ_COMPLETED,
                 true, false);
         throwOrCancel(FailureType.CANCEL_ASYNC_WITHOUT_PAUSE, ResponseStep.ON_READ_COMPLETED,
                 true, false);
         throwOrCancel(FailureType.THROW_SYNC, ResponseStep.ON_READ_COMPLETED,
                 true, true);
     }
 
     @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
     @Feature({"Cronet"})
     @OnlyRunNativeCronet // No destroyed callback for tests
     public void testExecutorShutdown() {
         TestUrlRequestCallback callback = new TestUrlRequestCallback();
 
         callback.setAutoAdvance(false);
         UrlRequest.Builder builder = new UrlRequest.Builder(NativeTestServer.getEchoBodyURL(),
                 callback, callback.getExecutor(), mTestFramework.mCronetEngine);
         CronetUrlRequest urlRequest = (CronetUrlRequest) builder.build();
         urlRequest.start();
@@ skipping 184 matching lines @@
         TestUrlRequestCallback callback = startAndWaitForComplete(
                 MockUrlRequestJobFactory.getMockUrlWithFailure(FailurePhase.START, netError));
         assertNull(callback.mResponseInfo);
         assertNotNull(callback.mError);
         assertEquals(netError, callback.mError.getCronetInternalErrorCode());
         assertEquals(errorCode, callback.mError.getErrorCode());
         assertEquals(
                 "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(),
     // as a String. Does not modify byteBuffer's position().
     private String bufferContentsToString(ByteBuffer byteBuffer, int start, int end) {
         // Use a duplicate to avoid modifying byteBuffer.
         ByteBuffer duplicate = byteBuffer.duplicate();
         duplicate.position(start);
         duplicate.limit(end);
         byte[] contents = new byte[duplicate.remaining()];
         duplicate.get(contents);
         return new String(contents);
     }
 }