Fix the failed cases in URLFetcherFileTest on Android.

Fix the failed cases in URLFetcherFileTest on Android.

* Push the needed data files to target device.
* Set the correct document root.

BUG=
TEST=net_unittests --gtest_filter=URLFetcherFileTest.*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164061

[Shouqun Liu, 2012-09-26 05:50:57]

Fix the failed cases in URLFetcherFileTest by pushing missing files and setting
correct document root. (manually switched _TestSuiteRequiresMockTestServer in
SingleTestRunner to return True). Please review.

[Isaac (away), 2012-09-26 05:53:15]

should these tests be re-enabled?  That list is in build/android/gtest_filter

[Shouqun Liu, 2012-09-26 06:01:21]

On 2012/09/26 05:53:15, Isaac wrote:
> should these tests be re-enabled?  That list is in build/android/gtest_filter
I let the SingleTestRunner._TestSuiteRequiresMockTestServer to return True
manually, then the test server will be launched.    But from the comment, Yaron
said "Disabled because of flakiness" and let it return False. In such case,
these cases that require test server  will still be failed on bot.    
I am not sure whether it's OK to turn on _TestSuiteRequiresMockTestServer now.

[Isaac (away), 2012-09-26 06:03:23]

On 2012/09/26 06:01:21, Shouqun Liu wrote:
> On 2012/09/26 05:53:15, Isaac wrote:
> > should these tests be re-enabled?  That list is in
build/android/gtest_filter
> I let the SingleTestRunner._TestSuiteRequiresMockTestServer to return True
> manually, then the test server will be launched.    But from the comment,
Yaron
> said "Disabled because of flakiness" and let it return False. In such case,
> these cases that require test server  will still be failed on bot.    
> I am not sure whether it's OK to turn on _TestSuiteRequiresMockTestServer now.

If you can send try jobs, you could turn it on this patch, and then send say, 10
triggered try jobs to see how flaky it is.

[Isaac (away), 2012-09-26 06:04:13]

i.e. from cmd line:

for i in {0..10}; do git cl try -b android_dbg; done

[Shouqun Liu, 2012-09-26 06:05:43]

On 2012/09/26 06:04:13, Isaac wrote:
> i.e. from cmd line:
> 
> for i in {0..10}; do git cl try -b android_dbg; done

OK. I'll try it:)

[Isaac (away), 2012-09-26 06:13:07]

If you can use CQ you can send try jobs.  It's the same permission set and both
will fail with cryptic errors if you can't do it :-)

[Isaac (away), 2012-09-26 06:29:00]

On 2012/09/26 06:13:07, Isaac wrote:
> If you can use CQ you can send try jobs.  It's the same permission set and
both
> will fail with cryptic errors if you can't do it :-)

yep, you don't have it.  I will send jobs for you

[Isaac (away), 2012-09-26 06:29:43]

On 2012/09/26 06:29:00, Isaac wrote:
> On 2012/09/26 06:13:07, Isaac wrote:
> > If you can use CQ you can send try jobs.  It's the same permission set and
> both
> > will fail with cryptic errors if you can't do it :-)
> 
> yep, you don't have it.  I will send jobs for you

huh, you can send via git try but not git cl try... !

[Shouqun Liu, 2012-09-26 06:30:32]

On 2012/09/26 06:13:07, Isaac wrote:
> If you can use CQ you can send try jobs.  It's the same permission set and
both
> will fail with cryptic errors if you can't do it :-)

unfortunately "git cl try -b android_dbg" failed,  but "git-try -b android_dbg"
can send try job:-)

[Isaac (away), 2012-09-26 06:33:18]

On 2012/09/26 06:30:32, Shouqun Liu wrote:
> On 2012/09/26 06:13:07, Isaac wrote:
> > If you can use CQ you can send try jobs.  It's the same permission set and
> both
> > will fail with cryptic errors if you can't do it :-)
> 
> unfortunately "git cl try -b android_dbg" failed,  but "git-try -b
android_dbg"
> can send try job:-)

