Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2572693003:
Fix CronetUrlRequestTest#testFailures flake (Closed)
|
Created:
4 years ago by xunjieli Modified:
4 years ago Reviewers:
pauljensen CC:
chromium-reviews, cbentzel+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix CronetUrlRequestTest#testFailures flake
When CronetUrlRequest is canceled asynchronously in a callback without
being paused, we might already have a onSucceeded() callback in the
executor's queue. If that is the case, we won't have a onCanceled()
callback. This led to test flake. This CL adjusts the test expectation
and adds a comment.
R=pauljensen@chromium.org
BUG=657415
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Committed: https://crrev.com/674e1123ddf931517378009e6cbd0705209a6edd
Cr-Commit-Position: refs/heads/master@{#438526}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Use a conditional and not skip all CANCEL_ASYNC_WITHOUT_PAUSE #Messages
Total messages: 19 (10 generated)
Description was changed from ========== Fix CronetUrlRequestTest#testFailures flake When CronetUrlRequest is canceled asynchronously in a callback without pausing the request, we might already have a onSucceeded() callback posted. If that is the case, we won't have a onCanceled() callback. This led to test flake. This CL adjusts the test expectation and adds a comment. R=pauljensen@chromium.org BUG=657415 ========== to ========== Fix CronetUrlRequestTest#testFailures flake When CronetUrlRequest is canceled asynchronously in a callback without pausing the request, we might already have a onSucceeded() callback posted. If that is the case, we won't have a onCanceled() callback. This led to test flake. This CL adjusts the test expectation and adds a comment. R=pauljensen@chromium.org BUG=657415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
Description was changed from ========== Fix CronetUrlRequestTest#testFailures flake When CronetUrlRequest is canceled asynchronously in a callback without pausing the request, we might already have a onSucceeded() callback posted. If that is the case, we won't have a onCanceled() callback. This led to test flake. This CL adjusts the test expectation and adds a comment. R=pauljensen@chromium.org BUG=657415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Fix CronetUrlRequestTest#testFailures flake When CronetUrlRequest is canceled asynchronously in a callback without being paused, we might already have a onSucceeded() callback in the executor's queue. If that is the case, we won't have a onCanceled() callback. This led to test flake. This CL adjusts the test expectation and adds a comment. R=pauljensen@chromium.org BUG=657415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
Description was changed from ========== Fix CronetUrlRequestTest#testFailures flake When CronetUrlRequest is canceled asynchronously in a callback without being paused, we might already have a onSucceeded() callback in the executor's queue. If that is the case, we won't have a onCanceled() callback. This led to test flake. This CL adjusts the test expectation and adds a comment. R=pauljensen@chromium.org BUG=657415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Fix CronetUrlRequestTest#testFailures flake When CronetUrlRequest is canceled asynchronously in a callback without being paused, we might already have a onSucceeded() callback in the executor's queue. If that is the case, we won't have a onCanceled() callback. This led to test flake. This CL adjusts the test expectation and adds a comment. R=pauljensen@chromium.org BUG=657415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
Paul, PTAL. Thanks!
https://codereview.chromium.org/2572693003/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/2572693003/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1687: // onCanceled() will not be invoked. See crbug.com/657415. I'm confused, we waited for the callback to get either onSucceeded, onFailed or onCanceled, as the call to callback.blockForDone() ensured. In the case of the flakes all of our asserts passed except for this one, which means onFailed was not called (as per the assertion on line 1681), and which means onSucceeded was not called (as per the assertion on line 1679) and onCanceled was not called (as per the assertion on line 1682). So how can onSucceeded have been called and not called?
lgtm
https://codereview.chromium.org/2572693003/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/2572693003/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1687: // onCanceled() will not be invoked. See crbug.com/657415. On 2016/12/14 14:40:31, pauljensen wrote: > I'm confused, we waited for the callback to get either onSucceeded, onFailed or > onCanceled, as the call to callback.blockForDone() ensured. In the case of the > flakes all of our asserts passed except for this one, which means onFailed was > not called (as per the assertion on line 1681), and which means onSucceeded was > not called (as per the assertion on line 1679) and onCanceled was not called (as > per the assertion on line 1682). So how can onSucceeded have been called and > not called? I was confused because mResponseInfo is set in onResponseStarted as well as onSucceeded.
Thanks! https://codereview.chromium.org/2572693003/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/2572693003/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1687: // onCanceled() will not be invoked. See crbug.com/657415. On 2016/12/14 15:17:32, pauljensen wrote: > On 2016/12/14 14:40:31, pauljensen wrote: > > I'm confused, we waited for the callback to get either onSucceeded, onFailed > or > > onCanceled, as the call to callback.blockForDone() ensured. In the case of > the > > flakes all of our asserts passed except for this one, which means onFailed was > > not called (as per the assertion on line 1681), and which means onSucceeded > was > > not called (as per the assertion on line 1679) and onCanceled was not called > (as > > per the assertion on line 1682). So how can onSucceeded have been called and > > not called? > > I was confused because mResponseInfo is set in onResponseStarted as well as > onSucceeded. After talking to you, I realize that we should only skip the case of canceling async without pause during onReadCompleted(). The rest two CANCEL_ASYNC_WITHOUT_PAUSEs should still have |mOnCanceledCalled| as true. Hope it's slightly clearer this way.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pauljensen@chromium.org Link to the patchset: https://codereview.chromium.org/2572693003/#ps20001 (title: "Use a conditional and not skip all CANCEL_ASYNC_WITHOUT_PAUSE")
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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481729788336500,
"parent_rev": "23757b916f9b56136efeac284d2f896821d86d39", "commit_rev":
"da1997d483a553845c93f3e2d6232546289c6cc4"}
Message was sent while issue was closed.
Description was changed from ========== Fix CronetUrlRequestTest#testFailures flake When CronetUrlRequest is canceled asynchronously in a callback without being paused, we might already have a onSucceeded() callback in the executor's queue. If that is the case, we won't have a onCanceled() callback. This led to test flake. This CL adjusts the test expectation and adds a comment. R=pauljensen@chromium.org BUG=657415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Fix CronetUrlRequestTest#testFailures flake When CronetUrlRequest is canceled asynchronously in a callback without being paused, we might already have a onSucceeded() callback in the executor's queue. If that is the case, we won't have a onCanceled() callback. This led to test flake. This CL adjusts the test expectation and adds a comment. R=pauljensen@chromium.org BUG=657415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2572693003 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix CronetUrlRequestTest#testFailures flake When CronetUrlRequest is canceled asynchronously in a callback without being paused, we might already have a onSucceeded() callback in the executor's queue. If that is the case, we won't have a onCanceled() callback. This led to test flake. This CL adjusts the test expectation and adds a comment. R=pauljensen@chromium.org BUG=657415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2572693003 ========== to ========== Fix CronetUrlRequestTest#testFailures flake When CronetUrlRequest is canceled asynchronously in a callback without being paused, we might already have a onSucceeded() callback in the executor's queue. If that is the case, we won't have a onCanceled() callback. This led to test flake. This CL adjusts the test expectation and adds a comment. R=pauljensen@chromium.org BUG=657415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Committed: https://crrev.com/674e1123ddf931517378009e6cbd0705209a6edd Cr-Commit-Position: refs/heads/master@{#438526} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/674e1123ddf931517378009e6cbd0705209a6edd Cr-Commit-Position: refs/heads/master@{#438526} |
