Add '--host' option to testserver.py and expose it via a new TestServer constructor.

Add '--host' option to testserver.py and expose it via a new TestServer constructor.

By default the value 127.0.0.1 is used (this is also the default if no option is passed to the testserver.py script). For HTTPS cert mismatch tests, the value localhost is used. This causes the server to listen on localhost/127.0.0.1 but causes the client to expect a cert matching localhost (this corresponds to the previous behaviour).

BUG=114369
TEST=net_unittests still pass

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

[erikwright (departed), 2012-02-15 15:48:51]

Depends on this change to testserver.py: http://codereview.chromium.org/9368030/

See also this change to expose a static host parameter for the tests in
question:
http://codereview.chromium.org/9368031/

Two more changes just need some tidying up (to consult that parameter in
url_request_unittest.cc and to set the parameter during chrome_frame_net_tests
initialization).

Motivation is mysterious failures in chrome_frame_net_tests when they are run
over loopback adapter. These failures go away when another adapter is used.

The failure has been localized to somewhere between socket.read() in python and
ws2_32!send in urlmon (via Internet Explorer). Further diagnosis will probably
require kernel debugging.

This workaround fixes the issue (dozens of try jobs without encountering the
failure, which otherwise occurred nearly every run) and will bring us closer to
getting the chrome_frame_net_tests back on the waterfall.

[Paweł Hajdan Jr., 2012-02-15 17:13:09]

Why is this separate from http://codereview.chromium.org/9368030/ ?

Please put both changes in the same CL, it's going to be _easier_ to review.

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

http://codereview.chromium.org/9369029/diff/1/net/test/test_server.h#newcode116
net/test/test_server.h:116: // Initialize a TestServer listening on a specific
host (IP or hostname).
Please don't add a 3rd ctor. Fit it into existing one, and change callers.

Rationale: simplicity.

[akalin, 2012-02-15 23:20:04]

leaving this review to pawel, too

[erikwright (departed), 2012-02-16 14:31:00]

I will combine the CLs, according to your preference.

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

http://codereview.chromium.org/9369029/diff/1/net/test/test_server.h#newcode116
net/test/test_server.h:116: // Initialize a TestServer listening on a specific
host (IP or hostname).
On 2012/02/15 17:13:10, Paweł Hajdan Jr. wrote:
> Please don't add a 3rd ctor. Fit it into existing one, and change callers.
> 
> Rationale: simplicity.

This would touch something on the order of 64 files according to git grep. I
have reservations about that, since it means 64 files will now need to
explicitly declare something that really is the default, whereas only one file
needs to do anything different.

Also, the medium term hope is that we will sort out the underlying problem and
we will be able to run the chrome_frame_net_tests over loopback again, in which
case we would possibly want to remove this functionality.

I see three options:

1) Add a third constructor.
2) Add a setter, instead. It violates the existing conventions for the class,
but it doesn't bother me that much.
3) Temporarily add a third constructor, submit one or more CLs to update the
existing clients to use it, and then submit a final CL to remove the old
constructor.

If not adding a 3rd constructor is your strong preference, I will get all the
CLs for (3) lined up before the first commit. I would ask your preference for
how the 64 files should specify localhost:

a) empty string
b) literal 127.0.0.1
c) define a net::TestServer::kLocalHost

A quick query did not reveal an existing constant for localhost.

[erikwright (departed), 2012-02-16 15:01:05]

I have combined the CLs here. PTAL.

[Paweł Hajdan Jr., 2012-02-16 15:43:48]

On 2012/02/16 14:31:00, erikwright wrote:
> This would touch something on the order of 64 files according to git grep. I
> have reservations about that, since it means 64 files will now need to
> explicitly declare something that really is the default, whereas only one file
> needs to do anything different.

I'm not sure if that's really 64. Anyway, I strongly prefer simplicity of
TestServer's public interface over other factors.

> Also, the medium term hope is that we will sort out the underlying problem and
> we will be able to run the chrome_frame_net_tests over loopback again, in
which
> case we would possibly want to remove this functionality.

Those temporary fixes for mysterious problems ofter "stick" for a long time.

> I see three options:
> 
> 1) Add a third constructor.

Nack.

> 2) Add a setter, instead. It violates the existing conventions for the class,
> but it doesn't bother me that much.

Same here.

> 3) Temporarily add a third constructor, submit one or more CLs to update the
> existing clients to use it, and then submit a final CL to remove the old
> constructor.

