Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2085353006:
[Cronet] Fix UploadTest#testUploadChannelWithReadError (Closed)
Description[Cronet] Fix UploadTest#testUploadChannelWithReadError
Internal implementation has changed, the status code is no
longer 200, and the response body is no longer valid.
R=mmenke@chromium.org
BUG=622861
Committed: https://crrev.com/35831931e473803d0413ffb0c62f35fe35b60bc2
Cr-Commit-Position: refs/heads/master@{#401795}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Adjust expected response string #Messages
Total messages: 18 (7 generated)
Matt: PTAL.
Description was changed from ========== [Cronet] Fix UploadTest.testUploadChannelWithReadError Internal implementation has changed, the status code is no longer 200. R=mmenke@chromium.org BUG=622861 ========== to ========== [Cronet] Fix UploadTest#testUploadChannelWithReadError Internal implementation has changed, the status code is no longer 200. R=mmenke@chromium.org BUG=622861 ==========
https://codereview.chromium.org/2085353006/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/UploadTest.java (right): https://codereview.chromium.org/2085353006/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/UploadTest.java:214: assertEquals(UPLOAD_CHANNEL_DATA + "\0\0", listener.mResponseAsString); Is this second check still true?
https://codereview.chromium.org/2085353006/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/UploadTest.java (right): https://codereview.chromium.org/2085353006/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/UploadTest.java:214: assertEquals(UPLOAD_CHANNEL_DATA + "\0\0", listener.mResponseAsString); On 2016/06/23 21:47:07, mmenke wrote: > Is this second check still true? I am not sure. Still running test. Will update once the test result is out.
Good, it failed. You should instead be making sure the request fails with the expected error code, whatever that is.
Thanks!PTAL https://codereview.chromium.org/2085353006/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/UploadTest.java (right): https://codereview.chromium.org/2085353006/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/UploadTest.java:214: assertEquals(UPLOAD_CHANNEL_DATA + "\0\0", listener.mResponseAsString); On 2016/06/23 21:49:13, xunjieli wrote: > On 2016/06/23 21:47:07, mmenke wrote: > > Is this second check still true? > > I am not sure. Still running test. Will update once the test result is out. Done. It is null. We aren't able to assert on the net error code (which is ERR_FAILED in this case) because this is the legacy API (which doesn't expose net error code)
Description was changed from ========== [Cronet] Fix UploadTest#testUploadChannelWithReadError Internal implementation has changed, the status code is no longer 200. R=mmenke@chromium.org BUG=622861 ========== to ========== [Cronet] Fix UploadTest#testUploadChannelWithReadError Internal implementation has changed, the status code is no longer 200, and the response body is no longer valid. R=mmenke@chromium.org BUG=622861 ==========
On 2016/06/23 22:13:07, xunjieli wrote: > Thanks!PTAL > > https://codereview.chromium.org/2085353006/diff/1/components/cronet/android/t... > File > components/cronet/android/test/javatests/src/org/chromium/net/UploadTest.java > (right): > > https://codereview.chromium.org/2085353006/diff/1/components/cronet/android/t... > components/cronet/android/test/javatests/src/org/chromium/net/UploadTest.java:214: > assertEquals(UPLOAD_CHANNEL_DATA + "\0\0", listener.mResponseAsString); > On 2016/06/23 21:49:13, xunjieli wrote: > > On 2016/06/23 21:47:07, mmenke wrote: > > > Is this second check still true? > > > > I am not sure. Still running test. Will update once the test result is out. > > Done. It is null. We aren't able to assert on the net error code (which is > ERR_FAILED in this case) because this is the legacy API (which doesn't expose > net error code) LGTM
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2085353006/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2085353006/20001
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Fix UploadTest#testUploadChannelWithReadError Internal implementation has changed, the status code is no longer 200, and the response body is no longer valid. R=mmenke@chromium.org BUG=622861 ========== to ========== [Cronet] Fix UploadTest#testUploadChannelWithReadError Internal implementation has changed, the status code is no longer 200, and the response body is no longer valid. R=mmenke@chromium.org BUG=622861 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Fix UploadTest#testUploadChannelWithReadError Internal implementation has changed, the status code is no longer 200, and the response body is no longer valid. R=mmenke@chromium.org BUG=622861 ========== to ========== [Cronet] Fix UploadTest#testUploadChannelWithReadError Internal implementation has changed, the status code is no longer 200, and the response body is no longer valid. R=mmenke@chromium.org BUG=622861 Committed: https://crrev.com/35831931e473803d0413ffb0c62f35fe35b60bc2 Cr-Commit-Position: refs/heads/master@{#401795} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/35831931e473803d0413ffb0c62f35fe35b60bc2 Cr-Commit-Position: refs/heads/master@{#401795} |
