Dynamically refresh pref-configured proxies.

chrome/browser/net/pref_proxy_config_service_unittest.cc

+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/command_line.h"
+#include "base/file_path.h"
+#include "chrome/browser/net/chrome_url_request_context.h"
+#include "chrome/browser/net/pref_proxy_config_service.h"

[eroman, 2010/11/20 03:19:07]

nit: please put this at the top of the file.

[Mattias Nissler (ping if slow), 2010/11/21 22:49:14]

Done.

+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/testing_pref_service.h"
+#include "net/proxy/proxy_config_service_fixed.h"
+#include "net/proxy/proxy_config_service_common_unittest.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::_;

[eroman, 2010/11/20 03:19:07]

What is this?

[Mattias Nissler (ping if slow), 2010/11/21 22:49:14]

This is gmock's "any value" argument matcher for setting up call expectations.

+using testing::Mock;
+
+namespace {
+
+const char kFixedPACURL[] = "http://chromium.org/fixed_pac_url";
+
+// Testing proxy config service that allows us to fire notifications at will.
+class TestProxyConfigService : public net::ProxyConfigServiceFixed {

[eroman, 2010/11/20 03:19:07]

ProxyConfigServiceFixed is so small, that I don't think there is any advantage
to extending it.

I think you would be better off just creating a standalone class.

[Mattias Nissler (ping if slow), 2010/11/21 22:49:14]

Done.

+ public:
+  explicit TestProxyConfigService(const net::ProxyConfig& config)
+      : net::ProxyConfigServiceFixed(config) {
+  }
+
+  void FireObservers() {

[eroman, 2010/11/20 03:19:07]

What is a weird about this, is the config didn't actually change (we are firing
with the same value as before).

See comment above about not inheriting from ProxyConfigServiceFixed.

I propose instead naming this method:

void SetProxyConfig(const ProxyConfig& new_config);

And updating our config_ member variable, and then firing the observers.

[Mattias Nissler (ping if slow), 2010/11/21 22:49:14]

Done.

+    net::ProxyConfig config;
+    GetLatestProxyConfig(&config);
+    FOR_EACH_OBSERVER(net::ProxyConfigService::Observer, observers_,
+                      OnProxyConfigChanged(config));
+  }
+
+ private:
+  virtual void AddObserver(net::ProxyConfigService::Observer* observer) {
+    observers_.AddObserver(observer);
+  }
+
+  virtual void RemoveObserver(net::ProxyConfigService::Observer* observer) {
+    observers_.RemoveObserver(observer);
+  }
+
+  ObserverList<net::ProxyConfigService::Observer> observers_;

[eroman, 2010/11/20 03:19:07]

I recommend adding the ", true" to template trait, to catch cases where we don't
remove observers.

[Mattias Nissler (ping if slow), 2010/11/21 22:49:14]

Done.

+};
+
+// A mock observer for capturing callbacks.
+class MockObserver : public net::ProxyConfigService::Observer {
+ public:
+  MOCK_METHOD1(OnProxyConfigChanged, void(const net::ProxyConfig&));
+};
+
+template<typename TESTBASE>
+class PrefProxyConfigServiceTestBase : public TESTBASE {
+ protected:
+  PrefProxyConfigServiceTestBase()
+      : ui_thread_(BrowserThread::UI, &loop_),
+        io_thread_(BrowserThread::IO, &loop_) {}
+
+  virtual void SetUp() {
+    ASSERT_TRUE(pref_service_.get());
+    ChromeURLRequestContextGetter::RegisterUserPrefs(pref_service_.get());
+    fixed_config_.set_pac_url(GURL(kFixedPACURL));
+    fixed_config_.set_id(42);
+    delegate_service_ = new TestProxyConfigService(fixed_config_);
+    proxy_config_tracker_ = new PrefProxyConfigTracker(pref_service_.get());
+    proxy_config_service_.reset(
+        new PrefProxyConfigService(proxy_config_tracker_.get(),
+                                   delegate_service_));
+  }
+
+  virtual void TearDown() {
+    proxy_config_tracker_->Shutdown();
+    loop_.RunAllPending();
+    proxy_config_service_.reset();
+    pref_service_.reset();
+  }
+
+  MessageLoop loop_;
+  TestProxyConfigService* delegate_service_; // weak
+  scoped_ptr<PrefProxyConfigService> proxy_config_service_;
+  scoped_ptr<TestingPrefService> pref_service_;

[eroman, 2010/11/20 03:19:07]

nit: I suggest ordering this above proxy_config_service_ (just to emphasize that
it outlives it).

[Mattias Nissler (ping if slow), 2010/11/21 22:49:14]

Done.

+  net::ProxyConfig fixed_config_;
+
+ private:
+  scoped_refptr<PrefProxyConfigTracker> proxy_config_tracker_;
+  BrowserThread ui_thread_;
+  BrowserThread io_thread_;
+};
+
+class PrefProxyConfigServiceTest :

