GTTF: test server cleanup:

GTTF: test server cleanup:

- simplify the public interface
- remove unneeded methods
- make it easier to understand

TEST=none
BUG=49680
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=53509

[Paweł Hajdan Jr., 2010-07-22 02:17:13]

I don't have trybot results yet, it migh need few updates, but rather trivial
ones.

[eroman, 2010-07-22 03:14:50]

nice simplification.

http://codereview.chromium.org/2881028/diff/1/19
File chrome/test/in_process_browser_test.cc (left):

http://codereview.chromium.org/2881028/diff/1/19#oldcode246
chrome/test/in_process_browser_test.cc:246:
g_browser_process->io_thread()->message_loop());
Why is this no longer an issue?

http://codereview.chromium.org/2881028/diff/1/25
File net/test/test_server.cc (right):

http://codereview.chromium.org/2881028/diff/1/25#newcode234
net/test/test_server.cc:234: rv =
pinger.Ping(base::TimeDelta::FromMilliseconds(kServerConnectionTimeoutMs),
nit: line is longer than 80 characters

http://codereview.chromium.org/2881028/diff/1/26
File net/test/test_server.h (right):

http://codereview.chromium.org/2881028/diff/1/26#newcode43
net/test/test_server.h:43: bool LoadTestRootCert() WARN_UNUSED_RESULT;
interesting, I didn't know about this!

http://codereview.chromium.org/2881028/diff/1/27
File net/tools/testserver/testserver.py (left):

http://codereview.chromium.org/2881028/diff/1/27#oldcode1203
net/tools/testserver/testserver.py:1203: if options.forking:
What is the impact of removing the forking server?

http://codereview.chromium.org/2881028/diff/1/27
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/2881028/diff/1/27#newcode85
net/tools/testserver/testserver.py:85: class
ForkingHTTPServer(SocketServer.ForkingMixIn, StoppableHTTPServer):
This should be removed too.

http://codereview.chromium.org/2881028/diff/1/27#newcode90
net/tools/testserver/testserver.py:90: class
ForkingHTTPSServer(SocketServer.ForkingMixIn, HTTPSServer):
This should be removed too.

http://codereview.chromium.org/2881028/diff/1/28
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/2881028/diff/1/28#newcode278
net/url_request/url_request_unittest.cc:278: if (event_.TimedWait(td))
how about:

return event_.TimedWait(td);

http://codereview.chromium.org/2881028/diff/1/28#newcode290
net/url_request/url_request_unittest.cc:290: URLRequest* request = new
URLRequest(url, delegate);
Who is responsible for deleting |request| ?

I would expect that |delegate| must have to delete |request|, however looking at
the caller it uses SyncTestDelegate and that does not seem to delete the
request.
I looked at the caller of this, and it uses

http://codereview.chromium.org/2881028/diff/1/28#newcode310
net/url_request/url_request_unittest.cc:310: // Build bot wait for only 300
seconds we should ensure wait do not take
wording suggestions: "Build bot waits for only 300 seconds, so we should ensure
the test doesn't take longer than this".

[Paweł Hajdan Jr., 2010-07-22 19:01:51]

Patch updated, contains some trybot fixes. Please take another look.

ananta, I modified some ChromeFrame test code. Please take a look, I hope you're
not too upset about removing this. It makes the code simpler, which is the first
step to make it faster and more reliable.

http://codereview.chromium.org/2881028/diff/1/19
File chrome/test/in_process_browser_test.cc (left):

http://codereview.chromium.org/2881028/diff/1/19#oldcode246
chrome/test/in_process_browser_test.cc:246:
g_browser_process->io_thread()->message_loop());
On 2010/07/22 03:14:51, eroman wrote:
> Why is this no longer an issue?

The MessageLoop* was only needed for QuitTest in net_unittests. I removed it
from the interface, it was not used here even when passed.

http://codereview.chromium.org/2881028/diff/1/25
File net/test/test_server.cc (right):

http://codereview.chromium.org/2881028/diff/1/25#newcode234
net/test/test_server.cc:234: rv =
pinger.Ping(base::TimeDelta::FromMilliseconds(kServerConnectionTimeoutMs),
On 2010/07/22 03:14:51, eroman wrote:
> nit: line is longer than 80 characters

Done.

http://codereview.chromium.org/2881028/diff/1/27
File net/tools/testserver/testserver.py (left):

http://codereview.chromium.org/2881028/diff/1/27#oldcode1203
net/tools/testserver/testserver.py:1203: if options.forking:
On 2010/07/22 03:14:51, eroman wrote:
> What is the impact of removing the forking server?

About 15-20 seconds faster run time on POSIX, possibly less isolated tests. But
we were running it on Windows non-forking all the time, so I guess it's fine.

The rationale for removing it complitly is that it makes writing QuitTest
easier. For QuitTest we shouldn't use the forking server (because only the child
will die, and not the master), and having the forking server setting switchable
makes things more complicated.

http://codereview.chromium.org/2881028/diff/1/27
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/2881028/diff/1/27#newcode85
net/tools/testserver/testserver.py:85: class
ForkingHTTPServer(SocketServer.ForkingMixIn, StoppableHTTPServer):
On 2010/07/22 03:14:51, eroman wrote:
> This should be removed too.

Done.

http://codereview.chromium.org/2881028/diff/1/27#newcode90
net/tools/testserver/testserver.py:90: class
ForkingHTTPSServer(SocketServer.ForkingMixIn, HTTPSServer):
On 2010/07/22 03:14:51, eroman wrote:
> This should be removed too.

Done.