In that case, I use:

for i in {0..10}; do (sleep $i && git try -b android_dbg &); done

[Shouqun Liu, 2012-09-26 06:39:01]

On 2012/09/26 06:33:18, Isaac wrote:
> On 2012/09/26 06:30:32, Shouqun Liu wrote:
> > On 2012/09/26 06:13:07, Isaac wrote:
> > > If you can use CQ you can send try jobs.  It's the same permission set and
> > both
> > > will fail with cryptic errors if you can't do it :-)
> > 
> > unfortunately "git cl try -b android_dbg" failed,  but "git-try -b
> android_dbg"
> > can send try job:-)
> 
> In that case, I use:
> 
> for i in {0..10}; do (sleep $i && git try -b android_dbg &); done

got it, I think put the change of  turning on test_server into another CL is
better?

[Isaac (away), 2012-09-26 06:42:04]

sure, that's fine too.

http://codereview.chromium.org/10986042/diff/7002/build/android/pylib/single_...
File build/android/pylib/single_test_runner.py (right):

http://codereview.chromium.org/10986042/diff/7002/build/android/pylib/single_...
build/android/pylib/single_test_runner.py:69: return
(self.test_package.test_suite_basename == 'net_unittests' or False)
we can remove 'or False'.  It doesn't change the boolean evaluation of the
statement.

[Shouqun Liu, 2012-09-26 07:00:58]

On 2012/09/26 06:42:04, Isaac wrote:
> sure, that's fine too.
> 
>
http://codereview.chromium.org/10986042/diff/7002/build/android/pylib/single_...
> File build/android/pylib/single_test_runner.py (right):
> 
>
http://codereview.chromium.org/10986042/diff/7002/build/android/pylib/single_...
> build/android/pylib/single_test_runner.py:69: return
> (self.test_package.test_suite_basename == 'net_unittests' or False)
> we can remove 'or False'.  It doesn't change the boolean evaluation of the
> statement.

failed on bots when starting test server,  which works fine locally:

net/url_request/url_fetcher_impl_unittest.cc:1089: Failure
Value of: test_server.Start()
  Actual: false
Expected: true

[Shouqun Liu, 2012-09-26 07:28:24]

On 2012/09/26 07:00:58, Shouqun Liu wrote:
> On 2012/09/26 06:42:04, Isaac wrote:
> > sure, that's fine too.
> > 
> >
>
http://codereview.chromium.org/10986042/diff/7002/build/android/pylib/single_...
> > File build/android/pylib/single_test_runner.py (right):
> > 
> >
>
http://codereview.chromium.org/10986042/diff/7002/build/android/pylib/single_...
> > build/android/pylib/single_test_runner.py:69: return
> > (self.test_package.test_suite_basename == 'net_unittests' or False)
> > we can remove 'or False'.  It doesn't change the boolean evaluation of the
> > statement.
> 
> failed on bots when starting test server,  which works fine locally:
> 
> net/url_request/url_fetcher_impl_unittest.cc:1089: Failure
> Value of: test_server.Start()
>   Actual: false
> Expected: true

Clean the patch and upload the patchset 3.    Remove the test_server enabling
code, postpone it to another CL which focuses on the test server enabling.

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

Review comments on patch set 3:

Thank you for the patch. I suggest two changes.

http://codereview.chromium.org/10986042/diff/11003/net/url_request/url_fetche...
File net/url_request/url_fetcher_impl_unittest.cc (right):

http://codereview.chromium.org/10986042/diff/11003/net/url_request/url_fetche...
net/url_request/url_fetcher_impl_unittest.cc:388:
.AppendASCII(FILE_PATH_LITERAL("data"));

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.

http://codereview.chromium.org/10986042/diff/11003/net/url_request/url_fetche...
net/url_request/url_fetcher_impl_unittest.cc:1096: expected_file_ =
document_root_.AppendASCII(kFileToFetch);

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]

Hi wtc, thanks for your comments, my answers below:

