Clean up URLFetcher unit tests, part 5.

net/url_request/url_fetcher_impl_unittest.cc

Index: net/url_request/url_fetcher_impl_unittest.cc
diff --git a/net/url_request/url_fetcher_impl_unittest.cc b/net/url_request/url_fetcher_impl_unittest.cc
index 3bf1a369169865c82933fb01157a96f784a76972..0dea16334db77c2fbcd5e43f8f0e84a57d794006 100644
--- a/net/url_request/url_fetcher_impl_unittest.cc
+++ b/net/url_request/url_fetcher_impl_unittest.cc
@@ -200,6 +200,86 @@ class URLFetcherTest : public testing::Test,
     return num_upload_streams_created_;
   }
 
+  // Downloads |file_to_fetch| to |out_path|, and checks the contents. Takes
+  // ownership of the file if |take_ownership| is true. Caller is epected to
+  // cleanup the resulting file.
+  void SaveFileTest(const char* file_to_fetch,
+                    const base::FilePath& out_path,
+                    bool take_ownership) {
+    base::FilePath expected_file =
+        test_server_->GetDocumentRoot().AppendASCII(file_to_fetch);
+    scoped_ptr<WaitingURLFetcherDelegate> delegate(
+        new WaitingURLFetcherDelegate());
+    delegate->CreateFetcherWithContext(
+        test_server_->GetURL(std::string(kTestServerFilePrefix) +
+                             file_to_fetch),
+        URLFetcher::GET, request_context());
+    delegate->fetcher()->SaveResponseToFileAtPath(
+        out_path, scoped_refptr<base::MessageLoopProxy>(
+                      base::MessageLoopProxy::current()));
+    delegate->StartFetcherAndWait();
+
+    EXPECT_TRUE(delegate->fetcher()->GetStatus().is_success());
+    EXPECT_EQ(200, delegate->fetcher()->GetResponseCode());
+
+    base::FilePath actual_out_path;
+    EXPECT_TRUE(delegate->fetcher()->GetResponseAsFilePath(take_ownership,
+                                                           &actual_out_path));
+    EXPECT_EQ(out_path, actual_out_path);
+
+    EXPECT_TRUE(base::ContentsEqual(
+        test_server_->GetDocumentRoot().AppendASCII(file_to_fetch),
+        actual_out_path));
+
+    // Delete the delegate and run the message loop to give the fetcher's
+    // destructor a chance to delete the file.
+    delegate.reset();
+    base::RunLoop().RunUntilIdle();
+
+    // File should only exist if |take_ownership| was true.
+    EXPECT_EQ(take_ownership, base::PathExists(actual_out_path));
+  }
+
+  // Downloads |file_to_fetch| to a temporary file, and checks the contents.
+  // Takes ownership of the file if |take_ownership| is true. Cleans up the
+  // temp file when the test is complete.
+  void SaveTempFileTest(const char* file_to_fetch, bool take_ownership) {

[davidben, 2015/04/15 18:21:10]

These two functions are almost completely identical. Do you think it'd be better
to have a shared helper. Sort of weird to pass in an optional FilePath, but you
could do something like...

  SaveFileTestImpl(const char* file_to_fetch,
                   bool use_temporary,
                   const base::FilePath& out_path, // ignored if use_temporary
                   bool take_ownership);

[mmenke, 2015/04/15 18:43:41]

Done.  Considered this, but was thinking they were a little more different than
they actually were.

+    base::FilePath expected_file =
+        test_server_->GetDocumentRoot().AppendASCII(file_to_fetch);
+    scoped_ptr<WaitingURLFetcherDelegate> delegate(
+        new WaitingURLFetcherDelegate());
+    delegate->CreateFetcherWithContext(
+        test_server_->GetURL(std::string(kTestServerFilePrefix) +
+                             file_to_fetch),
+        URLFetcher::GET, request_context());
+    delegate->fetcher()->SaveResponseToTemporaryFile(
+        scoped_refptr<base::MessageLoopProxy>(
+            base::MessageLoopProxy::current()));
+    delegate->StartFetcherAndWait();
+
+    EXPECT_TRUE(delegate->fetcher()->GetStatus().is_success());
+    EXPECT_EQ(200, delegate->fetcher()->GetResponseCode());
+
+    base::FilePath out_path;
+    EXPECT_TRUE(
+        delegate->fetcher()->GetResponseAsFilePath(take_ownership, &out_path));
+
+    EXPECT_TRUE(base::ContentsEqual(
+        test_server_->GetDocumentRoot().AppendASCII(file_to_fetch), out_path));
+
+    // Delete the delegate and run the message loop to give the fetcher's
+    // destructor a chance to delete the file.
+    delegate.reset();
+    base::RunLoop().RunUntilIdle();
+
+    // File should only exist if |take_ownership| was true.
+    EXPECT_EQ(take_ownership, base::PathExists(out_path));
+
+    // Cleanup.
+    if (base::PathExists(out_path))
+      base::DeleteFile(out_path, false);
+  }
+
   // Returns a URL that hangs on DNS resolution.  Only hangs when using the
   // request context returned by request_context().
   const GURL& hanging_url() const { return hanging_url_; }
