Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2292113002:
Fix CronetUrlRequestTest#testFailures flake (Closed)
|
Created:
4 years, 3 months ago by xunjieli Modified:
4 years, 3 months ago Reviewers:
kapishnikov CC:
chromium-reviews, cbentzel+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCronetUrlRequestTest#testFailures
CronetUrlRequestTest#testFailures() simulates callback
failures in each response step. However, the response step
is not updated in onFailed, so maybeThrowCancelOrPause()
will throw an exception again. This results in a flake
in the Java implementation.
This CL updates response step in TestUrlRequestCallback,
and adds two more tests to make sure throwing exception
or canceling does nothing in terminal callbacks (onFailed,
onCanceled, onSucceeded)
R=kapishnikov@chromium.org
BUG=635026
Committed: https://crrev.com/bd8d8d9b383908aa19602df995cdae3bf09ba484
Cr-Commit-Position: refs/heads/master@{#415764}
Patch Set 1 #
Total comments: 2
Patch Set 2 : address comments #Patch Set 3 : fix affected tests #
Total comments: 12
Patch Set 4 : address comments #Patch Set 5 : missed one #
Total comments: 4
Patch Set 6 : address comment #
Messages
Total messages: 23 (10 generated)
https://codereview.chromium.org/2292113002/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java (left): https://codereview.chromium.org/2292113002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java:234: maybeThrowCancelOrPause(request); Why don't we update mResponseStep to ON_FAILED here? This would allow us to potentially test the listener exceptions and cancelations that can happen in onFailed() callback.
https://codereview.chromium.org/2292113002/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java (left): https://codereview.chromium.org/2292113002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java:234: maybeThrowCancelOrPause(request); On 2016/08/30 16:15:57, kapishnikov wrote: > Why don't we update mResponseStep to ON_FAILED here? This would allow us to > potentially test the listener exceptions and cancelations that can happen in > onFailed() callback. There is no enum ON_FAILED. We can certainly add one, but I don't think it's a good idea to call maybeThrowCancelOrPause() with openDone(). If we want to test failure or cancellation during onFailed, we need to do something with openDone(). Otherwise, it's a bit confusing that the request is "done", but we are still "canceling it or throwing an exception". WDYT?
On 2016/08/30 17:05:44, xunjieli wrote: > https://codereview.chromium.org/2292113002/diff/1/components/cronet/android/t... > File > components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java > (left): > > https://codereview.chromium.org/2292113002/diff/1/components/cronet/android/t... > components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java:234: > maybeThrowCancelOrPause(request); > On 2016/08/30 16:15:57, kapishnikov wrote: > > Why don't we update mResponseStep to ON_FAILED here? This would allow us to > > potentially test the listener exceptions and cancelations that can happen in > > onFailed() callback. > > There is no enum ON_FAILED. We can certainly add one, but I don't think it's a > good idea to call maybeThrowCancelOrPause() with openDone(). If we want to test > failure or cancellation during onFailed, we need to do something with > openDone(). Otherwise, it's a bit confusing that the request is "done", but we > are still "canceling it or throwing an exception". WDYT? We should also fix onSucceeded() & onCanceled() by removing the call to maybeThrowCancelOrPause() but better we should find a way of testing a scenario when an exception is thrown inside these terminal callbacks.
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Fix TestUrlRequestCallback.java to not throw exception in onFailed CronetUrlRequestTest#testFailures simulates a callback failure in each response step. However, the response step is not updated in onFailed, so maybeThrowCancelOrPause() will throw an exception again. This results in a flake in the Java implementation because we also unblock done before maybeThrowCancelOrPause() in onFailed. This CL get rid of the maybeThrowCancelOrPause() in TestUrlRequestCallback#onFailed(). R=kapishnikov@chromium.org BUG=635026 ========== to ========== CronetUrlRequestTest#testFailures CronetUrlRequestTest#testFailures() simulates callback failures in each response step. However, the response step is not updated in onFailed, so maybeThrowCancelOrPause() will throw an exception again. This results in a flake in the Java implementation. This CL updates response step in TestUrlRequestCallback, and adds two more tests to make sure throwing exception or canceling does nothing in terminal callbacks (onFailed, onCanceled, onSucceeded) R=kapishnikov@chromium.org BUG=635026 ==========
Done. Thanks for the review. PTAL?
https://codereview.chromium.org/2292113002/diff/60001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/2292113002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:65: callback.blockForDone(); 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. https://codereview.chromium.org/2292113002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1573: callback.blockForDone(); Same here. Is it possible that callback.blockForDone() can proceed before cancel() has been handled? https://codereview.chromium.org/2292113002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1635: callback.shutdownExecutor(); 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() https://codereview.chromium.org/2292113002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1663: callback.shutdownExecutor(); Same: awaitTermination() https://codereview.chromium.org/2292113002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1693: callback.shutdownExecutor(); Same: awaitTermination() https://codereview.chromium.org/2292113002/diff/60001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java (right): https://codereview.chromium.org/2292113002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java:281: private boolean maybeThrowCancelOrPause(final UrlRequest request) { Maybe we should add assertEquals(mExecutorThread, Thread.currentThread()) here just in case if somebody, in the future, calls this method from a method that does not do this validation. It is crucial for the method to be called on the executor's thread. It will also make the code analysis easier.
Patchset #4 (id:80001) has been deleted
Thanks! https://codereview.chromium.org/2292113002/diff/60001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/2292113002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:65: callback.blockForDone(); On 2016/08/31 18:10:27, kapishnikov wrote: > 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. Done. I think it makes sense to wait for a bit and make sure there's nothing bad happens afterwards. thanks for the suggestion. https://codereview.chromium.org/2292113002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1573: callback.blockForDone(); On 2016/08/31 18:10:27, kapishnikov wrote: > Same here. Is it possible that callback.blockForDone() can proceed before > cancel() has been handled? Done. https://codereview.chromium.org/2292113002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1635: callback.shutdownExecutor(); On 2016/08/31 18:10:27, kapishnikov wrote: > 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() Done. Good idea! https://codereview.chromium.org/2292113002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1663: callback.shutdownExecutor(); On 2016/08/31 18:10:27, kapishnikov wrote: > Same: awaitTermination() Done. https://codereview.chromium.org/2292113002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1693: callback.shutdownExecutor(); On 2016/08/31 18:10:27, kapishnikov wrote: > Same: awaitTermination() Done. https://codereview.chromium.org/2292113002/diff/60001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java (right): https://codereview.chromium.org/2292113002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java:281: private boolean maybeThrowCancelOrPause(final UrlRequest request) { On 2016/08/31 18:10:27, kapishnikov wrote: > Maybe we should add > assertEquals(mExecutorThread, Thread.currentThread()) > here just in case if somebody, in the future, calls this method from a method > that does not do this validation. It is crucial for the method to be called on > the executor's thread. It will also make the code analysis easier. Done.
Looks good. One comment below. LGTM https://codereview.chromium.org/2292113002/diff/120001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java (right): https://codereview.chromium.org/2292113002/diff/120001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java:148: assertTrue(false); It would be good to add a message to the assert. E.g., we can use it to search the logs.
https://codereview.chromium.org/2292113002/diff/120001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java (right): https://codereview.chromium.org/2292113002/diff/120001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java:146: mExecutorService.awaitTermination(5, TimeUnit.MINUTES); Nit: Also, can we make the waiting interval smaller, let's say 30-60 sec? I think if the executor does not finish in around 30 seconds then there is something wrong with the test or impl.
Thanks! https://codereview.chromium.org/2292113002/diff/120001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java (right): https://codereview.chromium.org/2292113002/diff/120001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java:146: mExecutorService.awaitTermination(5, TimeUnit.MINUTES); On 2016/08/31 20:35:15, kapishnikov wrote: > Nit: Also, can we make the waiting interval smaller, let's say 30-60 sec? I > think if the executor does not finish in around 30 seconds then there is > something wrong with the test or impl. Done. https://codereview.chromium.org/2292113002/diff/120001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java:148: assertTrue(false); On 2016/08/31 20:13:20, kapishnikov wrote: > It would be good to add a message to the assert. E.g., we can use it to search > the logs. Done.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by xunjieli@chromium.org
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kapishnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2292113002/#ps140001 (title: "address comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== CronetUrlRequestTest#testFailures CronetUrlRequestTest#testFailures() simulates callback failures in each response step. However, the response step is not updated in onFailed, so maybeThrowCancelOrPause() will throw an exception again. This results in a flake in the Java implementation. This CL updates response step in TestUrlRequestCallback, and adds two more tests to make sure throwing exception or canceling does nothing in terminal callbacks (onFailed, onCanceled, onSucceeded) R=kapishnikov@chromium.org BUG=635026 ========== to ========== CronetUrlRequestTest#testFailures CronetUrlRequestTest#testFailures() simulates callback failures in each response step. However, the response step is not updated in onFailed, so maybeThrowCancelOrPause() will throw an exception again. This results in a flake in the Java implementation. This CL updates response step in TestUrlRequestCallback, and adds two more tests to make sure throwing exception or canceling does nothing in terminal callbacks (onFailed, onCanceled, onSucceeded) R=kapishnikov@chromium.org BUG=635026 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== CronetUrlRequestTest#testFailures CronetUrlRequestTest#testFailures() simulates callback failures in each response step. However, the response step is not updated in onFailed, so maybeThrowCancelOrPause() will throw an exception again. This results in a flake in the Java implementation. This CL updates response step in TestUrlRequestCallback, and adds two more tests to make sure throwing exception or canceling does nothing in terminal callbacks (onFailed, onCanceled, onSucceeded) R=kapishnikov@chromium.org BUG=635026 ========== to ========== CronetUrlRequestTest#testFailures CronetUrlRequestTest#testFailures() simulates callback failures in each response step. However, the response step is not updated in onFailed, so maybeThrowCancelOrPause() will throw an exception again. This results in a flake in the Java implementation. This CL updates response step in TestUrlRequestCallback, and adds two more tests to make sure throwing exception or canceling does nothing in terminal callbacks (onFailed, onCanceled, onSucceeded) R=kapishnikov@chromium.org BUG=635026 Committed: https://crrev.com/bd8d8d9b383908aa19602df995cdae3bf09ba484 Cr-Commit-Position: refs/heads/master@{#415764} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/bd8d8d9b383908aa19602df995cdae3bf09ba484 Cr-Commit-Position: refs/heads/master@{#415764} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