http://codereview.chromium.org/10986042/diff/11003/net/url_request/url_fetche...
File net/url_request/url_fetcher_impl_unittest.cc (right):

http://codereview.chromium.org/10986042/diff/11003/net/url_request/url_fetche...
net/url_request/url_fetcher_impl_unittest.cc:388:
.AppendASCII(FILE_PATH_LITERAL("data"));
yes, agree,  Android is a special case. 
On 2012/09/26 23:48:20, wtc wrote:
> 
> 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.

http://codereview.chromium.org/10986042/diff/11003/net/url_request/url_fetche...
net/url_request/url_fetcher_impl_unittest.cc:1096: expected_file_ =
document_root_.AppendASCII(kFileToFetch);
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:
> 
> 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.

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

http://codereview.chromium.org/10986042/diff/11003/net/url_request/url_fetche...
File net/url_request/url_fetcher_impl_unittest.cc (right):

http://codereview.chromium.org/10986042/diff/11003/net/url_request/url_fetche...
net/url_request/url_fetcher_impl_unittest.cc:1088: FilePath(kDocRoot));

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.

http://codereview.chromium.org/10986042/diff/11003/net/url_request/url_fetche...
net/url_request/url_fetcher_impl_unittest.cc:1096: expected_file_ =
document_root_.AppendASCII(kFileToFetch);

On 2012/09/27 01:24:51, Shouqun Liu wrote:
>
> 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

Hi Shouqun,

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

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

patch refined by adding GetDocumentRoot() function and avoiding duplicating
#ifdef in multiple tests, thanks for review

http://codereview.chromium.org/10986042/diff/11003/net/url_request/url_fetche...
File net/url_request/url_fetcher_impl_unittest.cc (right):

http://codereview.chromium.org/10986042/diff/11003/net/url_request/url_fetche...
net/url_request/url_fetcher_impl_unittest.cc:1096: expected_file_ =
document_root_.AppendASCII(kFileToFetch);
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:
> 
> On 2012/09/27 01:24:51, Shouqun Liu wrote:
> >
> > 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
> 
> Hi Shouqun,
> 
> Thank you for the explanation. We should avoid duplicating
> this ifdef in multiple tests.

[wtc, 2012-10-16 21:43:03]

Review comments on patch set 5:

Shouqun: I'm terribly sorry that I forgot to reply to you!
I believe I reviewed this patch set 5 shortly after you uploaded
it, but I might be traveling at that time and forgot to write
down my review comments :-(

http://codereview.chromium.org/10986042/diff/17001/net/url_request/url_fetche...
File net/url_request/url_fetcher_impl_unittest.cc (right):

http://codereview.chromium.org/10986042/diff/17001/net/url_request/url_fetche...
net/url_request/url_fetcher_impl_unittest.cc:398: return
test_server.document_root();

What does test_server.document_root() return for OS_ANDROID?
Does the return value contain kDocRoot?

If so, can we extract the kDocRoot part from the return value
and append that to base::DIR_SOURCE_ROOT?

What I want to avoid is the hardcoded knowledge of kDocRoot
on line 387. I'd like to get that from test_server.

http://codereview.chromium.org/10986042/diff/17001/net/url_request/url_fetche...
net/url_request/url_fetcher_impl_unittest.cc:406: FilePath document_root_;

The document_root_ member should be inside #if defined(OS_ANDROID).

[Shouqun Liu, 2012-10-17 08:44:30]

http://codereview.chromium.org/10986042/diff/17001/net/url_request/url_fetche...
File net/url_request/url_fetcher_impl_unittest.cc (right):

http://codereview.chromium.org/10986042/diff/17001/net/url_request/url_fetche...
net/url_request/url_fetcher_impl_unittest.cc:398: return
test_server.document_root();
Hi wtc, thanks for your advice!
For Android,  test_server.document_root()  simply returns the kDocRoot
("chrome/test/data"), as the document root, but this patch is not a valid path
for Android test case to use. 
So "base::DIR_SOURCE_ROOT" is appended here to let the case to find the correct
path.

And here are two options: 
(1)  [patchset 6] keep this change which uses "base::DIR_SOURCE_ROOT"
specifically for Android  ; 
(2)  [patchset 7] change the implementation of document_root() in test_server,
but the test server also use this "document_root" value to launch the test
server in host PC. 
And here is the conflict: test server in PC and test case in Android can't share
the same "document_root".   So need to add a additional method to get the
document root in device (e.g. GetDocumentRootInDevice).

Which one do you thinks is better? 

On 2012/10/16 21:43:03, wtc wrote:
> 
> What does test_server.document_root() return for OS_ANDROID?
> Does the return value contain kDocRoot?
> 
> If so, can we extract the kDocRoot part from the return value
> and append that to base::DIR_SOURCE_ROOT?
> 
> What I want to avoid is the hardcoded knowledge of kDocRoot
> on line 387. I'd like to get that from test_server.

[wtc, 2012-10-18 22:25:01]

http://codereview.chromium.org/10986042/diff/23001/net/url_request/url_fetche...
File net/url_request/url_fetcher_impl_unittest.cc (right):

http://codereview.chromium.org/10986042/diff/23001/net/url_request/url_fetche...
net/url_request/url_fetcher_impl_unittest.cc:394: FilePath GetDocumentRoot(const
TestServer& test_server) {

My proposal is to remove the document_root member and the
changes to the URLFetcherFileTest constructor.

Then, define the GetDocumentRoot method as follows:

  static FilePath GetDocumentRoot(const TestServer& test_server) {
    FilePath document_root = test_server.document_root();
    // Add comments to explain why this is necessary...
#if defined(OS_ANDROID)
    FilePath source_root;
    PathService::Get(base::DIR_SOURCE_ROOT, &source_root);
    document_root = source_root.AppendASCII(document_root);
#endif
    return document_root;
  }

I discovered that this is essentially your patch set 7.

So the question is whether this method should be defined
here or should be defined as a method of TestServer. What
do you think? I think if other unit tests will also need
this method, then we should define it as a method of
TestServer. Otherwise it is fine to just define it here.

[Shouqun Liu, 2012-10-19 01:15:31]

On 2012/10/18 22:25:01, wtc wrote:
>
http://codereview.chromium.org/10986042/diff/23001/net/url_request/url_fetche...
> File net/url_request/url_fetcher_impl_unittest.cc (right):
> 
>
http://codereview.chromium.org/10986042/diff/23001/net/url_request/url_fetche...
> net/url_request/url_fetcher_impl_unittest.cc:394: FilePath
GetDocumentRoot(const
> TestServer& test_server) {
> 
> My proposal is to remove the document_root member and the
> changes to the URLFetcherFileTest constructor.
> 
> Then, define the GetDocumentRoot method as follows:
> 
>   static FilePath GetDocumentRoot(const TestServer& test_server) {
>     FilePath document_root = test_server.document_root();
>     // Add comments to explain why this is necessary...
> #if defined(OS_ANDROID)
>     FilePath source_root;
>     PathService::Get(base::DIR_SOURCE_ROOT, &source_root);
>     document_root = source_root.AppendASCII(document_root);
> #endif
>     return document_root;
>   }
> 
> I discovered that this is essentially your patch set 7.
> 
> So the question is whether this method should be defined
> here or should be defined as a method of TestServer. What
> do you think? I think if other unit tests will also need
> this method, then we should define it as a method of
> TestServer. Otherwise it is fine to just define it here.

yes,  currently only two other unit tests use test_server->document_root() to
retrieve the doc root (GDataTest.Download, RenderViewHostTest.BaseURLParam),  so
I think we can put this method into TestServer.

[wtc, 2012-10-23 18:28:31]

Patch set 8 LGTM. Thanks. I suggest some comment changes.

http://codereview.chromium.org/10986042/diff/27003/net/test/local_test_server.h
File net/test/local_test_server.h (right):

http://codereview.chromium.org/10986042/diff/27003/net/test/local_test_server...
net/test/local_test_server.h:56: FilePath GetDocumentRoot() const { return
document_root(); };

Please document this function.

http://codereview.chromium.org/10986042/diff/27003/net/test/remote_test_serve...
File net/test/remote_test_server.cc (right):

http://codereview.chromium.org/10986042/diff/27003/net/test/remote_test_serve...
net/test/remote_test_server.cc:187: // in host machine where test server is
launched. So append DIR_SOURCE_ROOT

1. This method should be defined between Stop() and Init()
to match the declaration order in the .h file. This is
recommended by the Style Guide:

  Method definitions in the corresponding .cc file should
  be the same as the declaration order, as much as
  possible. 

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

2. Change "append" to "prepend" because src_dir is before
document_root(). Alternatively, say "So append the document
root to DIR_SOURCE_ROOT here to get the ..."

3. Nit: add "the" before "device", "host machine", "test server",
and "Android device".

http://codereview.chromium.org/10986042/diff/27003/net/test/remote_test_server.h
File net/test/remote_test_server.h (right):

http://codereview.chromium.org/10986042/diff/27003/net/test/remote_test_serve...
net/test/remote_test_server.h:40: FilePath GetDocumentRoot() const;

Please document this function. In particular, explain when
to use document_root() and when to use this function.

[Shouqun Liu, 2012-10-24 02:32:47]

thanks for your careful review, nits fixed.

http://codereview.chromium.org/10986042/diff/27003/net/test/local_test_server.h
File net/test/local_test_server.h (right):

http://codereview.chromium.org/10986042/diff/27003/net/test/local_test_server...
net/test/local_test_server.h:56: FilePath GetDocumentRoot() const { return
document_root(); };
On 2012/10/23 18:28:31, wtc wrote:
> 
> Please document this function.

Done.

http://codereview.chromium.org/10986042/diff/27003/net/test/remote_test_serve...
File net/test/remote_test_server.cc (right):

http://codereview.chromium.org/10986042/diff/27003/net/test/remote_test_serve...
net/test/remote_test_server.cc:187: // in host machine where test server is
launched. So append DIR_SOURCE_ROOT
On 2012/10/23 18:28:31, wtc wrote:
> 
> 1. This method should be defined between Stop() and Init()
> to match the declaration order in the .h file. This is
> recommended by the Style Guide:
> 
>   Method definitions in the corresponding .cc file should
>   be the same as the declaration order, as much as
>   possible. 
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order
> 
> 2. Change "append" to "prepend" because src_dir is before
> document_root(). Alternatively, say "So append the document
> root to DIR_SOURCE_ROOT here to get the ..."
> 
> 3. Nit: add "the" before "device", "host machine", "test server",
> and "Android device".

Done.

http://codereview.chromium.org/10986042/diff/27003/net/test/remote_test_server.h
File net/test/remote_test_server.h (right):

http://codereview.chromium.org/10986042/diff/27003/net/test/remote_test_serve...
net/test/remote_test_server.h:40: FilePath GetDocumentRoot() const;
On 2012/10/23 18:28:31, wtc wrote:
> 
> Please document this function. In particular, explain when
> to use document_root() and when to use this function.

Done.

[wtc, 2012-10-25 00:20:23]

Patch set 9 LGTM.

[commit-bot: I haz the power, 2012-10-25 04:23:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shouqun.liu@intel.com/10986042/36001

[commit-bot: I haz the power, 2012-10-25 08:23:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shouqun.liu@intel.com/10986042/36001

[commit-bot: I haz the power, 2012-10-25 08:31:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shouqun.liu@intel.com/10986042/51001

[commit-bot: I haz the power, 2012-10-25 09:31:47]

Retried try job too often for step(s) unit_tests

[commit-bot: I haz the power, 2012-10-25 12:50:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shouqun.liu@intel.com/10986042/51001

[commit-bot: I haz the power, 2012-10-25 13:17:30]

Change committed as 164061