@@ -464,30 +544,6 @@ class URLFetcherMultipleAttemptTest : public URLFetcherTest {
   std::string data_;
 };
 
-class URLFetcherFileTest : public URLFetcherTest {
- public:
-  URLFetcherFileTest() : take_ownership_of_file_(false),
-                         expected_file_error_(OK) {}
-
-  void CreateFetcherForFile(const GURL& url, const base::FilePath& file_path);
-  void CreateFetcherForTempFile(const GURL& url);
-
-  // URLFetcherDelegate:
-  void OnURLFetchComplete(const URLFetcher* source) override;
-
- protected:
-  base::FilePath expected_file_;
-  base::FilePath file_path_;
-
-  // Set by the test. Used in OnURLFetchComplete() to decide if
-  // the URLFetcher should own the temp file, so that we can test
-  // disowning prevents the file from being deleted.
-  bool take_ownership_of_file_;
-
-  // Expected file error code for the test.  OK when expecting success.
-  int expected_file_error_;
-};
-
 void URLFetcherDownloadProgressTest::CreateFetcher(const GURL& url) {
   fetcher_ = new URLFetcherImpl(url, URLFetcher::GET, this);
   fetcher_->SetRequestContext(new ThrottlingTestURLRequestContextGetter(
@@ -637,44 +693,6 @@ void URLFetcherMultipleAttemptTest::OnURLFetchComplete(
   }
 }
 
-void URLFetcherFileTest::CreateFetcherForFile(const GURL& url,
-                                              const base::FilePath& file_path) {
-  fetcher_ = new URLFetcherImpl(url, URLFetcher::GET, this);
-  fetcher_->SetRequestContext(new ThrottlingTestURLRequestContextGetter(
-      io_message_loop_proxy().get(), request_context()));
-
-  // Use the IO message loop to do the file operations in this test.
-  fetcher_->SaveResponseToFileAtPath(file_path, io_message_loop_proxy());
-  fetcher_->Start();
-}
-
-void URLFetcherFileTest::CreateFetcherForTempFile(const GURL& url) {
-  fetcher_ = new URLFetcherImpl(url, URLFetcher::GET, this);
-  fetcher_->SetRequestContext(new ThrottlingTestURLRequestContextGetter(
-      io_message_loop_proxy().get(), request_context()));
-
-  // Use the IO message loop to do the file operations in this test.
-  fetcher_->SaveResponseToTemporaryFile(io_message_loop_proxy());
-  fetcher_->Start();
-}
-
-void URLFetcherFileTest::OnURLFetchComplete(const URLFetcher* source) {
-  if (expected_file_error_ == OK) {
-    EXPECT_TRUE(source->GetStatus().is_success());
-    EXPECT_EQ(OK, source->GetStatus().error());
-    EXPECT_EQ(200, source->GetResponseCode());
-
-    EXPECT_TRUE(source->GetResponseAsFilePath(
-        take_ownership_of_file_, &file_path_));
-
-    EXPECT_TRUE(base::ContentsEqual(expected_file_, file_path_));
-  } else {
-    EXPECT_FALSE(source->GetStatus().is_success());
-    EXPECT_EQ(expected_file_error_, source->GetStatus().error());
-  }
-  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.
@@ -1202,149 +1220,93 @@ TEST_F(URLFetcherMultipleAttemptTest, SameData) {
   base::MessageLoop::current()->Run();
 }
 
-TEST_F(URLFetcherFileTest, SmallGet) {
+// Get a small file.
+TEST_F(URLFetcherTest, FileTestSmallGet) {
+  const char kFileToFetch[] = "simple.html";
+
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
-
-  // Get a small file.
-  static const char kFileToFetch[] = "simple.html";
-  expected_file_ = test_server_->GetDocumentRoot().AppendASCII(kFileToFetch);
-  CreateFetcherForFile(
-      test_server_->GetURL(std::string(kTestServerFilePrefix) + kFileToFetch),
-      temp_dir.path().AppendASCII(kFileToFetch));
-
-  base::MessageLoop::current()->Run();  // OnURLFetchComplete() will Quit().
-
-  ASSERT_FALSE(base::PathExists(file_path_))
-      << file_path_.value() << " not removed.";
+  base::FilePath out_path = temp_dir.path().AppendASCII(kFileToFetch);
+  SaveFileTest(kFileToFetch, out_path, false);
 }
 
-TEST_F(URLFetcherFileTest, LargeGet) {
+// Get a file large enough to require more than one read into URLFetcher::Core's
+// IOBuffer.
+TEST_F(URLFetcherTest, FileTestLargeGet) {
+  const char kFileToFetch[] = "animate1.gif";
+
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
-
-  // Get a file large enough to require more than one read into
-  // URLFetcher::Core's IOBuffer.
-  static const char kFileToFetch[] = "animate1.gif";
-  expected_file_ = test_server_->GetDocumentRoot().AppendASCII(kFileToFetch);
-  CreateFetcherForFile(
-      test_server_->GetURL(std::string(kTestServerFilePrefix) + kFileToFetch),
-      temp_dir.path().AppendASCII(kFileToFetch));
-
-  base::MessageLoop::current()->Run();  // OnURLFetchComplete() will Quit().
+  base::FilePath out_path = temp_dir.path().AppendASCII(kFileToFetch);
+  SaveFileTest(kFileToFetch, out_path, false);
 }
 
-TEST_F(URLFetcherFileTest, SavedOutputFileOwnerhisp) {
-  // If the caller takes the ownership of the output file, the file should
-  // persist even after URLFetcher is gone. If not, the file must be deleted.
-  const bool kTake[] = {false, true};
-  for (size_t i = 0; i < arraysize(kTake); ++i) {
-    take_ownership_of_file_ = kTake[i];
-    base::ScopedTempDir temp_dir;
-    ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
-
-    // Get a small file.
-    static const char kFileToFetch[] = "simple.html";
-    expected_file_ = test_server_->GetDocumentRoot().AppendASCII(kFileToFetch);
-    CreateFetcherForFile(
-        test_server_->GetURL(std::string(kTestServerFilePrefix) + kFileToFetch),
-        temp_dir.path().AppendASCII(kFileToFetch));
-
-    base::MessageLoop::current()->Run();  // OnURLFetchComplete() will Quit().
-
-    base::MessageLoop::current()->RunUntilIdle();
-    ASSERT_EQ(kTake[i], base::PathExists(file_path_)) <<
-        "FilePath: " << file_path_.value();
-  }
+// If the caller takes the ownership of the output file, the file should persist
+// even after URLFetcher is gone.
+TEST_F(URLFetcherTest, FileTestTakeOwnership) {
+  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, out_path, true);
 }
 
-TEST_F(URLFetcherFileTest, OverwriteExistingFile) {
+// Test that an existing file can be overwritten be a fetcher.
+TEST_F(URLFetcherTest, FileTestOverwriteExisting) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
   // Create a file before trying to fetch.
-  static const char kFileToFetch[] = "simple.html";
+  const char kFileToFetch[] = "simple.html";
   std::string data(10000, '?');  // Meant to be larger than simple.html.
-  file_path_ = temp_dir.path().AppendASCII(kFileToFetch);
+  base::FilePath out_path = temp_dir.path().AppendASCII(kFileToFetch);
   ASSERT_EQ(static_cast<int>(data.size()),
-            base::WriteFile(file_path_, data.data(), data.size()));
-  ASSERT_TRUE(base::PathExists(file_path_));
-  expected_file_ = test_server_->GetDocumentRoot().AppendASCII(kFileToFetch);
-  ASSERT_FALSE(base::ContentsEqual(file_path_, expected_file_));
-
-  // Get a small file.
-  CreateFetcherForFile(
-      test_server_->GetURL(std::string(kTestServerFilePrefix) + kFileToFetch),
-      file_path_);
+            base::WriteFile(out_path, data.data(), data.size()));
+  ASSERT_TRUE(base::PathExists(out_path));
 
-  base::MessageLoop::current()->Run();  // OnURLFetchComplete() will Quit().
+  SaveFileTest(kFileToFetch, out_path, true);

[davidben, 2015/04/15 18:21:10]

Not from this CL, but take_ownership = false + existing file seems a very weird
combination. Seems a better API would have been for take_ownership to require
SaveResponseToTemporaryFile rather than SaveResponseToFilePathAtPath.

[mmenke, 2015/04/15 18:43:41]

Yea, it does seem weird.  Not sure it's worth fixing, though.

[davidben, 2015/04/15 18:49:28]

Yeah, certainly not in this CL.

 }
 
-TEST_F(URLFetcherFileTest, TryToOverwriteDirectory) {
+// Test trying to overwrite a directory with a file when using a fetcher fails.
+TEST_F(URLFetcherTest, FileTestTryToOverwriteDirectory) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
   // Create a directory before trying to fetch.
   static const char kFileToFetch[] = "simple.html";
-  file_path_ = temp_dir.path().AppendASCII(kFileToFetch);
-  ASSERT_TRUE(base::CreateDirectory(file_path_));
-  ASSERT_TRUE(base::PathExists(file_path_));
-
-  // Get a small file.
-  expected_file_error_ = ERR_ACCESS_DENIED;
-  expected_file_ = test_server_->GetDocumentRoot().AppendASCII(kFileToFetch);
-  CreateFetcherForFile(
-      test_server_->GetURL(std::string(kTestServerFilePrefix) + kFileToFetch),
-      file_path_);
+  base::FilePath out_path = temp_dir.path().AppendASCII(kFileToFetch);
+  ASSERT_TRUE(base::CreateDirectory(out_path));
+  ASSERT_TRUE(base::PathExists(out_path));
 
-  base::MessageLoop::current()->Run();  // OnURLFetchComplete() will Quit().
+  WaitingURLFetcherDelegate delegate;
+  delegate.CreateFetcherWithContext(
+      test_server_->GetURL(std::string(kTestServerFilePrefix) + kFileToFetch),
+      URLFetcher::GET, request_context());
+  delegate.fetcher()->SaveResponseToFileAtPath(
+      out_path,
+      scoped_refptr<base::MessageLoopProxy>(base::MessageLoopProxy::current()));
+  delegate.StartFetcherAndWait();
 
-  base::MessageLoop::current()->RunUntilIdle();
+  EXPECT_FALSE(delegate.fetcher()->GetStatus().is_success());
+  EXPECT_EQ(ERR_ACCESS_DENIED, delegate.fetcher()->GetStatus().error());
 }
 
-TEST_F(URLFetcherFileTest, SmallGetToTempFile) {
-  // Get a small file.
-  static const char kFileToFetch[] = "simple.html";
-  expected_file_ = test_server_->GetDocumentRoot().AppendASCII(kFileToFetch);
-  CreateFetcherForTempFile(
-      test_server_->GetURL(std::string(kTestServerFilePrefix) + kFileToFetch));
-
-  base::MessageLoop::current()->Run();  // OnURLFetchComplete() will Quit().
-
-  ASSERT_FALSE(base::PathExists(file_path_))
-      << file_path_.value() << " not removed.";
+// Get a small file and save it to a temp file.
+TEST_F(URLFetcherTest, TempFileTestSmallGet) {
+  SaveTempFileTest("simple.html", false);
 }
 
-TEST_F(URLFetcherFileTest, LargeGetToTempFile) {
-  // Get a file large enough to require more than one read into
-  // URLFetcher::Core's IOBuffer.
-  static const char kFileToFetch[] = "animate1.gif";
-  expected_file_ = test_server_->GetDocumentRoot().AppendASCII(kFileToFetch);
-  CreateFetcherForTempFile(
-      test_server_->GetURL(std::string(kTestServerFilePrefix) + kFileToFetch));
-
-  base::MessageLoop::current()->Run();  // OnURLFetchComplete() will Quit().
+// Get a file large enough to require more than one read into URLFetcher::Core's
+// IOBuffer and save it to a temp file.
+TEST_F(URLFetcherTest, TempFileTestLargeGet) {
+  SaveTempFileTest("animate1.gif", false);
 }
 
-TEST_F(URLFetcherFileTest, SavedOutputTempFileOwnerhisp) {
-  // If the caller takes the ownership of the temp file, the file should persist
-  // even after URLFetcher is gone. If not, the file must be deleted.
-  const bool kTake[] = {false, true};
-  for (size_t i = 0; i < arraysize(kTake); ++i) {
-    take_ownership_of_file_ = kTake[i];
-
-    // Get a small file.
-    static const char kFileToFetch[] = "simple.html";
-    expected_file_ = test_server_->GetDocumentRoot().AppendASCII(kFileToFetch);
-    CreateFetcherForTempFile(test_server_->GetURL(
-        std::string(kTestServerFilePrefix) + kFileToFetch));
-
-    base::MessageLoop::current()->Run();  // OnURLFetchComplete() will Quit().
-
-    base::MessageLoop::current()->RunUntilIdle();
-    ASSERT_EQ(kTake[i], base::PathExists(file_path_)) <<
-        "FilePath: " << file_path_.value();
-  }
+// If the caller takes the ownership of the temp file, check that the file
+// persists even after URLFetcher is gone.
+TEST_F(URLFetcherTest, TempFileTestTakeOwnership) {
+  SaveTempFileTest("simple.html", true);
 }
 
 }  // namespace