Fix the failed cases in URLFetcherFileTest on Android.

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 a94e8870489f8d40f3b8d64c322adb53d3ece7d1..7f21566d978e18cf9c9988dcaf12ccd1e6c9daf2 100644
--- a/net/url_request/url_fetcher_impl_unittest.cc
+++ b/net/url_request/url_fetcher_impl_unittest.cc
@@ -381,7 +381,12 @@ class URLFetcherMultipleAttemptTest : public URLFetcherTest {
 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);
@@ -390,6 +395,7 @@ class URLFetcherFileTest : public URLFetcherTest {
   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
 
  protected:
+  FilePath document_root_;
   FilePath expected_file_;
   FilePath file_path_;
 
@@ -1087,7 +1093,7 @@ TEST_F(URLFetcherFileTest, SmallGet) {
 
   // 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));
@@ -1110,7 +1116,7 @@ TEST_F(URLFetcherFileTest, LargeGet) {
   // 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));
@@ -1129,7 +1135,7 @@ TEST_F(URLFetcherFileTest, CanTakeOwnershipOfFile) {
 
   // 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));
@@ -1158,7 +1164,7 @@ TEST_F(URLFetcherFileTest, OverwriteExistingFile) {
   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.
@@ -1204,7 +1210,7 @@ TEST_F(URLFetcherFileTest, SmallGetToTempFile) {
 
   // 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));
 
@@ -1223,7 +1229,7 @@ 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.document_root().AppendASCII(kFileToFetch);
+  expected_file_ = document_root_.AppendASCII(kFileToFetch);
   CreateFetcherForTempFile(test_server.GetURL(
       std::string(kTestServerFilePrefix) + kFileToFetch));
 
@@ -1238,7 +1244,7 @@ TEST_F(URLFetcherFileTest, CanTakeOwnershipOfTempFile) {
 
   // 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));