Adds support for the DHCP portion of the WPAD (proxy auto-discovery) protocol.

net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc

Index: net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc
diff --git a/net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc b/net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..3a12f55472d36464292cc924805ff458b6654557
--- /dev/null
+++ b/net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc
@@ -0,0 +1,252 @@
+// Copyright (c) 2011 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/timer.h"
+#include "base/perftimer.h"

[eroman, 2011/04/21 05:22:48]

nit: order header lexicographically.

[Jói, 2011/05/03 21:20:59]

Done.

+#include "net/base/net_errors.h"
+#include "net/proxy/dhcp_proxy_script_adapter_fetcher_win.h"

[eroman, 2011/04/21 05:22:48]

nit: it isn't explicit in the style guide, but I assume it is consistent to put
the header of the file under test at the top (similar to what we do for .cc
files).

[Jói, 2011/05/03 21:20:59]

Looking at _unittest.cc files under net/ it seems there may be a slight
preference for doing what you suggested, so I have switched to be consistent. 
Looking at all of the Chrome code base, the preference seems to be for what I
had before switching.

[Jói, 2011/05/05 14:58:42]

I was just looking at include order in the style guide again.  The Google style
actually does make this explicit, and the Chromium style guide refers to it.  So
thanks for pointing this out to me, I had been using the wrong approach for
quite a while.

+#include "net/proxy/mock_proxy_script_fetcher.h"
+#include "net/test/test_server.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+
+const char* const kPacUrl = "http://pacserver/script.pac";

[eroman, 2011/04/21 05:22:48]

can this go into the anonymous namespace?

[Jói, 2011/05/03 21:20:59]

Done.

+
+namespace {
+
+// In net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc there are a few
+// tests that exercise DhcpProxyScriptAdapterFetcher end-to-end along with
+// WindowsDhcpProxyScriptFetcher, i.e. it tests the end-to-end usage of Win32
+// APIs and the network.  In this file we test only by stubbing out
+// functionality.
+
+// Version of DhcpProxyScriptAdapterFetcher that mocks out dependencies
+// to allow unit testing.
+class MockDhcpProxyScriptAdapterFetcher : public DhcpProxyScriptAdapterFetcher {
+ public:
+  explicit MockDhcpProxyScriptAdapterFetcher()

[eroman, 2011/04/21 05:22:48]

nit: omit explicit on 0-arg constructors.

[Jói, 2011/05/03 21:20:59]

Thanks (it was 1-arg at some point).  It is now 1-arg again :)

+      : DhcpProxyScriptAdapterFetcher(new TestURLRequestContext()),

[eroman, 2011/04/21 05:22:48]

won't this leak? (I don't believe the adapter holds any references).

[Jói, 2011/05/03 21:20:59]

You're right; I was thinking it held it via scoped_refptr but it doesn't. Fixed
the test code.

