Clean up URLFetcher unit tests, part 7.

net/url_request/url_fetcher_impl_unittest.cc

 // Copyright (c) 2012 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 "net/url_request/url_fetcher_impl.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/bind.h"
@@ skipping 388 matching lines @@
   void OnURLFetchUploadProgress(const URLFetcher* source,
                                 int64 current,
                                 int64 total) override;
 
  protected:
   int64 previous_progress_;
   std::string chunk_;
   int64 number_of_chunks_added_;
 };
 
-// Version of URLFetcherTest that tests bad HTTPS requests.
-class URLFetcherBadHTTPSTest : public URLFetcherTest {
- public:
-  URLFetcherBadHTTPSTest();
-
-  // URLFetcherTest:
-  void SetUpServer() override;
-
-  // URLFetcherDelegate:
-  void OnURLFetchComplete(const URLFetcher* source) override;
-
- private:
-  base::FilePath cert_dir_;
-};
-
 // Version of URLFetcherTest that tests request cancellation on shutdown.
 class URLFetcherCancelTest : public URLFetcherTest {
  public:
   // URLFetcherTest:
   void CreateFetcher(const GURL& url) override;
 
   // URLFetcherDelegate:
   void OnURLFetchComplete(const URLFetcher* source) override;
 
   void CancelRequest();
@@ skipping 65 matching lines @@
  protected:
   ~CancelTestURLRequestContextGetter() override {}
 
  private:
   scoped_ptr<TestURLRequestContext> context_;
   scoped_refptr<base::MessageLoopProxy> io_message_loop_proxy_;
   base::WaitableEvent context_created_;
   GURL throttle_for_url_;
 };
 
