google_apis: Add GetURL() to test_server::HttpServer

google_apis: Add GetURL() to test_server::HttpServer

Add GetURL() and remove the return value from Register* functions,
as using GetURL() at the call sites of URLFetcher makes the code
clearer about target URLs.

Along the way, add tests for GetBaseURL(), GetURL(),
RegisterDefaultResponse(), RegisterFileResponse(), as well as
concurrent fetches.

BUG=148710
TEST=none; just changing test utils.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=168482

[hashimoto, 2012-11-16 05:38:10]

https://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/te...
File chrome/browser/google_apis/test_server/http_server.cc (right):

https://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/te...
chrome/browser/google_apis/test_server/http_server.cc:155:
DCHECK(StartsWithASCII(relative_url, "/", true));
Please add describe that 'true' means case sensitive by comments or adding a
variable.
The same goes for other places.

https://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/te...
File chrome/browser/google_apis/test_server/http_server.h (right):

https://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/te...
chrome/browser/google_apis/test_server/http_server.h:98: // custom data, such as
additional headers should be send from the server.
s/should be send/should be sent/

https://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/te...
File chrome/browser/google_apis/test_server/http_server_unittest.cc (right):

https://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/te...
chrome/browser/google_apis/test_server/http_server_unittest.cc:105:
http_response.set_code(MOVED);
Please add a brief description about why you chose 301 as the expected response
code for this test.

https://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/te...
chrome/browser/google_apis/test_server/http_server_unittest.cc:106:
http_response.set_content("<b>Moved!</b>");
How about making this data a constant?
The same goes for other tests.

https://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/te...
chrome/browser/google_apis/test_server/http_server_unittest.cc:107:
http_response.set_content_type("text/html");
ditto.

https://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/te...
chrome/browser/google_apis/test_server/http_server_unittest.cc:108:
http_response.AddCustomHeader("Server", "test server");
ditto.

https://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/te...
chrome/browser/google_apis/test_server/http_server_unittest.cc:109:
server_.RegisterDefaultResponse("/test", http_response);
ditto.

https://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/te...
chrome/browser/google_apis/test_server/http_server_unittest.cc:120:
EXPECT_EQ(301, fetcher->GetResponseCode());
Why are you using MOVED above and 301 here?
The same goes for other tests.

https://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/te...
chrome/browser/google_apis/test_server/http_server_unittest.cc:126: 
nit: No need have a blank line here.

