Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2401413002:
Fix testGetResponseAfterWriteFailed flake (Closed)
|
Created:
4 years, 2 months ago by xunjieli Modified:
4 years, 2 months ago Reviewers:
pauljensen CC:
chromium-reviews, cbentzel+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix testGetResponseAfterWriteFailed flake
When running the system HttpURLConnection implementation,
write() doesn't always fail. This CL forces system
implementation to flush the write by calling
OutputStream#flush(), so the system implementation must
know that the connection has died.
This is an attempt to fix the test flakiness observed when running system implementation.
BUG=653072
Committed: https://crrev.com/3651f8788be0a253a4e49e562a5b8e2c504b1fd2
Cr-Commit-Position: refs/heads/master@{#425805}
Patch Set 1 : self review #
Total comments: 2
Patch Set 2 : remove native test server change #
Total comments: 2
Patch Set 3 : Address comment to use out.flush() #
Total comments: 3
Patch Set 4 : remove testingSystemHttpURLConnection() #
Messages
Total messages: 19 (8 generated)
Description was changed from ========== Fix testGetResponseAfterWriteFailed flake This CL makes native test server wait on shutdown completion. This is an attempt to fix the test flakiness observed when running system implementation. BUG=653072 ========== to ========== Make NativeTestServer wait for shutdown completion This CL makes native test server wait on shutdown completion. This is an attempt to fix the test flakiness observed when running system implementation. BUG=653072 ==========
xunjieli@chromium.org changed reviewers: + pauljensen@chromium.org
Patchset #1 (id:1) has been deleted
Paul: PTAL. Thanks!
https://codereview.chromium.org/2401413002/diff/20001/components/cronet/andro... File components/cronet/android/test/native_test_server.cc (right): https://codereview.chromium.org/2401413002/diff/20001/components/cronet/andro... components/cronet/android/test/native_test_server.cc:238: delete g_test_server; Doesn't this line already do this? https://cs.chromium.org/chromium/src/net/test/embedded_test_server/embedded_t...
Description was changed from ========== Make NativeTestServer wait for shutdown completion This CL makes native test server wait on shutdown completion. This is an attempt to fix the test flakiness observed when running system implementation. BUG=653072 ========== to ========== Fix testGetResponseAfterWriteFailed flake When running the system HttpURLConnection implementation, write() doesn't always fail. This CL forces system implementation to flush the write by calling getResponseCode(), so the system implementation must know that the connection has died. This is an attempt to fix the test flakiness observed when running system implementation. BUG=653072 ==========
Mind taking a look at PS#2? https://codereview.chromium.org/2401413002/diff/20001/components/cronet/andro... File components/cronet/android/test/native_test_server.cc (right): https://codereview.chromium.org/2401413002/diff/20001/components/cronet/andro... components/cronet/android/test/native_test_server.cc:238: delete g_test_server; On 2016/10/10 12:56:21, pauljensen wrote: > Doesn't this line already do this? > https://cs.chromium.org/chromium/src/net/test/embedded_test_server/embedded_t... ah, that's a good point. I forgot about it :(
https://codereview.chromium.org/2401413002/diff/40001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java (right): https://codereview.chromium.org/2401413002/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:114: connection.getResponseCode(); This seems a bit circuitous, why not just out.flush()?
Thanks! PTAL https://codereview.chromium.org/2401413002/diff/40001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java (right): https://codereview.chromium.org/2401413002/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:114: connection.getResponseCode(); On 2016/10/10 13:19:22, pauljensen wrote: > This seems a bit circuitous, why not just out.flush()? Done.
Can you update the commit description to match the code https://codereview.chromium.org/2401413002/diff/60001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetChunkedOutputStreamTest.java (right): https://codereview.chromium.org/2401413002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetChunkedOutputStreamTest.java:131: // Forces system implementation to flush. can you add a crbug.com/ link to the bug? https://codereview.chromium.org/2401413002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetChunkedOutputStreamTest.java:132: if (testingSystemHttpURLConnection()) { Can we remove this if-statement? I imagine the flush() shouldn't affect our impl. I prefer to test that our impl and the system impl match. Ditto for the other file.
Description was changed from ========== Fix testGetResponseAfterWriteFailed flake When running the system HttpURLConnection implementation, write() doesn't always fail. This CL forces system implementation to flush the write by calling getResponseCode(), so the system implementation must know that the connection has died. This is an attempt to fix the test flakiness observed when running system implementation. BUG=653072 ========== to ========== Fix testGetResponseAfterWriteFailed flake When running the system HttpURLConnection implementation, write() doesn't always fail. This CL forces system implementation to flush the write by calling OutputStream#flush(), so the system implementation must know that the connection has died. This is an attempt to fix the test flakiness observed when running system implementation. BUG=653072 ==========
https://codereview.chromium.org/2401413002/diff/60001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetChunkedOutputStreamTest.java (right): https://codereview.chromium.org/2401413002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetChunkedOutputStreamTest.java:131: // Forces system implementation to flush. On 2016/10/17 15:24:16, pauljensen wrote: > can you add a crbug.com/ link to the bug? Done.
lgtm
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...
Message was sent while issue was closed.
Description was changed from ========== Fix testGetResponseAfterWriteFailed flake When running the system HttpURLConnection implementation, write() doesn't always fail. This CL forces system implementation to flush the write by calling OutputStream#flush(), so the system implementation must know that the connection has died. This is an attempt to fix the test flakiness observed when running system implementation. BUG=653072 ========== to ========== Fix testGetResponseAfterWriteFailed flake When running the system HttpURLConnection implementation, write() doesn't always fail. This CL forces system implementation to flush the write by calling OutputStream#flush(), so the system implementation must know that the connection has died. This is an attempt to fix the test flakiness observed when running system implementation. BUG=653072 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix testGetResponseAfterWriteFailed flake When running the system HttpURLConnection implementation, write() doesn't always fail. This CL forces system implementation to flush the write by calling OutputStream#flush(), so the system implementation must know that the connection has died. This is an attempt to fix the test flakiness observed when running system implementation. BUG=653072 ========== to ========== Fix testGetResponseAfterWriteFailed flake When running the system HttpURLConnection implementation, write() doesn't always fail. This CL forces system implementation to flush the write by calling OutputStream#flush(), so the system implementation must know that the connection has died. This is an attempt to fix the test flakiness observed when running system implementation. BUG=653072 Committed: https://crrev.com/3651f8788be0a253a4e49e562a5b8e2c504b1fd2 Cr-Commit-Position: refs/heads/master@{#425805} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3651f8788be0a253a4e49e562a5b8e2c504b1fd2 Cr-Commit-Position: refs/heads/master@{#425805} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
