GTTF: convert some tests in chrome to use EmbeddedTestServer patch nr 1

GTTF: convert some tests in chrome to use EmbeddedTestServer patch nr 1

This eliminates a whole class of problems with SpawnedTestServer
failing to start.

BUG=96594, 98194
R=asargent@chromium.org, rsleevi@chromium.org, sky@chromium.org

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

[sky, 2013-06-07 16:40:55]

LGTM

[Paweł Hajdan Jr., 2013-06-21 16:37:43]

Scott, feel free to take another look - tryservers made me make a lot of changes
to tests, so while most of trivial changes are similar, quite a lot more files
got changed.

Ryan, could you review net?

Antony, could you look at extension test changes? Lots of files modified here
(most changes are trivial), so some highlights:

1. chrome/browser/extensions/extension_apitest.cc - contains C++ request
handlers for the embedded test server; these are used by several API tests.

2. chrome/test/data/extensions/api_test/cross_origin_xhr - the tests were not
valid in the first place, as only HTTP server was started which somehow worked
with the old server but doesn't with the new one (request times out); note that
the test checks for failure of the request, but it fails for different reason:
the server just doesn't support HTTPS, so the test was invalid since the
beginning

[sky, 2013-06-21 19:35:32]

SLGTM

[asargent_no_longer_on_chrome, 2013-06-21 20:02:11]

https://codereview.chromium.org/16268017/diff/64001/chrome/browser/extensions...
File chrome/browser/extensions/extension_apitest.cc (left):

https://codereview.chromium.org/16268017/diff/64001/chrome/browser/extensions...
chrome/browser/extensions/extension_apitest.cc:309: if (!test_server()->Start())
It seems kind of strange to have a method called StartTestServer that no longer
starts the test server, leading to code in consumers like the following:

ASSERT_TRUE(StartTestServer());
ASSERT_TRUE(test_server()->Start());

which reads very awkwardly. 

If I'm understanding your change correctly, you're making this method now do
something like "initialize" the test server, but not start it. Would it make
sense to rename this method to InitializeTestServer, and then add a new
StartTestServer method which calls InitializeTestServer and then does the
test_server()->Start() call?

[Paweł Hajdan Jr., 2013-06-21 20:37:31]

https://codereview.chromium.org/16268017/diff/64001/chrome/browser/extensions...
File chrome/browser/extensions/extension_apitest.cc (left):

https://codereview.chromium.org/16268017/diff/64001/chrome/browser/extensions...
chrome/browser/extensions/extension_apitest.cc:309: if (!test_server()->Start())
On 2013/06/21 20:02:11, Antony Sargent wrote:
> If I'm understanding your change correctly, you're making this method now do
> something like "initialize" the test server, but not start it. Would it make
> sense to rename this method to InitializeTestServer, and then add a new
> StartTestServer method which calls InitializeTestServer and then does the
> test_server()->Start() call?

There are two test servers here: test_server() - old, out of process, and
embedded_test_server() - new, in process one.

This change in code makes StartTestServer() start the embedded test server.
Tests that need the test_server() for any reason still call
test_server()->Start(), but I think it's an exception rather than rule.

[asargent_no_longer_on_chrome, 2013-06-21 23:08:14]

lgtm w/ one request for a change before commit

https://codereview.chromium.org/16268017/diff/64001/chrome/browser/extensions...
File chrome/browser/extensions/extension_apitest.cc (left):

https://codereview.chromium.org/16268017/diff/64001/chrome/browser/extensions...
chrome/browser/extensions/extension_apitest.cc:309: if (!test_server()->Start())
On 2013/06/21 20:37:31, Paweł Hajdan Jr. wrote:
> On 2013/06/21 20:02:11, Antony Sargent wrote:
> > If I'm understanding your change correctly, you're making this method now do
> > something like "initialize" the test server, but not start it. Would it make
> > sense to rename this method to InitializeTestServer, and then add a new
> > StartTestServer method which calls InitializeTestServer and then does the
> > test_server()->Start() call?
> 
> There are two test servers here: test_server() - old, out of process, and
> embedded_test_server() - new, in process one.
> 
> This change in code makes StartTestServer() start the embedded test server.
> Tests that need the test_server() for any reason still call
> test_server()->Start(), but I think it's an exception rather than rule.

Ok, I see. Can we rename one of the two then? I don't care so much which one,
but right now it is confusing that the names are so similar. Perhaps something
like one of these two:

ASSERT_TRUE(StartEmbeddedTestServer());
ASSERT_TRUE(test_server()->Start());

or

ASSERT_TRUE(StartTestServer());
ASSERT_TRUE(external_test_server()->Start());

?

https://codereview.chromium.org/16268017/diff/64001/chrome/browser/extensions...
File chrome/browser/extensions/extension_apitest.cc (right):

https://codereview.chromium.org/16268017/diff/64001/chrome/browser/extensions...
chrome/browser/extensions/extension_apitest.cc:125: 
observation: This whole block of new functions seems generally useful and not
particularly specific to ExtensionApiTest. Could it live in a more common place?

