Run safebrowsing_service_test through the net testserver code.

chrome/browser/safe_browsing/safe_browsing_test.cc

Index: chrome/browser/safe_browsing/safe_browsing_test.cc
diff --git a/chrome/browser/safe_browsing/safe_browsing_test.cc b/chrome/browser/safe_browsing/safe_browsing_test.cc
index 3983e833c941f2389561de6ea081fbc82c001f77..0899f349de636d4d22aa0286c89f7bb09e07750b 100644
--- a/chrome/browser/safe_browsing/safe_browsing_test.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_test.cc
@@ -45,6 +45,7 @@
 #include "net/base/load_flags.h"
 #include "net/base/net_log.h"
 #include "net/test/python_utils.h"
+#include "net/test/test_server.h"
 #include "net/url_request/url_request_status.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -56,7 +57,6 @@ const FilePath::CharType kDataFile[] =
     FILE_PATH_LITERAL("testing_input_nomac.dat");
 const char kUrlVerifyPath[] = "/safebrowsing/verify_urls";
 const char kDBVerifyPath[] = "/safebrowsing/verify_database";
-const char kDBResetPath[] = "/reset";
 const char kTestCompletePath[] = "/test_complete";
 
 struct PhishingUrl {
@@ -107,103 +107,6 @@ bool ParsePhishingUrls(const std::string& data,
 
 }  // namespace
 
-class SafeBrowsingTestServer {
- public:
-  explicit SafeBrowsingTestServer(const FilePath& datafile)
-      : datafile_(datafile),
-        server_handle_(base::kNullProcessHandle) {
-  }
-
-  ~SafeBrowsingTestServer() {
-    EXPECT_EQ(base::kNullProcessHandle, server_handle_);
-  }
-
-  // Start the python server test suite.
-  bool Start() {
-    // Get path to python server script
-    FilePath testserver_path;
-    if (!PathService::Get(base::DIR_SOURCE_ROOT, &testserver_path)) {
-      LOG(ERROR) << "Failed to get DIR_SOURCE_ROOT";
-      return false;
-    }
-    testserver_path = testserver_path
-        .Append(FILE_PATH_LITERAL("third_party"))
-        .Append(FILE_PATH_LITERAL("safe_browsing"))
-        .Append(FILE_PATH_LITERAL("testing"));
-    AppendToPythonPath(testserver_path);
-    FilePath testserver = testserver_path.Append(
-        FILE_PATH_LITERAL("safebrowsing_test_server.py"));
-
-    FilePath pyproto_code_dir;
-    if (!GetPyProtoPath(&pyproto_code_dir)) {
-      LOG(ERROR) << "Failed to get generated python protobuf dir";
-      return false;
-    }
-    AppendToPythonPath(pyproto_code_dir);
-    pyproto_code_dir = pyproto_code_dir.Append(FILE_PATH_LITERAL("google"));
-    AppendToPythonPath(pyproto_code_dir);
-
-    FilePath python_runtime;
-    EXPECT_TRUE(GetPythonRunTime(&python_runtime));
-    CommandLine cmd_line(python_runtime);
-    // Make python stdout and stderr unbuffered, to prevent incomplete stderr on
-    // win bots, and also fix mixed up ordering of stdout and stderr.
-    cmd_line.AppendSwitch("-u");
-    FilePath datafile = testserver_path.Append(datafile_);
-    cmd_line.AppendArgPath(testserver);
-    cmd_line.AppendArg(base::StringPrintf("--port=%d", kPort_));
-    cmd_line.AppendArgNative(FILE_PATH_LITERAL("--datafile=") +
-                             datafile.value());
-
-    base::LaunchOptions options;
-#if defined(OS_WIN)
-    options.start_hidden = true;
-#endif
-    if (!base::LaunchProcess(cmd_line, options, &server_handle_)) {
-      LOG(ERROR) << "Failed to launch server: "
-                 << cmd_line.GetCommandLineString();
-      return false;
-    }
-    return true;
-  }
-
-  // Stop the python server test suite.
-  bool Stop() {
-    if (server_handle_ == base::kNullProcessHandle)
-      return true;
-
-    // First check if the process has already terminated.
-    if (!base::WaitForSingleProcess(server_handle_, 0) &&
-        !base::KillProcess(server_handle_, 1, true)) {
-      VLOG(1) << "Kill failed?";
-      return false;
-    }
-
-    base::CloseProcessHandle(server_handle_);
-    server_handle_ = base::kNullProcessHandle;
-    VLOG(1) << "Stopped.";
-    return true;
-  }
-
-  static const char* Host() {
-    return kHost_;
-  }
-
-  static int Port() {
-    return kPort_;
-  }
-
- private:
-  static const char kHost_[];
-  static const int kPort_;
-  FilePath datafile_;
-  base::ProcessHandle server_handle_;
-  DISALLOW_COPY_AND_ASSIGN(SafeBrowsingTestServer);
-};
-
-const char SafeBrowsingTestServer::kHost_[] = "localhost";
-const int SafeBrowsingTestServer::kPort_ = 40102;
-
 // This starts the browser and keeps status of states related to SafeBrowsing.
 class SafeBrowsingServiceTest : public InProcessBrowserTest {
  public:
@@ -218,6 +121,27 @@ class SafeBrowsingServiceTest : public InProcessBrowserTest {
   virtual ~SafeBrowsingServiceTest() {
   }
 
+  virtual void SetUp() OVERRIDE {
+    FilePath datafile_path = FilePath(FILE_PATH_LITERAL("third_party"))
+          .Append(FILE_PATH_LITERAL("safe_browsing"))
+          .Append(FILE_PATH_LITERAL("testing"))
+          .Append(kDataFile);
+    test_server_.reset(new net::TestServer(net::TestServer::TYPE_SAFEBROWSING,
+                                           net::TestServer::kLocalhost,
+                                           datafile_path));
+    ASSERT_TRUE(test_server_->Start());
+
+    server_host_ = test_server_->host_port_pair().host();
+    server_port_ = test_server_->host_port_pair().port();
+    LOG(INFO) << "server is " << server_host_ << ":" << server_port_;
+    InProcessBrowserTest::SetUp();
+  }
+
+  virtual void TearDown() OVERRIDE {
+    InProcessBrowserTest::TearDown();
+    test_server_->Stop();
+  }
+
   void UpdateSafeBrowsingStatus() {
     ASSERT_TRUE(safe_browsing_service_);
     base::AutoLock lock(update_status_mutex_);
@@ -285,13 +209,21 @@ class SafeBrowsingServiceTest : public InProcessBrowserTest {
     return safe_browsing_service_->safe_browsing_thread_->message_loop();
   }
 
+  const std::string& server_host() const {
+    return server_host_;

[Paweł Hajdan Jr., 2012/04/17 06:38:05]

nit: Why don't you just propagate things from test_server_? Those local
server_host_ and server_port_ variables might get out of sync.

[mattm, 2012/04/18 00:20:00]

Done.

+  }
+
+  int server_port() const {
+    return server_port_;
+  }
+
  protected:
   bool InitSafeBrowsingService() {
     safe_browsing_service_ = g_browser_process->safe_browsing_service();
     return safe_browsing_service_ != NULL;
   }
 
-  virtual void SetUpCommandLine(CommandLine* command_line) {
+  virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
     // Makes sure the auto update is not triggered. This test will force the
     // update when needed.
     command_line->AppendSwitch(switches::kSbDisableAutoUpdate);
@@ -313,8 +245,8 @@ class SafeBrowsingServiceTest : public InProcessBrowserTest {
     // Point to the testing server for all SafeBrowsing requests.
     std::string url_prefix =
         base::StringPrintf("http://%s:%d/safebrowsing",
-                           SafeBrowsingTestServer::Host(),
-                           SafeBrowsingTestServer::Port());
+                           server_host_.c_str(),
+                           server_port_);
     command_line->AppendSwitchASCII(switches::kSbURLPrefix, url_prefix);
   }
 
@@ -326,6 +258,10 @@ class SafeBrowsingServiceTest : public InProcessBrowserTest {
  private:
   SafeBrowsingService* safe_browsing_service_;
 
+  scoped_ptr<net::TestServer> test_server_;
+  std::string server_host_;
+  int server_port_;
+
   // Protects all variables below since they are read on UI thread
   // but updated on IO thread or safebrowsing thread.
   base::Lock update_status_mutex_;
@@ -455,51 +391,35 @@ class SafeBrowsingServiceTestHelper
     ui_test_utils::RunMessageLoop();
   }
 
-  void WaitTillServerReady(const char* host, int port) {
-    response_status_ = net::URLRequestStatus::FAILED;
-    GURL url(base::StringPrintf("http://%s:%d%s?test_step=0",
-                                host, port, kDBResetPath));
-    // TODO(lzheng): We should have a way to reliably tell when a server is
-    // ready so we could get rid of the Sleep and retry loop.
-    while (true) {
-      if (FetchUrl(url) == net::URLRequestStatus::SUCCESS)
-        break;
-      // Wait and try again if last fetch was failed. The loop will hit the
-      // timeout in OutOfProcTestRunner if the fetch can not get success
-      // response.
-      base::PlatformThread::Sleep(TestTimeouts::tiny_timeout());
-    }
-  }
-
   // Calls test server to fetch database for verification.
-  net::URLRequestStatus::Status FetchDBToVerify(const char* host, int port,
-                                                int test_step) {
+  net::URLRequestStatus::Status FetchDBToVerify(
+      const std::string& host, int port, int test_step) {
     // TODO(lzheng): Remove chunk_type=add once it is not needed by the server.
     GURL url(base::StringPrintf(
         "http://%s:%d%s?"
         "client=chromium&appver=1.0&pver=2.2&test_step=%d&"
         "chunk_type=add",
-        host, port, kDBVerifyPath, test_step));
+        host.c_str(), port, kDBVerifyPath, test_step));
     return FetchUrl(url);
   }
 
   // Calls test server to fetch URLs for verification.
-  net::URLRequestStatus::Status FetchUrlsToVerify(const char* host, int port,
-                                                  int test_step) {
+  net::URLRequestStatus::Status FetchUrlsToVerify(
+      const std::string& host, int port, int test_step) {
     GURL url(base::StringPrintf(
         "http://%s:%d%s?"
         "client=chromium&appver=1.0&pver=2.2&test_step=%d",
-        host, port, kUrlVerifyPath, test_step));
+        host.c_str(), port, kUrlVerifyPath, test_step));
     return FetchUrl(url);
   }
 
   // Calls test server to check if test data is done. E.g.: if there is a
   // bad URL that server expects test to fetch full hash but the test didn't,
   // this verification will fail.
-  net::URLRequestStatus::Status VerifyTestComplete(const char* host, int port,
-                                                   int test_step) {
+  net::URLRequestStatus::Status VerifyTestComplete(
+      const std::string& host, int port, int test_step) {
     GURL url(StringPrintf("http://%s:%d%s?test_step=%d",
-                          host, port, kTestCompletePath, test_step));
+                          host.c_str(), port, kTestCompletePath, test_step));
     return FetchUrl(url);
   }
 
@@ -546,19 +466,11 @@ class SafeBrowsingServiceTestHelper
 IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest,
                        DISABLED_SafeBrowsingSystemTest) {
   LOG(INFO) << "Start test";
-  const char* server_host = SafeBrowsingTestServer::Host();
-  int server_port = SafeBrowsingTestServer::Port();
   ASSERT_TRUE(InitSafeBrowsingService());
 
   scoped_refptr<SafeBrowsingServiceTestHelper> safe_browsing_helper(
       new SafeBrowsingServiceTestHelper(this));
   int last_step = 0;
-  FilePath datafile_path = FilePath(kDataFile);
-  SafeBrowsingTestServer test_server(datafile_path);
-  ASSERT_TRUE(test_server.Start());
-
-  // Make sure the server is running.
-  safe_browsing_helper->WaitTillServerReady(server_host, server_port);
 
   // Waits and makes sure safebrowsing update is not happening.
   // The wait will stop once OnWaitForStatusUpdateDone in
@@ -599,8 +511,8 @@ IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest,
 
     // Fetches URLs to verify and waits till server responses with data.
     EXPECT_EQ(net::URLRequestStatus::SUCCESS,
-              safe_browsing_helper->FetchUrlsToVerify(server_host,
-                                                      server_port,
+              safe_browsing_helper->FetchUrlsToVerify(server_host(),
+                                                      server_port(),
                                                       step));
 
     std::vector<PhishingUrl> phishing_urls;
@@ -630,8 +542,8 @@ IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest,
     // TODO(lzheng): We should verify the fetched database with local
     // database to make sure they match.
     EXPECT_EQ(net::URLRequestStatus::SUCCESS,
-              safe_browsing_helper->FetchDBToVerify(server_host,
-                                                    server_port,
+              safe_browsing_helper->FetchDBToVerify(server_host(),
+                                                    server_port(),
                                                     step));
     EXPECT_GT(safe_browsing_helper->response_data().size(), 0U);
     last_step = step;
@@ -639,9 +551,8 @@ IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest,
 
   // Verifies with server if test is done and waits till server responses.
   EXPECT_EQ(net::URLRequestStatus::SUCCESS,
-            safe_browsing_helper->VerifyTestComplete(server_host,
-                                                     server_port,
+            safe_browsing_helper->VerifyTestComplete(server_host(),
+                                                     server_port(),
                                                      last_step));
   EXPECT_EQ("yes", safe_browsing_helper->response_data());
-  test_server.Stop();
 }