New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 826570 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Apr 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

JobControllerReconsiderProxyAfterErrorTest.ReconsiderProxyAfterError doesn't work as expected

Project Member Reported by eroman@chromium.org, Mar 28 2018

Issue description

The test JobControllerReconsiderProxyAfterErrorTest.ReconsiderProxyAfterError is parameterized on various proxy errors:

https://chromium.googlesource.com/chromium/src/+/0895a6d43d26a7dc2c156f983378211d7ec4f8eb/net/http/http_stream_factory_impl_job_controller_unittest.cc#67

However in practice this isn't really extending test coverage.

The parameterized error is being used to fail the mock socket connect. The consequence is that HttpProxyClientSocketWrapper re-writes the error to ERR_PROXY_CONNECTION_FAILED. So the proxy fallback logic is being exercised with ERR_PROXY_CONNECTION_FAILED each time.
 
Cc: -xunji...@chromium.org
Owner: xunji...@chromium.org
Status: Assigned (was: Untriaged)
Good find. Thanks, Eric. Let me upload a patch to fix this.
Status: Started (was: Assigned)
Sorry for the delay. I started working on this. I'll add some test utils (a mock proxy client socket that wraps mock transport client socket) to support returning non-overriden mock errors from the proxy client socket.
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ac3c51e4abe6610262c78e4afc09a7f5cdb98779

commit ac3c51e4abe6610262c78e4afc09a7f5cdb98779
Author: Helen Li <xunjieli@chromium.org>
Date: Tue Apr 24 00:02:13 2018

Adds a net::MockProxyClientSocket so tests can mock out ProxyClientSocket

- adds a net::MockProxyClientSocket allows consumer to specify connect errors
  through MockConnect.
- adds a CreateProxyClientSocket to net::ClientSocketFactory, so test
  factory impl can mock out that function.
- Use MockProxyClientSocket in ReconsiderProxyAfterError so we test error
  fallback correctly. Please see linked bug for more info.

BUG:  826570 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I0c7e858c0bce128fa92293e3a47de530025e4c45
Reviewed-on: https://chromium-review.googlesource.com/1011119
Reviewed-by: Eric Roman <eroman@chromium.org>
Commit-Queue: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552912}
[modify] https://crrev.com/ac3c51e4abe6610262c78e4afc09a7f5cdb98779/net/dns/address_sorter_posix_unittest.cc
[modify] https://crrev.com/ac3c51e4abe6610262c78e4afc09a7f5cdb98779/net/dns/dns_session_unittest.cc
[modify] https://crrev.com/ac3c51e4abe6610262c78e4afc09a7f5cdb98779/net/http/http_proxy_client_socket_wrapper.cc
[modify] https://crrev.com/ac3c51e4abe6610262c78e4afc09a7f5cdb98779/net/http/http_stream_factory_impl_job_controller_unittest.cc
[modify] https://crrev.com/ac3c51e4abe6610262c78e4afc09a7f5cdb98779/net/socket/client_socket_factory.cc
[modify] https://crrev.com/ac3c51e4abe6610262c78e4afc09a7f5cdb98779/net/socket/client_socket_factory.h
[modify] https://crrev.com/ac3c51e4abe6610262c78e4afc09a7f5cdb98779/net/socket/client_socket_pool_base_unittest.cc
[modify] https://crrev.com/ac3c51e4abe6610262c78e4afc09a7f5cdb98779/net/socket/fuzzed_socket_factory.cc
[modify] https://crrev.com/ac3c51e4abe6610262c78e4afc09a7f5cdb98779/net/socket/fuzzed_socket_factory.h
[modify] https://crrev.com/ac3c51e4abe6610262c78e4afc09a7f5cdb98779/net/socket/socket_test_util.cc
[modify] https://crrev.com/ac3c51e4abe6610262c78e4afc09a7f5cdb98779/net/socket/socket_test_util.h
[modify] https://crrev.com/ac3c51e4abe6610262c78e4afc09a7f5cdb98779/net/socket/transport_client_socket_pool.cc
[modify] https://crrev.com/ac3c51e4abe6610262c78e4afc09a7f5cdb98779/net/socket/transport_client_socket_pool.h
[modify] https://crrev.com/ac3c51e4abe6610262c78e4afc09a7f5cdb98779/net/socket/transport_client_socket_pool_test_util.cc
[modify] https://crrev.com/ac3c51e4abe6610262c78e4afc09a7f5cdb98779/net/socket/transport_client_socket_pool_test_util.h

Project Member

Comment 4 by bugdroid1@chromium.org, Apr 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bef1774162a6f11a4fe5e35a2fa34ba31e10dc6e

commit bef1774162a6f11a4fe5e35a2fa34ba31e10dc6e
Author: Helen Li <xunjieli@chromium.org>
Date: Tue Apr 24 19:11:46 2018

Add tests for proxy fallback logic on ERR_MSG_TOO_BIG

The fallback logic is different for quic and non-quic proxies on
ERR_MSG_TOO_BIG. This CL adds test coverage for this error code.

Bug:  826570 
Change-Id: Ied1470a878ddf3c6fabee1fe7170894a4de02f40
Reviewed-on: https://chromium-review.googlesource.com/1026050
Commit-Queue: Helen Li <xunjieli@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553223}
[modify] https://crrev.com/bef1774162a6f11a4fe5e35a2fa34ba31e10dc6e/net/http/http_stream_factory_job_controller_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2f856bb2c1e6b19c0a529a9eadf9481d4959cbe1

commit 2f856bb2c1e6b19c0a529a9eadf9481d4959cbe1
Author: Helen Li <xunjieli@chromium.org>
Date: Tue Apr 24 23:07:36 2018

Fix ProxyResolvingClientSocket unit test to correctly test fallback behavior

This CL uses mock proxy sockets so that we can correctly test the proxy fallback
behavior. For more info, see linked bug.

Bug:  826570 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Ie13e36d4b16b32d1e739fb0a7e513d4c6570c3a3
Reviewed-on: https://chromium-review.googlesource.com/1026421
Reviewed-by: Eric Roman <eroman@chromium.org>
Commit-Queue: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553340}
[modify] https://crrev.com/2f856bb2c1e6b19c0a529a9eadf9481d4959cbe1/services/network/proxy_resolving_client_socket_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment