chromeos: Simplify D-Bus proxy provider (but not tests).

chromeos: Simplify D-Bus proxy provider (but not tests).

Move some complexity out of
ProxyResolutionServiceProvider::Delegate and add a lot of
complexity to its unit tests by constructing an alternate
net::ProxyService.

BUG=446115, 703217, 706665
Review-Url: https://codereview.chromium.org/2787253002
Cr-Commit-Position: refs/heads/master@{#461258}
Committed: https://chromium.googlesource.com/chromium/src/+/4a50453a68e5577dd9a54510c3810d112b35a763

[Daniel Erat, 2017-03-31 02:39:43]

derat@chromium.org changed reviewers:
+ eroman@chromium.org, jamescook@chromium.org

[Daniel Erat, 2017-03-31 02:39:44]

james, i'm curious about what you'll think about this. :-P

eric, would you mind taking a quick look at the test file to see if i missed
some easier way to accomplish what i'm trying to do?

as background, i've been cleaning up some old chrome os code that gets proxy
information from net::ProxyService::ResolveProxy() and returns it to other
system services. the test coverage was poor, and i'm trying to improve that.

i'd previously added a wrapper around the ResolveProxy() call, but i'd rather
not complicate the non-test code just for the sake of testing. i'm experimenting
here with creating my own ProxyService that uses a resolver that i can configure
to return sync or async results, but the code feels like it's extremely
dependent on implementation details of the proxy code.

ideally, i think that i'd be able to derive from ProxyService in my test code
and just write my own ResolveProxy() implementation that does whatever i want,
but that doesn't appear to be possible at present.

https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/prox...
File chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc
(right):

https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/prox...
chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc:308: //
about how to get a config with that setting into net::ProxyService().
i'm not sure how to fix this, or whether i should just delete the test. :-/

[Daniel Erat, 2017-03-31 02:40:46]

Description was changed from

==========
chromeos: Simplify D-Bus proxy provider (but not tests).

Move some complexity out of
ProxyResolutionServiceProvider::Delegate and add a lot of
complexity to its unit tests by constructing an alternate
net::ProxyService.

BUG=446115,703217
==========

to

==========
chromeos: Simplify D-Bus proxy provider (but not tests).

Move some complexity out of
ProxyResolutionServiceProvider::Delegate and add a lot of
complexity to its unit tests by constructing an alternate
net::ProxyService.

BUG=446115,703217,706665
==========

[Daniel Erat, 2017-03-31 03:22:51]

On 2017/03/31 02:39:44, Daniel Erat wrote:
> james, i'm curious about what you'll think about this. :-P
> 
> eric, would you mind taking a quick look at the test file to see if i missed
> some easier way to accomplish what i'm trying to do?
> 
> as background, i've been cleaning up some old chrome os code that gets proxy
> information from net::ProxyService::ResolveProxy() and returns it to other
> system services. the test coverage was poor, and i'm trying to improve that.
> 
> i'd previously added a wrapper around the ResolveProxy() call, but i'd rather
> not complicate the non-test code just for the sake of testing. i'm
experimenting
> here with creating my own ProxyService that uses a resolver that i can
configure
> to return sync or async results, but the code feels like it's extremely
> dependent on implementation details of the proxy code.
> 
> ideally, i think that i'd be able to derive from ProxyService in my test code
> and just write my own ResolveProxy() implementation that does whatever i want,
> but that doesn't appear to be possible at present.

so to summarize this question, i think: what's the best way to make
net::ProxyService::ResolveProxy() return arbitrary results and codes either
synchronously or asynchronously within a unit test?

[James Cook, 2017-03-31 17:33:10]

LGTM with one question. I like pushing the complexity onto the test code.

https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/prox...
File chromeos/dbus/services/proxy_resolution_service_provider.cc (right):

https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/prox...
chromeos/dbus/services/proxy_resolution_service_provider.cc:202: const int
result = proxy_service->ResolveProxy(
optional: net::Error (or do we prefer int for errors?)

https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/prox...
File chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc
(right):

https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/prox...
chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc:57:
results->Use(proxy_info_);
Should this only happen in the non-async case? I'm unclear on the contract for
GetProxyForURL().

[Daniel Erat, 2017-03-31 17:46:38]

https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/prox...
File chromeos/dbus/services/proxy_resolution_service_provider.cc (right):

https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/prox...
chromeos/dbus/services/proxy_resolution_service_provider.cc:202: const int
result = proxy_service->ResolveProxy(
On 2017/03/31 17:33:10, James Cook wrote:
> optional: net::Error (or do we prefer int for errors?)

i was actually confused about this too. net::ProxyService::ResolveProxy()
returns int, and moreover, //net/proxy seems to use int extensively. net::Error
is just a regular non-class enum (defined using macros in net/base/net_errors.h
and net_error_list.h), so it probably doesn't matter at all to the compiler, but
i found that this made the code harder to follow if these are all actually
net::Errors. maybe there's some history here, though...

https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/prox...
File chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc
(right):

https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/prox...
chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc:57:
results->Use(proxy_info_);
On 2017/03/31 17:33:10, James Cook wrote:
> Should this only happen in the non-async case? I'm unclear on the contract for
> GetProxyForURL().

nope, this is how the results are returned for async queries too.
net::CompletionCallback is just base::Callback<void(int)>.

[Daniel Erat, 2017-03-31 21:33:26]

i've returned the test code for making ProxyResolver return custom errors, along
with the test that verified that errors are reported correctly. if there's some
way to convince ProxyService to pass back the error that the ProxyResolver
returns, finding it is beyond me.

[Daniel Erat, 2017-03-31 21:33:31]

The CQ bit was checked by derat@chromium.org

[Daniel Erat, 2017-03-31 21:33:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2787253002/#ps20001
(title: "remove error stuff from tests")

[commit-bot: I haz the power, 2017-03-31 21:34:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 22:44:19]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490996011233670,
"parent_rev": "9d766b01914f09b20ef999b3aa6a676c38e65792", "commit_rev":
"4a50453a68e5577dd9a54510c3810d112b35a763"}

[commit-bot: I haz the power, 2017-03-31 22:45:10]

Description was changed from

==========
chromeos: Simplify D-Bus proxy provider (but not tests).

Move some complexity out of
ProxyResolutionServiceProvider::Delegate and add a lot of
complexity to its unit tests by constructing an alternate
net::ProxyService.

BUG=446115,703217,706665
==========

to

==========
chromeos: Simplify D-Bus proxy provider (but not tests).

Move some complexity out of
ProxyResolutionServiceProvider::Delegate and add a lot of
complexity to its unit tests by constructing an alternate
net::ProxyService.

BUG=446115,703217,706665

Review-Url: https://codereview.chromium.org/2787253002
Cr-Commit-Position: refs/heads/master@{#461258}
Committed:
https://chromium.googlesource.com/chromium/src/+/4a50453a68e5577dd9a54510c381...
==========

[commit-bot: I haz the power, 2017-03-31 22:45:11]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/4a50453a68e5577dd9a54510c381...

[eroman, 2017-04-04 21:06:10]

> eric, would you mind taking a quick look at the test file to see if i missed
> some easier way to accomplish what i'm trying to do?

Test LGTM.

Unfortunately there isn't a way around needing to create all these mocks. As you
point out, ProxyService is not mockable, only the ProxyResolver is (so you have
to compose a mock factory, to return a mock resolver, and specify a config that
will cause it to be used).

https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/prox...
File chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc
(right):

https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/prox...
chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc:308: //
about how to get a config with that setting into net::ProxyService().
On 2017/03/31 02:39:44, Daniel Erat wrote:
> i'm not sure how to fix this, or whether i should just delete the test. :-/

If your question is how to mock it in the test, then add:
   config.set_pac_mandatory(true);
After the line "net::ProxyConfig config = net::ProxyConfig::CreateAutoDetect();"
in initialization.

If your question is when it happens in practice, the "mandatory pac" is IIRC
only reachable when configuration is set via extension.

[Daniel Erat, 2017-04-05 02:34:14]

thanks for taking a look!

https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/prox...
File chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc
(right):

https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/prox...
chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc:308: //
about how to get a config with that setting into net::ProxyService().
On 2017/04/04 21:06:10, eroman wrote:
> On 2017/03/31 02:39:44, Daniel Erat wrote:
> > i'm not sure how to fix this, or whether i should just delete the test. :-/
> 
> If your question is how to mock it in the test, then add:
>    config.set_pac_mandatory(true);
> After the line "net::ProxyConfig config =
net::ProxyConfig::CreateAutoDetect();"
> in initialization.
> 
> If your question is when it happens in practice, the "mandatory pac" is IIRC
> only reachable when configuration is set via extension.

oh, hmm. my question was how to mock it in this test. i'm pretty sure that's
exactly what i tried (along with also setting a pac url when that didn't work,
since there appeared to be expectations in ProxyConfig that the url would also
be set).

however, pac_mandatory is always unset in the config that ProxyService ends up
using when i do this. i wasn't successful in tracing exactly what was going on,
but my working theory was that ProxyService ended up not using the original
config from the ProxyConfigService in this case due to the pac config not being
successfully fetched from the pac url.

i can make another change demonstrating the problem if that behavior seems
unexpected to you.

[eroman, 2017-04-05 18:27:40]

Sure, feel free to follow-up with me (can show me using a separate CL where you
see the problem).

I think what you need to change is:

Replace:

net::ProxyConfig config = net::ProxyConfig::CreateAutoDetect();

With:

ProxyConfig config;
config.set_pac_url(GURL("http://www.example.com");
config.set_pac_mandatory(true);

[Daniel Erat, 2017-04-06 00:39:54]

On 2017/04/05 18:27:40, eroman wrote:
> Sure, feel free to follow-up with me (can show me using a separate CL where
you
> see the problem).
> 
> I think what you need to change is:
> 
> Replace:
> 
> net::ProxyConfig config = net::ProxyConfig::CreateAutoDetect();
> 
> With:
> 
> ProxyConfig config;
> config.set_pac_url(GURL("http://www.example.com");
> config.set_pac_mandatory(true);

thanks! that's what i was missing, i think -- i was trying to set pac_url and
pac_mandatory after calling CreateAutoDetect(). when i instead set them on an
empty config that doesn't have auto_detect set and then return an error from
ProxyResolver::GetProxyForURL(), i get an ultimate response of
net::ERR_MANDATORY_PROXY_CONFIGURATION_FAILED (along with "DIRECT" as the PAC
string).