+        dhcp_delay_ms_(1),
+        configured_url_(kPacUrl),
+        fetcher_delay_ms_(1),
+        fetcher_result_(OK),
+        pac_script_("bingo") {
+  }
+
+  void Cancel() {
+    DhcpProxyScriptAdapterFetcher::Cancel();
+    fetcher_ = NULL;
+  }
+
+  std::string ImplGetPacURLFromDhcp(const std::string& adapter_name) OVERRIDE {
+    base::PlatformThread::Sleep(dhcp_delay_ms_);
+    return configured_url_;
+  }
+
+  virtual ProxyScriptFetcher* ImplCreateScriptFetcher(
+      const GURL& url) OVERRIDE {
+    // We don't maintain ownership of the fetcher, it is transferred to
+    // the caller.
+    fetcher_ = new MockProxyScriptFetcher();
+    fetcher_->SetURL(url);
+    if (fetcher_delay_ms_ != -1) {
+      fetcher_timer_.Start(
+          base::TimeDelta::FromMilliseconds(fetcher_delay_ms_),
+          this, &MockDhcpProxyScriptAdapterFetcher::OnFetcherTimer);
+    }
+    return fetcher_;
+  }
+
+  // Use a shorter timeout so tests can finish more quickly.
+  virtual int ImplGetTimeoutMs() const OVERRIDE {
+    return 25;
+  }
+
+  void OnFetcherTimer() {
+    DCHECK(fetcher_);
+    fetcher_->NotifyFetchCompletion(fetcher_result_, pac_script_);
+    fetcher_ = NULL;
+  }
+
+  bool IsWaitingForFetcher() const {
+    return state_ == STATE_WAIT_URL;
+  }
+
+  bool WasCancelled() const {
+    return state_ == STATE_CANCEL;
+  }
+
+  int dhcp_delay_ms_;
+  std::string configured_url_;
+  int fetcher_delay_ms_;
+  int fetcher_result_;
+  std::string pac_script_;
+  MockProxyScriptFetcher* fetcher_;
+  base::OneShotTimer<MockDhcpProxyScriptAdapterFetcher> fetcher_timer_;
+};
+
+class FetcherClient {
+public:

[eroman, 2011/04/21 05:22:48]

nit: indent by 1.

[Jói, 2011/05/03 21:20:59]

Done.

+  FetcherClient()
+      : finished_(false),
+        fetcher_(new MockDhcpProxyScriptAdapterFetcher()),
+        ALLOW_THIS_IN_INITIALIZER_LIST(
+            completion_callback_(this, &FetcherClient::OnCompletion)) {
+  }
+
+  void RunTest() {
+    fetcher_->Fetch("adapter name", &completion_callback_);
+  }
+
+  void RunMessageLoopUntilComplete() {
+    while (!finished_) {
+      MessageLoop::current()->RunAllPending();
+    }
+    MessageLoop::current()->RunAllPending();
+  }
+
+  void OnCompletion(int result) {
+    finished_ = true;
+  }
+
+  bool finished_;
+  scoped_refptr<MockDhcpProxyScriptAdapterFetcher> fetcher_;
+  string16 pac_text_;
+  CompletionCallbackImpl<FetcherClient> completion_callback_;
+};
+
+TEST(DhcpProxyScriptAdapterFetcher, NormalCaseURLNotInDhcp) {
+  FetcherClient client;
+  client.fetcher_->configured_url_ = "";
+  client.RunTest();
+  client.RunMessageLoopUntilComplete();
+  ASSERT_TRUE(client.fetcher_->DidFinish());
+  EXPECT_EQ(ERR_PAC_NOT_IN_DHCP, client.fetcher_->result());
+  EXPECT_EQ(string16(L""), client.fetcher_->pac_script());

[eroman, 2011/04/21 05:22:48]

nit: i believe you can leave of the string16() in cases like this and rely on
implicit constructor. Up to you.

[Jói, 2011/05/03 21:20:59]

Leaving as is.

+}
+
+TEST(DhcpProxyScriptAdapterFetcher, NormalCaseURLInDhcp) {
+  FetcherClient client;
+  client.RunTest();
+  client.RunMessageLoopUntilComplete();
+  ASSERT_TRUE(client.fetcher_->DidFinish());
+  EXPECT_EQ(OK, client.fetcher_->result());
+  EXPECT_EQ(string16(L"bingo"), client.fetcher_->pac_script());
+  EXPECT_EQ(GURL(kPacUrl), client.fetcher_->pac_url());
+}
+
+TEST(DhcpProxyScriptAdapterFetcher, TimeoutDuringDhcp) {
+  // Does a Fetch() with a long enough delay on accessing DHCP that the
+  // fetcher should time out.  This is to test a case manual testing found,
+  // where under certain circumstances (e.g. adapter enabled for DHCP and
+  // needs to retrieve its configuration from DHCP, but no DHCP server
+  // present on the network) accessing DHCP can take on the order of tens
+  // of seconds.
+  FetcherClient client;
+  client.fetcher_->dhcp_delay_ms_ = 20 * 1000;

[eroman, 2011/04/21 05:22:48]

One thing to be aware of is valgrind may end up reporting leaks for this sort of
test (since the worker thread may be keeping objects alive past the test).

One approach to mitigate that, is to set a lower timeout (which we are willing
to wait for), and then wait for it to complete before exiting the test.
Otherwise be prepared to add some valgrind suppressions :)

[Jói, 2011/05/03 21:20:59]

I added some granularity to the wait loop so that the test can stop the fake
worker thread soon when the test is done, and used that to exit as soon as
possible from the test after the thread is gone.

+
+  PerfTimer timer;

[eroman, 2011/04/21 05:22:48]

What is the perf timer being used for?

[Jói, 2011/05/03 21:20:59]

See the call to timer.Elapsed() within the ASSERT_GT a few lines further down.

