Fix the failed cases in URLFetcherFileTest on Android.

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 <string>
 
 #include "base/bind.h"
 #include "base/file_util.h"
@@ skipping 363 matching lines @@
  public:
   // URLFetcherDelegate
   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
  private:
   std::string data_;
 };
 
 class URLFetcherFileTest : public URLFetcherTest {
  public:
   URLFetcherFileTest() : take_ownership_of_file_(false),
-                         expected_file_error_(base::PLATFORM_FILE_OK) {}
+                         expected_file_error_(base::PLATFORM_FILE_OK) {
+    PathService::Get(base::DIR_SOURCE_ROOT, &document_root_);
+    document_root_= document_root_.AppendASCII(FILE_PATH_LITERAL("chrome"))
+                                  .AppendASCII(FILE_PATH_LITERAL("test"))
+                                  .AppendASCII(FILE_PATH_LITERAL("data"));

[wtc, 2012/09/26 23:48:20]

Instead of hardcoding the strings "chrome", "test", and "data,
this should use the constant kDocRoot.

Also, the document root should ideally be computed locally
in each unit test that needs it. I will explain why below.

[Shouqun Liu, 2012/09/27 01:24:51]

yes, agree,  Android is a special case. 
On 2012/09/26 23:48:20, wtc wrote:

+  }
 
   void CreateFetcherForFile(const GURL& url, const FilePath& file_path);
   void CreateFetcherForTempFile(const GURL& url);
 
   // URLFetcherDelegate
   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
 
  protected:
+  FilePath document_root_;
   FilePath expected_file_;
   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.
   // PLATFORM_FILE_OK when expecting success.
@@ skipping 669 matching lines @@
   // thread, this will test URLFetcher's ability to do everything on one
   // thread.
   CreateFetcher(test_server.GetURL("defaultresponse"));
 
   MessageLoop::current()->Run();
 }
 
 TEST_F(URLFetcherFileTest, SmallGet) {
   TestServer test_server(TestServer::TYPE_HTTP,
                          TestServer::kLocalhost,
                          FilePath(kDocRoot));

[wtc, 2012/09/28 21:43:13]

Perhaps we can pass document_root_ or something based on
document_root_ here, instead of FilePath(kDocRoot).

I'd like to make it easy to verify that the test_server
and the unit test are using the same document root.

   ASSERT_TRUE(test_server.Start());
 
   ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
   // Get a small file.
   static const char kFileToFetch[] = "simple.html";
-  expected_file_ = test_server.document_root().AppendASCII(kFileToFetch);
+  expected_file_ = document_root_.AppendASCII(kFileToFetch);

[wtc, 2012/09/26 23:48:20]

This unit test is a representative example.

On line 1088, we pass kDocRoot to the |test_server| constructor.

Ideally, here we should use kDocRoot explicitly, to make
it clear that the |test_server| and this unit test are
using the same document root.

In the current patch, the fact that this unit test is
using kDocRoot is hidden in the URLFetcherFileTest
constructor. It is not easy to verify that both the
|test_server| and this unit test are using the same
document root.

If all the |test_server| instances in this file use
the same document root (kDocRoot), then it is fine to
just set up document_root_ in the URLFetcherFileTest
constructor.

[Shouqun Liu, 2012/09/27 01:24:51]

Yes, for general cases, I think we should use  test_server.document_root() to
ensure that the document roots are the same. 

But for Android, the test cases are running on the device, and
test_server.document_root() does not return the correct path for Android unit
test to access.  Sp before cases are running,  I need to copy the files in doc
root to devices  and point the correct document_root in the test case.

Maybe using "#if defined(OS_ANDROID)" to distinguish the difference of Android
and other platform is better?    E.g. in other platforms, still using the
test_server.document_root() to fetch docRoot, and in Android use document_root_:

#if defined(OS_ANDROID)
  expected_file_ = document_root_.AppendASCII(kFileToFetch);
#else
  expected_file_ = test_server.document_root().AppendASCII(kFileToFetch);
#endif

Do you think it's OK?  I'll submit a new patchset.

On 2012/09/26 23:48:20, wtc  wrote:

[wtc, 2012/09/28 21:43:13]

Hi Shouqun,

Thank you for the explanation. We should avoid duplicating
this ifdef in multiple tests.

[Shouqun Liu, 2012/09/29 06:16:07]

Yes,  thanks for your suggestion, I have refined the patch by avoiding
duplicating #ifdef in multiple tests, please review the patchset #5 :-)

On 2012/09/28 21:43:13, wtc wrote:

   CreateFetcherForFile(
       test_server.GetURL(std::string(kTestServerFilePrefix) + kFileToFetch),
       temp_dir.path().AppendASCII(kFileToFetch));
 
   MessageLoop::current()->Run();  // OnURLFetchComplete() will Quit().
 
   ASSERT_FALSE(file_util::PathExists(file_path_))
       << file_path_.value() << " not removed.";
 }
 
 TEST_F(URLFetcherFileTest, LargeGet) {
   TestServer test_server(TestServer::TYPE_HTTP,
                          TestServer::kLocalhost,
                          FilePath(kDocRoot));
   ASSERT_TRUE(test_server.Start());
 
   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.document_root().AppendASCII(kFileToFetch);
+  expected_file_ = document_root_.AppendASCII(kFileToFetch);
   CreateFetcherForFile(
       test_server.GetURL(std::string(kTestServerFilePrefix) + kFileToFetch),
       temp_dir.path().AppendASCII(kFileToFetch));
 
   MessageLoop::current()->Run();  // OnURLFetchComplete() will Quit().
 }
 
 TEST_F(URLFetcherFileTest, CanTakeOwnershipOfFile) {
   TestServer test_server(TestServer::TYPE_HTTP,
                          TestServer::kLocalhost,
                          FilePath(kDocRoot));
   ASSERT_TRUE(test_server.Start());
 
   ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
   // Get a small file.
   static const char kFileToFetch[] = "simple.html";
-  expected_file_ = test_server.document_root().AppendASCII(kFileToFetch);
+  expected_file_ = document_root_.AppendASCII(kFileToFetch);
   CreateFetcherForFile(
       test_server.GetURL(std::string(kTestServerFilePrefix) + kFileToFetch),
       temp_dir.path().AppendASCII(kFileToFetch));
 
   MessageLoop::current()->Run();  // OnURLFetchComplete() will Quit().
 
   MessageLoop::current()->RunAllPending();
   ASSERT_FALSE(file_util::PathExists(file_path_))
       << file_path_.value() << " not removed.";
 }
 
 
 TEST_F(URLFetcherFileTest, OverwriteExistingFile) {
   TestServer test_server(TestServer::TYPE_HTTP,
                          TestServer::kLocalhost,
                          FilePath(kDocRoot));
   ASSERT_TRUE(test_server.Start());
 
   ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
   // Create a file before trying to fetch.
   static const char kFileToFetch[] = "simple.html";
   static const char kData[] = "abcdefghijklmnopqrstuvwxyz";
   file_path_ = temp_dir.path().AppendASCII(kFileToFetch);
   const int data_size = arraysize(kData);
   ASSERT_EQ(file_util::WriteFile(file_path_, kData, data_size), data_size);
   ASSERT_TRUE(file_util::PathExists(file_path_));
-  expected_file_ = test_server.document_root().AppendASCII(kFileToFetch);
+  expected_file_ = document_root_.AppendASCII(kFileToFetch);
   ASSERT_FALSE(file_util::ContentsEqual(file_path_, expected_file_));
 
   // Get a small file.
   CreateFetcherForFile(
       test_server.GetURL(std::string(kTestServerFilePrefix) + kFileToFetch),
       file_path_);
 
   MessageLoop::current()->Run();  // OnURLFetchComplete() will Quit().
 }
 