[Paweł Hajdan Jr., 2013-06-22 00:16:01]

https://codereview.chromium.org/16268017/diff/64001/chrome/browser/extensions...
File chrome/browser/extensions/extension_apitest.cc (left):

https://codereview.chromium.org/16268017/diff/64001/chrome/browser/extensions...
chrome/browser/extensions/extension_apitest.cc:309: if (!test_server()->Start())
On 2013/06/21 23:08:14, Antony Sargent wrote:
> Ok, I see. Can we rename one of the two then? I don't care so much which one,
> but right now it is confusing that the names are so similar. Perhaps something
> like one of these two:
> 
> ASSERT_TRUE(StartEmbeddedTestServer());
> ASSERT_TRUE(test_server()->Start());

Done.

https://codereview.chromium.org/16268017/diff/64001/chrome/browser/extensions...
File chrome/browser/extensions/extension_apitest.cc (right):

https://codereview.chromium.org/16268017/diff/64001/chrome/browser/extensions...
chrome/browser/extensions/extension_apitest.cc:125: 
On 2013/06/21 23:08:14, Antony Sargent wrote:
> observation: This whole block of new functions seems generally useful and not
> particularly specific to ExtensionApiTest. Could it live in a more common
place?

I was thinking about it. For now I prefer it to be limited, so that other new
tests don't use it.

If there is a need, it will be moved, yes.

[Ryan Sleevi, 2013-06-24 18:32:33]

It seems like a number of these changes could be replaced by using
URLRequestPrepackagedInterceptor instead. This was the general concern I raised
with the embedded test server when proposed.

https://codereview.chromium.org/16268017/diff/77001/net/http/http_status_code...
File net/http/http_status_code_list.h (right):

https://codereview.chromium.org/16268017/diff/77001/net/http/http_status_code...
net/http/http_status_code_list.h:38: HTTP_STATUS(PERMANENT_REDIRECT, 308,
"Permanent Redirect")
This seems like it should be in its own CL. I'm having trouble understanding the
motivation for this change.

Of all the status codes, this one is experimental (see http://crbug.com/109012
). I would prefer not to land this in this CL.

[Paweł Hajdan Jr., 2013-06-24 19:23:43]

The prepackaged interceptor is less convenient to use than the embedded test
server. Also, for integration tests I'd rather use the full stack (your points
about SSL are good though so for SSL I'm going to use a different solution
indeed).

Please just review the net parts.

https://codereview.chromium.org/16268017/diff/77001/net/http/http_status_code...
File net/http/http_status_code_list.h (right):

https://codereview.chromium.org/16268017/diff/77001/net/http/http_status_code...
net/http/http_status_code_list.h:38: HTTP_STATUS(PERMANENT_REDIRECT, 308,
"Permanent Redirect")
On 2013/06/24 18:32:34, Ryan Sleevi wrote:
> This seems like it should be in its own CL. I'm having trouble understanding
the
> motivation for this change.

Embedded HTTP Server uses this list to construct HTTP response (it needs the
description of the code to make extension Request API tests pass).

Some other tests (related to gdata) use 308 code, and so it needs description
here.

Alternatively, I'd need to remove the DCHECK and make the function to query the
description have a bool return value.

> Of all the status codes, this one is experimental (see http://crbug.com/109012
> ). I would prefer not to land this in this CL.

I can extract this change to a separate CL, please let me know if that works for
you.

[Ryan Sleevi, 2013-06-26 19:06:38]

My suggestion to use the URLRequest interceptor is to make these tests *less*
integration tests and *more* about actual unit tests.

I agree that integration tests have value, but I'd much rather be able to test
fewer moving parts.

That said, net/ LGTM, mod comments about seeing 308 split out in a separate CL
for clarity (I think that's good), and the note about the BUG below.

https://codereview.chromium.org/16268017/diff/87001/net/test/embedded_test_se...
File net/test/embedded_test_server/http_response.cc (right):

https://codereview.chromium.org/16268017/diff/87001/net/test/embedded_test_se...
net/test/embedded_test_server/http_response.cc:35:
base::StringAppendF(&response_builder, "Connection: closed\r\n");
BUG: This should be "Connection: close" (note the lack of d)

[Paweł Hajdan Jr., 2013-06-26 19:47:22]

https://codereview.chromium.org/16268017/diff/87001/net/test/embedded_test_se...
File net/test/embedded_test_server/http_response.cc (right):

https://codereview.chromium.org/16268017/diff/87001/net/test/embedded_test_se...
net/test/embedded_test_server/http_response.cc:35:
base::StringAppendF(&response_builder, "Connection: closed\r\n");
On 2013/06/26 19:06:39, Ryan Sleevi wrote:
> BUG: This should be "Connection: close" (note the lack of d)

Done.

[Paweł Hajdan Jr., 2013-06-26 20:38:06]

fixes

[Paweł Hajdan Jr., 2013-06-26 20:59:13]

Committed patchset #19 manually as r208784 (presubmit successful).