From ab552d10808324919b9c3f1489d39bde6bbd29b9 Mon Sep 17 00:00:00 2001 From: Stefano Duo Date: Fri, 10 Feb 2023 13:24:54 +0000 Subject: [PATCH] Always cancel requests during tearDown Assertions can terminate a test prior to that test's blockForDone call. In this case we will try to shutdown the engine with an outstanding request, in which case the method will throw, effectively failing the test. To avoid that, store a reference by the request created by the test and always cancel it (before calling engine.shutdown()). Also, instead of asserting the success of the requests, assume it. This is required because these tests are not hermetic and the request might fail due to reasons unrelated to the code being tested. Bug: 267618889 Test: m && atest CtsNetHttpTestCasess Change-Id: Ia1e30c99303f05342a2a73562e85d4ab2486c0d7 --- .../android/net/http/cts/HttpEngineTest.java | 27 ++++++++++++++----- .../http/cts/util/TestUrlRequestCallback.java | 16 +++++++++++ 2 files changed, 37 insertions(+), 6 deletions(-) diff --git a/Cronet/tests/cts/src/android/net/http/cts/HttpEngineTest.java b/Cronet/tests/cts/src/android/net/http/cts/HttpEngineTest.java index 6d27b43fad..8663a67d1a 100644 --- a/Cronet/tests/cts/src/android/net/http/cts/HttpEngineTest.java +++ b/Cronet/tests/cts/src/android/net/http/cts/HttpEngineTest.java @@ -47,6 +47,7 @@ public class HttpEngineTest { private HttpEngine.Builder mEngineBuilder; private TestUrlRequestCallback mCallback; + private UrlRequest mRequest; private HttpEngine mEngine; @Before @@ -59,6 +60,10 @@ public class HttpEngineTest { @After public void tearDown() throws Exception { + if (mRequest != null) { + mRequest.cancel(); + mCallback.blockForDone(); + } if (mEngine != null) { mEngine.shutdown(); } @@ -73,9 +78,12 @@ public class HttpEngineTest { mEngine = mEngineBuilder.build(); UrlRequest.Builder builder = mEngine.newUrlRequestBuilder(URL, mCallback, mCallback.getExecutor()); - builder.build().start(); + mRequest = builder.build(); + mRequest.start(); - mCallback.expectCallback(ResponseStep.ON_SUCCEEDED); + // This tests uses a non-hermetic server. Instead of asserting, assume the next callback. + // This way, if the request were to fail, the test would just be skipped instead of failing. + mCallback.assumeCallback(ResponseStep.ON_SUCCEEDED); UrlResponseInfo info = mCallback.mResponseInfo; assertOKStatusCode(info); assertEquals("h2", info.getNegotiatedProtocol()); @@ -86,9 +94,12 @@ public class HttpEngineTest { mEngine = mEngineBuilder.setEnableHttp2(false).build(); UrlRequest.Builder builder = mEngine.newUrlRequestBuilder(URL, mCallback, mCallback.getExecutor()); - builder.build().start(); + mRequest = builder.build(); + mRequest.start(); - mCallback.expectCallback(ResponseStep.ON_SUCCEEDED); + // This tests uses a non-hermetic server. Instead of asserting, assume the next callback. + // This way, if the request were to fail, the test would just be skipped instead of failing. + mCallback.assumeCallback(ResponseStep.ON_SUCCEEDED); UrlResponseInfo info = mCallback.mResponseInfo; assertOKStatusCode(info); assertEquals("http/1.1", info.getNegotiatedProtocol()); @@ -104,9 +115,13 @@ public class HttpEngineTest { mCallback = new TestUrlRequestCallback(); UrlRequest.Builder builder = mEngine.newUrlRequestBuilder(URL, mCallback, mCallback.getExecutor()); - builder.build().start(); + mRequest = builder.build(); + mRequest.start(); - mCallback.expectCallback(ResponseStep.ON_SUCCEEDED); + // This tests uses a non-hermetic server. Instead of asserting, assume the next + // callback. This way, if the request were to fail, the test would just be skipped + // instead of failing. + mCallback.assumeCallback(ResponseStep.ON_SUCCEEDED); UrlResponseInfo info = mCallback.mResponseInfo; assumeOKStatusCode(info); quicWasUsed = isQuic(info.getNegotiatedProtocol()); diff --git a/Cronet/tests/cts/src/android/net/http/cts/util/TestUrlRequestCallback.java b/Cronet/tests/cts/src/android/net/http/cts/util/TestUrlRequestCallback.java index 0b9e90fcba..fc4912c362 100644 --- a/Cronet/tests/cts/src/android/net/http/cts/util/TestUrlRequestCallback.java +++ b/Cronet/tests/cts/src/android/net/http/cts/util/TestUrlRequestCallback.java @@ -16,6 +16,7 @@ package android.net.http.cts.util; +import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; @@ -23,6 +24,8 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.junit.Assume.assumeThat; +import static org.junit.Assume.assumeTrue; import android.net.http.CallbackException; import android.net.http.HttpException; @@ -231,6 +234,19 @@ public class TestUrlRequestCallback extends UrlRequest.Callback { assertSame(expectedStep, mResponseStep); } + /** + * Waits for a terminal callback to complete execution before skipping the test if the + * callback is not the expected one + * + * @param expectedStep the expected callback step + */ + public void assumeCallback(ResponseStep expectedStep) { + if (isTerminalCallback(expectedStep)) { + assumeTrue("Did not receive terminal callback before timeout", blockForDone()); + } + assumeThat(expectedStep, equalTo(mResponseStep)); + } + /** * Blocks the calling thread until one of the final states has been called. * This is called before the callback has finished executed.