I'm fine with that, but it should be possible to do in one CL (those TestServer
invocations rarely change). But if you prefer 3) I'm fine.

> If not adding a 3rd constructor is your strong preference, I will get all the
> CLs for (3) lined up before the first commit. I would ask your preference for
> how the 64 files should specify localhost:
> 
> a) empty string
> b) literal 127.0.0.1
> c) define a net::TestServer::kLocalHost

I'm fine with either b) or c), just please make it consistent.

[erikwright (departed), 2012-02-21 20:30:04]

Hi Pawel,

I have uploaded two follow-up CLs that update clients and then remove the extra
constructor:

http://codereview.chromium.org/9372065/
http://codereview.chromium.org/9431002/

It seems that they are good with the exception of needing to fix the pyauto
wrappers. While I'm sorting that out, do you mind giving an LGTM on this CL?

Thanks,

Erik

On 2012/02/16 15:43:48, Paweł Hajdan Jr. wrote:
> On 2012/02/16 14:31:00, erikwright wrote:
> > This would touch something on the order of 64 files according to git grep. I
> > have reservations about that, since it means 64 files will now need to
> > explicitly declare something that really is the default, whereas only one
file
> > needs to do anything different.
> 
> I'm not sure if that's really 64. Anyway, I strongly prefer simplicity of
> TestServer's public interface over other factors.
> 
> > Also, the medium term hope is that we will sort out the underlying problem
and
> > we will be able to run the chrome_frame_net_tests over loopback again, in
> which
> > case we would possibly want to remove this functionality.
> 
> Those temporary fixes for mysterious problems ofter "stick" for a long time.
> 
> > I see three options:
> > 
> > 1) Add a third constructor.
> 
> Nack.
> 
> > 2) Add a setter, instead. It violates the existing conventions for the
class,
> > but it doesn't bother me that much.
> 
> Same here.
> 
> > 3) Temporarily add a third constructor, submit one or more CLs to update the
> > existing clients to use it, and then submit a final CL to remove the old
> > constructor.
> 
> I'm fine with that, but it should be possible to do in one CL (those
TestServer
> invocations rarely change). But if you prefer 3) I'm fine.
> 
> > If not adding a 3rd constructor is your strong preference, I will get all
the
> > CLs for (3) lined up before the first commit. I would ask your preference
for
> > how the 64 files should specify localhost:
> > 
> > a) empty string
> > b) literal 127.0.0.1
> > c) define a net::TestServer::kLocalHost
> 
> I'm fine with either b) or c), just please make it consistent.

[Paweł Hajdan Jr., 2012-02-22 08:21:14]

Looks like this CL did not receive any of the fixes I asked for.

Feel free to send those two mentioned CLs for review.

[erikwright (departed), 2012-02-22 11:31:59]

On 2012/02/22 08:21:14, Paweł Hajdan Jr. wrote:
> Looks like this CL did not receive any of the fixes I asked for.
> 
> Feel free to send those two mentioned CLs for review.

I am not sure what fixes you are referring to. My understanding was that the
only fix asked for was to not add a constructor, and I proposed a three-step
process:

1. Add the new constructor
2. Update clients
3. Remove the old constructor.

This CL is step 1, steps 2 and 3 are here:

http://codereview.chromium.org/9372065/
http://codereview.chromium.org/9431002/

I have sent them to you for review. Please let me know if further changes are
needed to this CL before it is submitted as the first step.

[Paweł Hajdan Jr., 2012-02-22 14:41:26]

LGTM

[commit-bot: I haz the power, 2012-02-22 14:58:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/9369029/6001

[commit-bot: I haz the power, 2012-02-22 15:20:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/9369029/15001

[commit-bot: I haz the power, 2012-02-22 16:41:13]

Change committed as 123040

[eroman, 2012-02-23 20:51:44]

http://codereview.chromium.org/9369029/diff/15001/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/9369029/diff/15001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:1919: print 'HTTPS server started on
%s:%d...' % (host, server.server_port)
note that if host is an IPv6 literal, this host:port concatenation won't look
correct.

[eroman, 2012-02-23 20:55:07]

http://codereview.chromium.org/9369029/diff/15001/net/test/test_server.h
File net/test/test_server.h (right):

http://codereview.chromium.org/9369029/diff/15001/net/test/test_server.h#newc...
net/test/test_server.h:114: static const char* kLocalhost;
Technically this is ambiguous, since "localhost" might also mean ::1

No change required, just saying.