[eroman, 2010/11/20 03:19:07]

style nit: put the colon on the next line (and indent by 4 spaces).

[Mattias Nissler (ping if slow), 2010/11/21 22:49:14]

Done.

+  public PrefProxyConfigServiceTestBase<testing::Test> {
+ protected:
+  virtual void SetUp() {
+    pref_service_.reset(new TestingPrefService);
+    PrefProxyConfigServiceTestBase<testing::Test>::SetUp();
+  }
+};
+
+TEST_F(PrefProxyConfigServiceTest, BaseConfiguration) {
+  net::ProxyConfig actual_config;
+  proxy_config_service_->GetLatestProxyConfig(&actual_config);
+  EXPECT_EQ(GURL(kFixedPACURL), actual_config.pac_url());
+}
+
+TEST_F(PrefProxyConfigServiceTest, DynamicPrefOverrides) {
+  pref_service_->SetManagedPref(
+      prefs::kProxyServer, Value::CreateStringValue("http://example.com:3128"));
+  loop_.RunAllPending();
+
+  net::ProxyConfig actual_config;
+  proxy_config_service_->GetLatestProxyConfig(&actual_config);
+  EXPECT_FALSE(actual_config.auto_detect());
+  EXPECT_EQ(net::ProxyConfig::ProxyRules::TYPE_SINGLE_PROXY,
+            actual_config.proxy_rules().type);
+  EXPECT_EQ(actual_config.proxy_rules().single_proxy,
+            net::ProxyServer::FromURI("http://example.com:3128",
+                                      net::ProxyServer::SCHEME_HTTP));
+
+  pref_service_->SetManagedPref(
+      prefs::kProxyAutoDetect, Value::CreateBooleanValue(true));
+  loop_.RunAllPending();
+
+  proxy_config_service_->GetLatestProxyConfig(&actual_config);
+  EXPECT_TRUE(actual_config.auto_detect());
+}
+
+// Compares proxy configurations, but allows different identifiers.
+MATCHER_P(ProxyConfigMatches, config, "") {
+  net::ProxyConfig reference(config);
+  reference.set_id(arg.id());
+  return reference.Equals(arg);
+}
+
+TEST_F(PrefProxyConfigServiceTest, Observers) {

[eroman, 2010/11/20 03:19:07]

Thanks for adding extensive tests!

+  MockObserver observer;
+  proxy_config_service_->AddObserver(&observer);
+
+  // Firing the observers in the delegate should trigger a notification.
+  EXPECT_CALL(observer,

[eroman, 2010/11/20 03:19:07]

I'll give this a more careful re-read later, I must admit that I don't actually
understand gmock.

[Mattias Nissler (ping if slow), 2010/11/21 22:49:14]

It declares that the subsequent code will generate a call to
OnProxyConfigChanged with a config object that'll match fixed_config_ (as
determined by the matcher). When we call Mock::VerifyAndClearExpactations below,
gmock will check that this has happened and fail the test if it didn't.

+              OnProxyConfigChanged(ProxyConfigMatches(fixed_config_))).Times(1);
+  delegate_service_->FireObservers();
+  loop_.RunAllPending();
+  Mock::VerifyAndClearExpectations(&observer);
+
+  // Override configuration, this should trigger a notification.
+  net::ProxyConfig pref_config;
+  pref_config.set_auto_detect(true);
+  EXPECT_CALL(observer,
+              OnProxyConfigChanged(ProxyConfigMatches(pref_config))).Times(1);
+  pref_service_->SetManagedPref(
+      prefs::kProxyAutoDetect, Value::CreateBooleanValue(true));
+  loop_.RunAllPending();
+  Mock::VerifyAndClearExpectations(&observer);
+
+  // Since there are pref overrides, delegate changes should be ignored.
+  EXPECT_CALL(observer, OnProxyConfigChanged(_)).Times(0);
+  delegate_service_->FireObservers();
+  loop_.RunAllPending();
+  Mock::VerifyAndClearExpectations(&observer);
+
+  // Clear the override should switch back to the fixed configuration.
+  EXPECT_CALL(observer,
+              OnProxyConfigChanged(ProxyConfigMatches(fixed_config_))).Times(1);
+  pref_service_->RemoveManagedPref(prefs::kProxyAutoDetect);
+  loop_.RunAllPending();
+  Mock::VerifyAndClearExpectations(&observer);
+
+  // Delegate service notifications should show up again.
+  EXPECT_CALL(observer,
+              OnProxyConfigChanged(ProxyConfigMatches(fixed_config_))).Times(1);
+  delegate_service_->FireObservers();
+  loop_.RunAllPending();
+  Mock::VerifyAndClearExpectations(&observer);
+
+  proxy_config_service_->RemoveObserver(&observer);
+}
+
+// Test parameter object for testing command line proxy configuration.
+struct CommandLineTestParams {
+  // Explicit assignment operator, so testing::TestWithParam works with MSVC.
+  CommandLineTestParams& operator=(const CommandLineTestParams& other) {
+    description = other.description;
+    for (unsigned int i = 0; i < arraysize(switches); i++)
+      switches[i] = other.switches[i];
+    is_null = other.is_null;
+    auto_detect = other.auto_detect;
+    pac_url = other.pac_url;
+    proxy_rules = other.proxy_rules;
+    return *this;
+  }
+
+  // Short description to identify the test.
+  const char* description;
+
+  // The command line to build a ProxyConfig from.
+  struct SwitchValue {
+    const char* name;
+    const char* value;
+  } switches[2];
+
+  // Expected outputs (fields of the ProxyConfig).
+  bool is_null;
+  bool auto_detect;
+  GURL pac_url;
+  net::ProxyRulesExpectation proxy_rules;
+};
+
+void PrintTo(const CommandLineTestParams& params, std::ostream* os) {
+  *os << params.description;
+}
+
+class PrefProxyConfigServiceCommandLineTest
+    : public PrefProxyConfigServiceTestBase<
+          testing::TestWithParam<CommandLineTestParams> > {
+ protected:
+  PrefProxyConfigServiceCommandLineTest()
+      : command_line_(CommandLine::NO_PROGRAM) {}
+
+  virtual void SetUp() {
+    for (unsigned int i = 0; i < arraysize(GetParam().switches); i++) {

[eroman, 2010/11/20 03:19:07]

nit: I have typically seen size_t used for such loops.

[Mattias Nissler (ping if slow), 2010/11/21 22:49:14]

Done.

+      const char* name = GetParam().switches[i].name;
+      const char* value = GetParam().switches[i].value;
+      if (name && value)
+        command_line_.AppendSwitchASCII(name, value);
+      else if (name)
+        command_line_.AppendSwitch(name);
+    }
+    pref_service_.reset(new TestingPrefService(NULL, &command_line_));
+    PrefProxyConfigServiceTestBase<
+        testing::TestWithParam<CommandLineTestParams> >::SetUp();
+  }
+
+ private:
+  CommandLine command_line_;
+};
+
+TEST_P(PrefProxyConfigServiceCommandLineTest, CommandLine) {
+  net::ProxyConfig config;
+  proxy_config_service_->GetLatestProxyConfig(&config);
+
+  if (GetParam().is_null) {
+    EXPECT_EQ(GURL(kFixedPACURL), config.pac_url());
+  } else {
+    EXPECT_NE(GURL(kFixedPACURL), config.pac_url());
+    EXPECT_EQ(GetParam().auto_detect, config.auto_detect());
+    EXPECT_EQ(GetParam().pac_url, config.pac_url());
+    EXPECT_TRUE(GetParam().proxy_rules.Matches(config.proxy_rules()));
+  }
+}
+
+static const CommandLineTestParams kCommandLineTestParams[] = {
+  {
+    "Empty command line",
+    // Input
+    { },
+    // Expected result
+    true,                                               // is_null
+    false,                                              // auto_detect
+    GURL(),                                             // pac_url
+    net::ProxyRulesExpectation::Empty(),
+  },
+  {
+    "No proxy",
+    // Input
+    {
+      { switches::kNoProxyServer, NULL },
+    },
+    // Expected result
+    false,                                              // is_null
+    false,                                              // auto_detect
+    GURL(),                                             // pac_url
+    net::ProxyRulesExpectation::Empty(),
+  },
+  {
+    "No proxy with extra parameters.",
+    // Input
+    {
+      { switches::kNoProxyServer, NULL },
+      { switches::kProxyServer, "http://proxy:8888" },
+    },
+    // Expected result
+    false,                                              // is_null
+    false,                                              // auto_detect
+    GURL(),                                             // pac_url
+    net::ProxyRulesExpectation::Empty(),
+  },
+  {
+    "Single proxy.",
+    // Input
+    {
+      { switches::kProxyServer, "http://proxy:8888" },
+    },
+    // Expected result
+    false,                                              // is_null
+    false,                                              // auto_detect
+    GURL(),                                             // pac_url
+    net::ProxyRulesExpectation::Single(
+        "proxy:8888",  // single proxy
+        ""),           // bypass rules
+  },
+  {
+    "Per scheme proxy.",
+    // Input
+    {
+      { switches::kProxyServer, "http=httpproxy:8888;ftp=ftpproxy:8889" },
+    },
+    // Expected result
+    false,                                              // is_null
+    false,                                              // auto_detect
+    GURL(),                                             // pac_url
+    net::ProxyRulesExpectation::PerScheme(
+        "httpproxy:8888",  // http
+        "",                // https
+        "ftpproxy:8889",   // ftp
+        ""),               // bypass rules
+  },
+  {
+    "Per scheme proxy with bypass URLs.",
+    // Input
+    {
+      { switches::kProxyServer, "http=httpproxy:8888;ftp=ftpproxy:8889" },
+      { switches::kProxyBypassList,
+        ".google.com, foo.com:99, 1.2.3.4:22, 127.0.0.1/8" },
+    },
+    // Expected result
+    false,                                              // is_null
+    false,                                              // auto_detect
+    GURL(),                                             // pac_url
+    net::ProxyRulesExpectation::PerScheme(
+        "httpproxy:8888",  // http
+        "",                // https
+        "ftpproxy:8889",   // ftp
+        "*.google.com,foo.com:99,1.2.3.4:22,127.0.0.1/8"),
+  },
+  {
+    "Pac URL with proxy bypass URLs",
+    // Input
+    {
+      { switches::kProxyPacUrl, "http://wpad/wpad.dat" },
+      { switches::kProxyBypassList,
+        ".google.com, foo.com:99, 1.2.3.4:22, 127.0.0.1/8" },
+    },
+    // Expected result
+    false,                                              // is_null
+    false,                                              // auto_detect
+    GURL("http://wpad/wpad.dat"),                       // pac_url
+    net::ProxyRulesExpectation::EmptyWithBypass(
+        "*.google.com,foo.com:99,1.2.3.4:22,127.0.0.1/8"),
+  },
+  {
+    "Autodetect",
+    // Input
+    {
+      { switches::kProxyAutoDetect, NULL },
+    },
+    // Expected result
+    false,                                              // is_null
+    true,                                               // auto_detect
+    GURL(),                                             // pac_url
+    net::ProxyRulesExpectation::Empty(),
+  },
+};
+
+INSTANTIATE_TEST_CASE_P(
+    PrefProxyConfigServiceCommandLineTestInstance,
+    PrefProxyConfigServiceCommandLineTest,
+    testing::ValuesIn(kCommandLineTestParams));
+
+}  // namespace