@@ skipping 25 matching lines @@
 }
 
 TEST_F(URLFetcherFileTest, SmallGetToTempFile) {
   TestServer test_server(TestServer::TYPE_HTTP,
                          TestServer::kLocalhost,
                          FilePath(kDocRoot));
   ASSERT_TRUE(test_server.Start());
 
   // Get a small file.
   static const char kFileToFetch[] = "simple.html";
-  expected_file_ = test_server.document_root().AppendASCII(kFileToFetch);
+  expected_file_ = document_root_.AppendASCII(kFileToFetch);
   CreateFetcherForTempFile(
       test_server.GetURL(std::string(kTestServerFilePrefix) + kFileToFetch));
 
   MessageLoop::current()->Run();  // OnURLFetchComplete() will Quit().
 
   ASSERT_FALSE(file_util::PathExists(file_path_))
       << file_path_.value() << " not removed.";
 }
 
 TEST_F(URLFetcherFileTest, LargeGetToTempFile) {
   TestServer test_server(TestServer::TYPE_HTTP,
                          TestServer::kLocalhost,
                          FilePath(kDocRoot));
   ASSERT_TRUE(test_server.Start());
 
   // 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.document_root().AppendASCII(kFileToFetch);
+  expected_file_ = document_root_.AppendASCII(kFileToFetch);
   CreateFetcherForTempFile(test_server.GetURL(
       std::string(kTestServerFilePrefix) + kFileToFetch));
 
   MessageLoop::current()->Run();  // OnURLFetchComplete() will Quit().
 }
 
 TEST_F(URLFetcherFileTest, CanTakeOwnershipOfTempFile) {
   TestServer test_server(TestServer::TYPE_HTTP,
                          TestServer::kLocalhost,
                          FilePath(kDocRoot));
   ASSERT_TRUE(test_server.Start());
 
   // Get a small file.
   static const char kFileToFetch[] = "simple.html";
-  expected_file_ = test_server.document_root().AppendASCII(kFileToFetch);
+  expected_file_ = document_root_.AppendASCII(kFileToFetch);
   CreateFetcherForTempFile(test_server.GetURL(
       std::string(kTestServerFilePrefix) + kFileToFetch));
 
   MessageLoop::current()->Run();  // OnURLFetchComplete() will Quit().
 
   MessageLoop::current()->RunAllPending();
   ASSERT_FALSE(file_util::PathExists(file_path_))
       << file_path_.value() << " not removed.";
 }
 
 }  // namespace
 
 }  // namespace net