http://codereview.chromium.org/2881028/diff/1/28
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/2881028/diff/1/28#newcode278
net/url_request/url_request_unittest.cc:278: if (event_.TimedWait(td))
On 2010/07/22 03:14:51, eroman wrote:
> how about:
> 
> return event_.TimedWait(td);

Oh, sure. I was just moving the code without really reading it.

http://codereview.chromium.org/2881028/diff/1/28#newcode290
net/url_request/url_request_unittest.cc:290: URLRequest* request = new
URLRequest(url, delegate);
On 2010/07/22 03:14:51, eroman wrote:
> Who is responsible for deleting |request| ?
> 
> I would expect that |delegate| must have to delete |request|, however looking
at
> the caller it uses SyncTestDelegate and that does not seem to delete the
> request.
> I looked at the caller of this, and it uses 

See the DeleteSoon call in OnResponseCompleted above. Yeah, I was also slightly
confused by this. Can it be done in a more obvious way?

http://codereview.chromium.org/2881028/diff/1/28#newcode310
net/url_request/url_request_unittest.cc:310: // Build bot wait for only 300
seconds we should ensure wait do not take
On 2010/07/22 03:14:51, eroman wrote:
> wording suggestions: "Build bot waits for only 300 seconds, so we should
ensure
> the test doesn't take longer than this".

Just removed the comment and decreased the timeout. If it takes minutes to kill
the server, we're screwed anyway.

[eroman, 2010-07-22 19:14:59]

LGTM

http://codereview.chromium.org/2881028/diff/1/28
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/2881028/diff/1/28#newcode290
net/url_request/url_request_unittest.cc:290: URLRequest* request = new
URLRequest(url, delegate);
On 2010/07/22 19:01:52, Paweł Hajdan Jr. wrote:
> On 2010/07/22 03:14:51, eroman wrote:
> > Who is responsible for deleting |request| ?
> > 
> > I would expect that |delegate| must have to delete |request|, however
looking
> at
> > the caller it uses SyncTestDelegate and that does not seem to delete the
> > request.
> > I looked at the caller of this, and it uses 
> 
> See the DeleteSoon call in OnResponseCompleted above. Yeah, I was also
slightly
> confused by this. Can it be done in a more obvious way?

Oh d'uh, how did I not see the DeleteSoon! I think my eyes must have been
scanning for a "delete" token :)

[huanr, 2010-07-23 07:10:06]

Pawel, anything you want me to look at particularly in addition to Eric's
review?

Huan

On Thu, Jul 22, 2010 at 12:15 PM, <eroman@chromium.org> wrote:

> LGTM
>
>
>
> http://codereview.chromium.org/2881028/diff/1/28
> File net/url_request/url_request_unittest.cc (right):
>
> http://codereview.chromium.org/2881028/diff/1/28#newcode290
> net/url_request/url_request_unittest.cc:290: URLRequest* request = new
> URLRequest(url, delegate);
> On 2010/07/22 19:01:52, Paweł Hajdan Jr. wrote:
>
>> On 2010/07/22 03:14:51, eroman wrote:
>> > Who is responsible for deleting |request| ?
>> >
>> > I would expect that |delegate| must have to delete |request|,
>>
> however looking
>
>> at
>> > the caller it uses SyncTestDelegate and that does not seem to delete
>>
> the
>
>> > request.
>> > I looked at the caller of this, and it uses
>>
>
>  See the DeleteSoon call in OnResponseCompleted above. Yeah, I was also
>>
> slightly
>
>> confused by this. Can it be done in a more obvious way?
>>
>
> Oh d'uh, how did I not see the DeleteSoon! I think my eyes must have
> been scanning for a "delete" token :)
>
>
> http://codereview.chromium.org/2881028/show
>

[Paweł Hajdan Jr., 2010-07-23 14:23:26]

If you see nothing bad in the changes to test server's public interface, I
guess it's fine. It's also used by UI tests, so I also included you in the
review.

On Fri, Jul 23, 2010 at 00:09, Huan Ren <huanr@google.com> wrote:

> Pawel, anything you want me to look at particularly in addition to Eric's
> review?
>
> Huan
>
>
> On Thu, Jul 22, 2010 at 12:15 PM, <eroman@chromium.org> wrote:
>
>> LGTM
>>
>>
>>
>> http://codereview.chromium.org/2881028/diff/1/28
>> File net/url_request/url_request_unittest.cc (right):
>>
>> http://codereview.chromium.org/2881028/diff/1/28#newcode290
>> net/url_request/url_request_unittest.cc:290: URLRequest* request = new
>> URLRequest(url, delegate);
>> On 2010/07/22 19:01:52, Paweł Hajdan Jr. wrote:
>>
>>> On 2010/07/22 03:14:51, eroman wrote:
>>> > Who is responsible for deleting |request| ?
>>> >
>>> > I would expect that |delegate| must have to delete |request|,
>>>
>> however looking
>>
>>> at
>>> > the caller it uses SyncTestDelegate and that does not seem to delete
>>>
>> the
>>
>>> > request.
>>> > I looked at the caller of this, and it uses
>>>
>>
>>  See the DeleteSoon call in OnResponseCompleted above. Yeah, I was also
>>>
>> slightly
>>
>>> confused by this. Can it be done in a more obvious way?
>>>
>>
>> Oh d'uh, how did I not see the DeleteSoon! I think my eyes must have
>> been scanning for a "delete" token :)
>>
>>
>> http://codereview.chromium.org/2881028/show
>>
>
>

[huanr, 2010-07-23 19:10:08]

LGTM.

reviewed test_server.{h,cc}