testserver.py listens on ephemeral ports by default.

testserver.py listens on ephemeral ports by default.

If --port is specified on the command line, testserver.py will listen on that port, otherwise it will listen on an ephemeral port. If --startup_pipe is specified, the port number is written to the pipe as a 2 byte unsigned int in host order.

TestServer spawns testserver.py to listen on an ephemeral port and reads the port value from the pipe. A fixed port can not be specified.

BUG=56814
TEST=try bots pass

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

[cbentzel, 2010-10-29 20:24:38]

I ended up reverting the initial ephemeral port change because of flakiness with
the SSLUI tests. Now that I uncovered the root reason for those being flaky
(they still used fixed ports), may as well continue with this.

This is the same CL as before, but merged with trunk. I'll probably need to
remerge if Pawel gets the Windows pipe-read timeout landed before this.

[Paweł Hajdan Jr., 2010-11-02 07:05:00]

Sorry for late review. Yeah, my Windows change is now landed so you can remerge.

http://codereview.chromium.org/4136008/diff/2001/3001
File net/test/test_server.cc (right):

http://codereview.chromium.org/4136008/diff/2001/3001#newcode88
net/test/test_server.cc:88: : type_(type), started_(false) {
nit: Style guide says those should be on separate lines.

http://codereview.chromium.org/4136008/diff/2001/3001#newcode94
net/test/test_server.cc:94: : https_options_(https_options), type_(TYPE_HTTPS),
started_(false) {
nit: Same here.

http://codereview.chromium.org/4136008/diff/2001/3001#newcode106
net/test/test_server.cc:106: host_port_pair_ = HostPortPair(GetHostname(type_,
https_options_), 0);
nit: Document the intention of putting 0 here.

http://codereview.chromium.org/4136008/diff/2001/3001#newcode108
net/test/test_server.cc:108: started_ = false;
nit: Is this redundant? If not, why is it needed?

http://codereview.chromium.org/4136008/diff/2001/3003
File net/test/test_server_posix.cc (right):

http://codereview.chromium.org/4136008/diff/2001/3003#newcode113
net/test/test_server_posix.cc:113: TestTimeouts::action_max_timeout_ms()));
The entire operation has a timeout of action_max_timeout. We should calculate
the timeout at the beginning, and subtract the amount of time spent in poll.

Otherwise the effective timeout would be action_max_timeout * number_of_reads.
In net_unittests context, 15 seconds vs 30 vs 45 is a meaningful difference.

http://codereview.chromium.org/4136008/diff/2001/3005
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/4136008/diff/2001/3005#newcode1243
net/tools/testserver/testserver.py:1243: option_parser.add_option('', '--port',
default='0', type='int',
Do we need --port at all now?

[cbentzel, 2010-11-06 18:06:55]

Thanks for the latest round of comments. Sorry for delay in between rounds.

http://codereview.chromium.org/4136008/diff/2001/3001
File net/test/test_server.cc (right):

http://codereview.chromium.org/4136008/diff/2001/3001#newcode88
net/test/test_server.cc:88: : type_(type), started_(false) {
On 2010/11/02 07:05:01, Paweł Hajdan Jr. wrote:
> nit: Style guide says those should be on separate lines.

Done.

http://codereview.chromium.org/4136008/diff/2001/3001#newcode94
net/test/test_server.cc:94: : https_options_(https_options), type_(TYPE_HTTPS),
started_(false) {
On 2010/11/02 07:05:01, Paweł Hajdan Jr. wrote:
> nit: Same here.

Done.

http://codereview.chromium.org/4136008/diff/2001/3001#newcode106
net/test/test_server.cc:106: host_port_pair_ = HostPortPair(GetHostname(type_,
https_options_), 0);
On 2010/11/02 07:05:01, Paweł Hajdan Jr. wrote:
> nit: Document the intention of putting 0 here.

Done.

http://codereview.chromium.org/4136008/diff/2001/3001#newcode108
net/test/test_server.cc:108: started_ = false;
On 2010/11/02 07:05:01, Paweł Hajdan Jr. wrote:
> nit: Is this redundant? If not, why is it needed?

Redundant, and removed.

http://codereview.chromium.org/4136008/diff/2001/3003
File net/test/test_server_posix.cc (right):

http://codereview.chromium.org/4136008/diff/2001/3003#newcode113
net/test/test_server_posix.cc:113: TestTimeouts::action_max_timeout_ms()));
On 2010/11/02 07:05:01, Paweł Hajdan Jr. wrote:
> The entire operation has a timeout of action_max_timeout. We should calculate
> the timeout at the beginning, and subtract the amount of time spent in poll.
> 
> Otherwise the effective timeout would be action_max_timeout * number_of_reads.
> In net_unittests context, 15 seconds vs 30 vs 45 is a meaningful difference.

Good point. done.

http://codereview.chromium.org/4136008/diff/2001/3005
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/4136008/diff/2001/3005#newcode1243
net/tools/testserver/testserver.py:1243: option_parser.add_option('', '--port',
default='0', type='int',
On 2010/11/02 07:05:01, Paweł Hajdan Jr. wrote:
> Do we need --port at all now?

I considered removing it, but retained it in this CL in case people ran
testserver.py from the command line and needed this functionality (I do this
occasionally). 

I think it makes sense to remove it, but I'd prefer to do it in a separate CL,
so this one won't have to be rolled back if people still want that
functionality.

[Paweł Hajdan Jr., 2010-11-08 10:38:41]

LGTM

[Sam Kerner (Chrome), 2010-11-08 13:53:21]

LGTM