https://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/te...
chrome/browser/google_apis/test_server/http_server_unittest.cc:172:
net::URLFetcher::Create(server_.GetBaseURL(),
Instead of GetBaseURL, how about using something like GetURL("/non-existent")?
I think this would make the purpose of this test clearer.

https://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/te...
chrome/browser/google_apis/test_server/http_server_unittest.cc:224:
EXPECT_EQ(net::URLRequestStatus::SUCCESS, fetcher1->GetStatus().status());
fetcher2?

https://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/te...
chrome/browser/google_apis/test_server/http_server_unittest.cc:229:
EXPECT_EQ(net::URLRequestStatus::SUCCESS, fetcher1->GetStatus().status());
fetcher3?

[satorux1, 2012-11-16 05:56:44]

http://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/tes...
File chrome/browser/google_apis/test_server/http_server.cc (right):

http://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/tes...
chrome/browser/google_apis/test_server/http_server.cc:155:
DCHECK(StartsWithASCII(relative_url, "/", true));
On 2012/11/16 05:38:10, hashimoto wrote:
> Please add describe that 'true' means case sensitive by comments or adding a
> variable.
> The same goes for other places.

Done. boolean parameters like this are evil...

http://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/tes...
File chrome/browser/google_apis/test_server/http_server.h (right):

http://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/tes...
chrome/browser/google_apis/test_server/http_server.h:98: // custom data, such as
additional headers should be send from the server.
On 2012/11/16 05:38:10, hashimoto wrote:
> s/should be send/should be sent/

Done.

http://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/tes...
File chrome/browser/google_apis/test_server/http_server_unittest.cc (right):

http://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/tes...
chrome/browser/google_apis/test_server/http_server_unittest.cc:105:
http_response.set_code(MOVED);
On 2012/11/16 05:38:10, hashimoto wrote:
> Please add a brief description about why you chose 301 as the expected
response
> code for this test.

Done.

http://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/tes...
chrome/browser/google_apis/test_server/http_server_unittest.cc:106:
http_response.set_content("<b>Moved!</b>");
On 2012/11/16 05:38:10, hashimoto wrote:
> How about making this data a constant?
> The same goes for other tests.

It's rather repetitive, but I think just using string literals is easier to
read.

http://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/tes...
chrome/browser/google_apis/test_server/http_server_unittest.cc:120:
EXPECT_EQ(301, fetcher->GetResponseCode());
On 2012/11/16 05:38:10, hashimoto wrote:
> Why are you using MOVED above and 301 here?
> The same goes for other tests.

Done.

http://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/tes...
chrome/browser/google_apis/test_server/http_server_unittest.cc:126: 
On 2012/11/16 05:38:10, hashimoto wrote:
> nit: No need have a blank line here.

Done.

http://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/tes...
chrome/browser/google_apis/test_server/http_server_unittest.cc:172:
net::URLFetcher::Create(server_.GetBaseURL(),
On 2012/11/16 05:38:10, hashimoto wrote:
> Instead of GetBaseURL, how about using something like GetURL("/non-existent")?
> I think this would make the purpose of this test clearer.

Done.

http://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/tes...
chrome/browser/google_apis/test_server/http_server_unittest.cc:224:
EXPECT_EQ(net::URLRequestStatus::SUCCESS, fetcher1->GetStatus().status());
On 2012/11/16 05:38:10, hashimoto wrote:
> fetcher2?

Good catch! Done.

http://codereview.chromium.org/11416030/diff/1/chrome/browser/google_apis/tes...
chrome/browser/google_apis/test_server/http_server_unittest.cc:229:
EXPECT_EQ(net::URLRequestStatus::SUCCESS, fetcher1->GetStatus().status());
On 2012/11/16 05:38:10, hashimoto wrote:
> fetcher3?

Done.

[hashimoto, 2012-11-16 06:13:07]

lgtm, please diff against the correct branch.

[satorux1, 2012-11-16 06:33:51]

oops. uploaded the right diff.

[hashimoto, 2012-11-16 06:43:09]

lgtm

[satorux1, 2012-11-16 08:10:09]

fix the win test failure...

[mtomasz, 2012-11-16 17:24:17]

lgtm

Thanks for this!

https://codereview.chromium.org/11416030/diff/11003/chrome/browser/google_api...
File chrome/browser/google_apis/test_server/http_server_unittest.cc (right):

https://codereview.chromium.org/11416030/diff/11003/chrome/browser/google_api...
chrome/browser/google_apis/test_server/http_server_unittest.cc:166: // Trim the
trailing whitespace as it can be CRLF on Windows...
Why this can happen? Is it a bug in URLFetcher, file_util::ReadFileToString or
sth else?

[satorux1, 2012-11-16 23:46:50]

https://codereview.chromium.org/11416030/diff/11003/chrome/browser/google_api...
File chrome/browser/google_apis/test_server/http_server_unittest.cc (right):

https://codereview.chromium.org/11416030/diff/11003/chrome/browser/google_api...
chrome/browser/google_apis/test_server/http_server_unittest.cc:166: // Trim the
trailing whitespace as it can be CRLF on Windows...
On 2012/11/16 17:24:17, mtomasz wrote:
> Why this can happen? Is it a bug in URLFetcher, file_util::ReadFileToString or
> sth else?

It's SVN converting the line breaks for Windows.

[satorux1, 2012-11-19 00:07:49]

HttpServerTest.RegisterFileResponse failed on Android, due to failure to open a
test file. Looks we cannot open test files on Android. For now, I'm going to
exclude the test from Android.