-// Version of URLFetcherTest that tests retying the same request twice.
-class URLFetcherMultipleAttemptTest : public URLFetcherTest {
+// Version of URLFetcherTest that tests bad HTTPS requests.
+class URLFetcherBadHTTPSTest : public URLFetcherTest {

[mmenke, 2015/04/16 19:33:28]

Moved this test fixture (Which we have to keep) and its test to the bottom, so
order is consistent.  We'll be deleting the other fixtures, anyways, of course.

  public:
-  // URLFetcherDelegate:
-  void OnURLFetchComplete(const URLFetcher* source) override;
+  URLFetcherBadHTTPSTest() {}
 
- private:
-  std::string data_;
+  // URLFetcherTest:
+  void SetUpServer() override {
+    SpawnedTestServer::SSLOptions ssl_options(
+        SpawnedTestServer::SSLOptions::CERT_EXPIRED);
+    test_server_.reset(new SpawnedTestServer(
+        SpawnedTestServer::TYPE_HTTPS, ssl_options, base::FilePath(kDocRoot)));
+  }
 };
 
 void URLFetcherDownloadProgressTest::CreateFetcher(const GURL& url) {
   fetcher_ = new URLFetcherImpl(url, URLFetcher::GET, this);
   fetcher_->SetRequestContext(new ThrottlingTestURLRequestContextGetter(
       io_message_loop_proxy().get(), request_context()));
   fetcher_->Start();
 }
 
 void URLFetcherDownloadProgressTest::OnURLFetchDownloadProgress(
@@ skipping 53 matching lines @@
   EXPECT_LE(previous_progress_, current);
   previous_progress_ = current;
   EXPECT_EQ(-1, total);
 
   if (number_of_chunks_added_ < 2) {
     number_of_chunks_added_ += 1;
     fetcher_->AppendChunkToUpload(chunk_, true);
   }
 }
 
-URLFetcherBadHTTPSTest::URLFetcherBadHTTPSTest() {
-  PathService::Get(base::DIR_SOURCE_ROOT, &cert_dir_);
-  cert_dir_ = cert_dir_.AppendASCII("chrome");
-  cert_dir_ = cert_dir_.AppendASCII("test");
-  cert_dir_ = cert_dir_.AppendASCII("data");
-  cert_dir_ = cert_dir_.AppendASCII("ssl");
-  cert_dir_ = cert_dir_.AppendASCII("certificates");
-}
-
-void URLFetcherBadHTTPSTest::SetUpServer() {
-  SpawnedTestServer::SSLOptions ssl_options(
-      SpawnedTestServer::SSLOptions::CERT_EXPIRED);
-  test_server_.reset(new SpawnedTestServer(
-      SpawnedTestServer::TYPE_HTTPS, ssl_options, base::FilePath(kDocRoot)));
-}
-
-// The "server certificate expired" error should result in automatic
-// cancellation of the request by
-// URLRequest::Delegate::OnSSLCertificateError.
-void URLFetcherBadHTTPSTest::OnURLFetchComplete(
-    const URLFetcher* source) {
-  // This part is different from URLFetcherTest::OnURLFetchComplete
-  // because this test expects the request to be cancelled.
-  EXPECT_EQ(URLRequestStatus::CANCELED, source->GetStatus().status());
-  EXPECT_EQ(ERR_ABORTED, source->GetStatus().error());
-  EXPECT_EQ(-1, source->GetResponseCode());
-  EXPECT_TRUE(source->GetCookies().empty());
-  std::string data;
-  EXPECT_TRUE(source->GetResponseAsString(&data));
-  EXPECT_TRUE(data.empty());
-  CleanupAfterFetchComplete();
-}
-
 void URLFetcherCancelTest::CreateFetcher(const GURL& url) {
   fetcher_ = new URLFetcherImpl(url, URLFetcher::GET, this);
   CancelTestURLRequestContextGetter* context_getter =
       new CancelTestURLRequestContextGetter(io_message_loop_proxy().get(), url);
   fetcher_->SetRequestContext(context_getter);
   fetcher_->SetMaxRetriesOn5xx(2);
   fetcher_->Start();
   // We need to wait for the creation of the URLRequestContext, since we
   // rely on it being destroyed as a signal to end the test.
   context_getter->WaitForContextCreation();
   CancelRequest();
 }
 
 void URLFetcherCancelTest::OnURLFetchComplete(
     const URLFetcher* source) {
   // We should have cancelled the request before completion.
   ADD_FAILURE();
   CleanupAfterFetchComplete();
 }
 
 void URLFetcherCancelTest::CancelRequest() {
   delete fetcher_;
   // The URLFetcher's test context will post a Quit task once it is
   // deleted. So if this test simply hangs, it means cancellation
   // did not work.
 }
 
-void URLFetcherMultipleAttemptTest::OnURLFetchComplete(
-    const URLFetcher* source) {
-  EXPECT_TRUE(source->GetStatus().is_success());
-  EXPECT_EQ(200, source->GetResponseCode());  // HTTP OK
-  std::string data;
-  EXPECT_TRUE(source->GetResponseAsString(&data));
-  EXPECT_FALSE(data.empty());
-  if (!data.empty() && data_.empty()) {
-    data_ = data;
-    fetcher_->SetRequestContext(new ThrottlingTestURLRequestContextGetter(
-        io_message_loop_proxy().get(), request_context()));
-    fetcher_->Start();
-  } else {
-    EXPECT_EQ(data, data_);
-    CleanupAfterFetchComplete();
-  }
-}
-
 // Create the fetcher on the main thread.  Since network IO will happen on the
 // main thread, this will test URLFetcher's ability to do everything on one
 // thread.
 TEST_F(URLFetcherTest, SameThreadTest) {
   WaitingURLFetcherDelegate delegate;
   delegate.CreateFetcherWithContext(test_server_->GetURL(kDefaultResponsePath),
                                     URLFetcher::GET, request_context());
   delegate.StartFetcherAndWait();
 
   EXPECT_TRUE(delegate.fetcher()->GetStatus().is_success());
@@ skipping 442 matching lines @@
   std::string data;
   ASSERT_TRUE(delegate.fetcher()->GetResponseAsString(&data));
   EXPECT_FALSE(data.empty());
   EXPECT_GT(delegate.fetcher()->GetBackoffDelay().InMicroseconds(), 0);
 
   // The request should not have been retried at all.  If it had attempted all
   // 11 retries, that should have taken 2.5 minutes.
   EXPECT_TRUE(Time::Now() - start_time < TimeDelta::FromMinutes(1));
 }
 
-TEST_F(URLFetcherBadHTTPSTest, BadHTTPSTest) {
-  CreateFetcher(test_server_->GetURL(kDefaultResponsePath));
-  base::MessageLoop::current()->Run();
-}
-
 TEST_F(URLFetcherCancelTest, ReleasesContext) {
   GURL url(test_server_->GetURL("files/server-unavailable.html"));
 
   // Create a separate thread that will create the URLFetcher.  The current
   // (main) thread will do the IO, and when the fetch is complete it will
   // terminate the main thread's message loop; then the other thread's
   // message loop will be shut down automatically as the thread goes out of
   // scope.
   base::Thread t("URLFetcher test thread");
   ASSERT_TRUE(t.Start());
@@ skipping 31 matching lines @@
 
   base::Thread t("URLFetcher test thread");
   ASSERT_TRUE(t.Start());
   t.message_loop()->PostTask(
       FROM_HERE,
       base::Bind(&URLFetcherTest::CreateFetcher, base::Unretained(this), url));
 
   base::MessageLoop::current()->Run();
 }
 
-TEST_F(URLFetcherMultipleAttemptTest, SameData) {
-  // Create the fetcher on the main thread.  Since IO will happen on the main
-  // thread, this will test URLFetcher's ability to do everything on one
-  // thread.
-  CreateFetcher(test_server_->GetURL(kDefaultResponsePath));
+// A URLFetcherDelegate that expects to receive a response body of "request1"
+// and then reuses the fetcher for the same URL, setting the "test" request
+// header to "request2".
+class ReuseFetcherDelegate : public WaitingURLFetcherDelegate {
+ public:
+  // |second_request_context_getter| is the context getter used for the second
+  // request. Can't reuse the old one because fetchers release it on completion.

[davidben, 2015/04/16 21:48:39]

Yuck. :-P I don't have a much better suggestion though.

+  ReuseFetcherDelegate(
+      scoped_refptr<URLRequestContextGetter> second_request_context_getter)
+      : first_request_complete_(false),
+        second_request_context_getter_(second_request_context_getter) {}
 
-  base::MessageLoop::current()->Run();
+  ~ReuseFetcherDelegate() override {}
+
+  void OnURLFetchComplete(const URLFetcher* source) override {
+    EXPECT_EQ(fetcher(), source);
+    if (!first_request_complete_) {
+      first_request_complete_ = true;
+      EXPECT_TRUE(fetcher()->GetStatus().is_success());
+      EXPECT_EQ(200, fetcher()->GetResponseCode());
+      std::string data;
+      ASSERT_TRUE(fetcher()->GetResponseAsString(&data));
+      EXPECT_EQ("request1", data);
+
+      fetcher()->SetRequestContext(second_request_context_getter_.get());
+      fetcher()->SetExtraRequestHeaders("test: request2");
+      fetcher()->Start();
+      return;
+    }
+    WaitingURLFetcherDelegate::OnURLFetchComplete(source);
+  }
+
+ private:
+  bool first_request_complete_;
+  scoped_refptr<URLRequestContextGetter> second_request_context_getter_;
+
+  DISALLOW_COPY_AND_ASSIGN(ReuseFetcherDelegate);
+};
+
+TEST_F(URLFetcherTest, ReuseFetcherForSameURL) {
+  // TODO(mmenke):  It's really weird that this is supported, particularly
+  // some fields can be modified between requests, but some (Like upload body)
+  // cannot be. Can we get rid of support for this?

[davidben, 2015/04/16 21:48:39]

Especially weird that you have to specify a new getter. What is this API...

The tests were added in http://codereview.chromium.org/5322015 which doesn't say
much about consumers who do this.

[mmenke, 2015/04/16 21:56:29]

I think the intent is to prevent the fetcher from extending the life of the
getter.  Some getters own their request contexts, so that may have made sense,
if we didn't allow reuse.

+  ReuseFetcherDelegate delegate(new TrivialURLRequestContextGetter(
+      request_context(), base::MessageLoopProxy::current()));
+  delegate.CreateFetcherWithContext(test_server_->GetURL("echoheader?test"),
+                                    URLFetcher::GET, request_context());
+  delegate.fetcher()->SetExtraRequestHeaders("test: request1");
+  delegate.StartFetcherAndWait();
+
+  EXPECT_TRUE(delegate.fetcher()->GetStatus().is_success());
+  EXPECT_EQ(200, delegate.fetcher()->GetResponseCode());
+  std::string data;
+  ASSERT_TRUE(delegate.fetcher()->GetResponseAsString(&data));
+  EXPECT_EQ("request2", data);
 }
 
 // Get a small file.
 TEST_F(URLFetcherTest, FileTestSmallGet) {
   const char kFileToFetch[] = "simple.html";
 
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
   base::FilePath out_path = temp_dir.path().AppendASCII(kFileToFetch);
   SaveFileTest(kFileToFetch, false, out_path, false);
@@ skipping 71 matching lines @@
 TEST_F(URLFetcherTest, TempFileTestLargeGet) {
   SaveFileTest("animate1.gif", true, base::FilePath(), false);
 }
 
 // If the caller takes the ownership of the temp file, check that the file
 // persists even after URLFetcher is gone.
 TEST_F(URLFetcherTest, TempFileTestTakeOwnership) {
   SaveFileTest("simple.html", true, base::FilePath(), true);
 }
 
+TEST_F(URLFetcherBadHTTPSTest, BadHTTPS) {
+  WaitingURLFetcherDelegate delegate;
+  delegate.CreateFetcherWithContext(test_server_->GetURL(kDefaultResponsePath),
+                                    URLFetcher::GET, request_context());
+  delegate.StartFetcherAndWait();
+
+  EXPECT_EQ(URLRequestStatus::CANCELED,
+            delegate.fetcher()->GetStatus().status());
+  EXPECT_EQ(ERR_ABORTED, delegate.fetcher()->GetStatus().error());
+  EXPECT_EQ(-1, delegate.fetcher()->GetResponseCode());
+  EXPECT_TRUE(delegate.fetcher()->GetCookies().empty());
+  std::string data;
+  EXPECT_TRUE(delegate.fetcher()->GetResponseAsString(&data));
+  EXPECT_TRUE(data.empty());
+}
+
 }  // namespace
 
 }  // namespace net