Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2798223002:
chromeos: Reintroduce proxy resolution service error test. (Closed)
|
Created:
3 years, 8 months ago by Daniel Erat Modified:
3 years, 8 months ago Reviewers:
eroman CC:
chromium-reviews, oshima+watch_chromium.org, hashimoto+watch_chromium.org, James Cook Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionchromeos: Reintroduce proxy resolution service error test.
Reintroduce an error-reporting test for
ProxyResolutionServiceProvider. This is a bit trickier to do
when using a full net::ProxyService.
BUG=706665
Review-Url: https://codereview.chromium.org/2798223002
Cr-Commit-Position: refs/heads/master@{#463078}
Committed: https://chromium.googlesource.com/chromium/src/+/bee89c0dab49b79856ec089111e2d70cd97402ac
Patch Set 1 #
Total comments: 4
Messages
Total messages: 16 (12 generated)
derat@chromium.org changed reviewers: + eroman@chromium.org
thanks again for the help here, eric! https://codereview.chromium.org/2798223002/diff/1/chromeos/dbus/services/prox... File chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc (right): https://codereview.chromium.org/2798223002/diff/1/chromeos/dbus/services/prox... chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc:162: config.set_pac_mandatory(true); is it legitimate to always set this, or should i only use it when i want to trigger an error and use CreateAutoDetect() otherwise? (that'll require some restructuring since this is currently initialized in the test class's c'tor.) https://codereview.chromium.org/2798223002/diff/1/chromeos/dbus/services/prox... chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc:406: EXPECT_EQ("DIRECT", proxy_info); it's a bit surprising to me that the chrome os code returns proxy info even when an error was received, but it looks like this is what the code did before i started trying to clean it up. hopefully all the callers do the right thing... :-/
The CQ bit was checked by derat@chromium.org to run a CQ dry run
Dry run: 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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by derat@chromium.org to run a CQ dry run
Dry run: 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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thanks for the follow-through! https://codereview.chromium.org/2798223002/diff/1/chromeos/dbus/services/prox... File chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc (right): https://codereview.chromium.org/2798223002/diff/1/chromeos/dbus/services/prox... chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc:162: config.set_pac_mandatory(true); On 2017/04/06 01:21:39, Daniel Erat wrote: > is it legitimate to always set this, or should i only use it when i want to > trigger an error and use CreateAutoDetect() otherwise? (that'll require some > restructuring since this is currently initialized in the test class's c'tor.) As written is good! https://codereview.chromium.org/2798223002/diff/1/chromeos/dbus/services/prox... chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc:406: EXPECT_EQ("DIRECT", proxy_info); On 2017/04/06 01:21:39, Daniel Erat wrote: > it's a bit surprising to me that the chrome os code returns proxy info even when > an error was received, but it looks like this is what the code did before i > started trying to clean it up. hopefully all the callers do the right thing... > :-/ .. that I can't comment on, not familiar with the protocol being used here. As long as callers check the error first then I guess it is fine :)
The CQ bit was checked by derat@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": 1, "attempt_start_ts": 1491609833429100, "parent_rev":
"46a8c1980ada358234a339dc009954b8c9abfe6a", "commit_rev":
"bee89c0dab49b79856ec089111e2d70cd97402ac"}
Message was sent while issue was closed.
Description was changed from ========== chromeos: Reintroduce proxy resolution service error test. Reintroduce an error-reporting test for ProxyResolutionServiceProvider. This is a bit trickier to do when using a full net::ProxyService. BUG=706665 ========== to ========== chromeos: Reintroduce proxy resolution service error test. Reintroduce an error-reporting test for ProxyResolutionServiceProvider. This is a bit trickier to do when using a full net::ProxyService. BUG=706665 Review-Url: https://codereview.chromium.org/2798223002 Cr-Commit-Position: refs/heads/master@{#463078} Committed: https://chromium.googlesource.com/chromium/src/+/bee89c0dab49b79856ec089111e2... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/bee89c0dab49b79856ec089111e2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