+  client.RunTest();
+  client.RunMessageLoopUntilComplete();
+
+  // The timeout should occur within about 25 ms, way before the 20s set as
+  // the API delay above.
+  ASSERT_GT(base::TimeDelta::FromMilliseconds(35), timer.Elapsed());
+  ASSERT_TRUE(client.fetcher_->DidFinish());
+  EXPECT_EQ(ERR_TIMED_OUT, client.fetcher_->result());
+  EXPECT_EQ(string16(L""), client.fetcher_->pac_script());
+  EXPECT_EQ(GURL(), client.fetcher_->pac_url());
+}
+
+TEST(DhcpProxyScriptAdapterFetcher, CancelWhileDhcp) {
+  FetcherClient client;
+  client.fetcher_->dhcp_delay_ms_ = 10;
+  client.RunTest();
+  client.fetcher_->Cancel();
+  MessageLoop::current()->RunAllPending();
+  ASSERT_FALSE(client.fetcher_->DidFinish());
+  ASSERT_TRUE(client.fetcher_->WasCancelled());
+  EXPECT_EQ(ERR_ABORTED, client.fetcher_->result());
+  EXPECT_EQ(string16(L""), client.fetcher_->pac_script());
+  EXPECT_EQ(GURL(), client.fetcher_->pac_url());
+}
+
+TEST(DhcpProxyScriptAdapterFetcher, CancelWhileFetcher) {
+  FetcherClient client;
+  // This causes the mock fetcher not to pretend the
+  // fetcher finishes after a timeout.
+  client.fetcher_->fetcher_delay_ms_ = -1;
+  client.RunTest();
+  int max_loops = 4;
+  while (!client.fetcher_->IsWaitingForFetcher() && max_loops--) {
+    base::PlatformThread::Sleep(10);
+    MessageLoop::current()->RunAllPending();
+  }
+  client.fetcher_->Cancel();
+  MessageLoop::current()->RunAllPending();
+  ASSERT_FALSE(client.fetcher_->DidFinish());
+  ASSERT_TRUE(client.fetcher_->WasCancelled());
+  EXPECT_EQ(ERR_ABORTED, client.fetcher_->result());
+  EXPECT_EQ(string16(L""), client.fetcher_->pac_script());
+  // GetPacURL() still returns the URL fetched in this case.
+  EXPECT_EQ(GURL(kPacUrl), client.fetcher_->pac_url());
+}
+
+TEST(DhcpProxyScriptAdapterFetcher, CancelAtCompletion) {
+  FetcherClient client;
+  client.RunTest();
+  client.RunMessageLoopUntilComplete();
+  client.fetcher_->Cancel();
+  // Canceling after you're done should have no effect, so these
+  // are identical expectations to the NormalCaseURLInDhcp test.
+  ASSERT_TRUE(client.fetcher_->DidFinish());
+  EXPECT_EQ(OK, client.fetcher_->result());
+  EXPECT_EQ(string16(L"bingo"), client.fetcher_->pac_script());
+  EXPECT_EQ(GURL(kPacUrl), client.fetcher_->pac_url());
+}
+
+// Does a real fetch on a mock DHCP configuration.
+class MockDhcpRealFetchProxyScriptAdapterFetcher
+    : public MockDhcpProxyScriptAdapterFetcher {
+public:

[eroman, 2011/04/21 05:22:48]

extra space.

[Jói, 2011/05/03 21:20:59]

Done.

+  MockDhcpRealFetchProxyScriptAdapterFetcher()
+      : MockDhcpProxyScriptAdapterFetcher(),
+        url_request_context_(new TestURLRequestContext()) {
+  }
+
+  // Returns a real proxy script fetcher.
+  ProxyScriptFetcher* ImplCreateScriptFetcher(const GURL& url) OVERRIDE {
+    URLProxyScriptFetcher* fetcher =
+        new ProxyScriptFetcherImpl(url_request_context_.get());
+    fetcher->SetURL(url);
+    return fetcher;
+  }
+
+  scoped_refptr<URLRequestContext> url_request_context_;
+};
+
+TEST(DhcpProxyScriptAdapterFetcher, MockDhcpRealFetch) {
+  TestServer test_server(
+      TestServer::TYPE_HTTP,
+      FilePath(FILE_PATH_LITERAL("net/data/proxy_script_fetcher_unittest")));
+  ASSERT_TRUE(test_server.Start());
+
+  GURL configured_url = test_server.GetURL("files/downloadable.pac");
+
+  FetcherClient client;
+  client.fetcher_ = new MockDhcpRealFetchProxyScriptAdapterFetcher();
+  client.fetcher_->configured_url_ = configured_url.spec();
+  client.RunTest();
+  client.RunMessageLoopUntilComplete();
+  ASSERT_TRUE(client.fetcher_->DidFinish());
+  EXPECT_EQ(OK, client.fetcher_->result());
+  EXPECT_EQ(string16(L"-downloadable.pac-\n"), client.fetcher_->pac_script());
+  EXPECT_EQ(configured_url,
+            client.fetcher_->pac_url());
+}
+
+}  // namespace
+}  // namespace net