[sync] Add --port and --xmpp-port parameters to run_testserver.cc

[sync] Add --port and --xmpp-port parameters to run_testserver.cc

run_testserver is a useful test utility that launches a python
testserver after setting the required python path.

When it is used to start a sync testserver, the only parameter it
supports is --sync, indicating that we want a sync server. It doesn't
support the --port and --xmpp-port flags, that are used to specify the
ports on which we want the sync server and sync xmpp server to run.

This patch adds the parameters to run_testserver.cc and plumbs them down
to the TestServer objects created by run_testserver.cc. It also separates out
sync specific testserver code into a new class called LocalSyncTestServer.

Separating the sync server in python code is out of the scope of this CL
because it requires significant refactoring, since it is in use on multiple
platforms, and will be dealt with separately.

BUG=117559
TEST=Build and run run_testserver with the paramters --sync, --port and --xmpp-port

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

[Raghu Simha, 2012-05-18 23:38:03]

Guys, please review. Thanks.

akalin: changes to net/tools/testserver
phajdan.jr: changes to net/test

[akalin, 2012-05-19 00:24:19]

On 2012/05/18 23:38:03, rsimha wrote:
> Guys, please review. Thanks.
> 
> akalin: changes to net/tools/testserver
> phajdan.jr: changes to net/test

Hmm what's the use case for this?  I thought the testserver communicated back to
the test process the xmpp port already.

[Raghu Simha, 2012-05-19 01:54:19]

On 2012/05/19 00:24:19, akalin wrote:
> Hmm what's the use case for this?  I thought the testserver communicated back
to
> the test process the xmpp port already.

The use case is for when you want to manually specify ports to
run_testserver.exe. Right now, it is a pain to use the python server for manual
testing because it uses a different port each time you run it, and you have to
go in and change the flags passed in to chrome each time.

I can provide you more details offline, about why this is necessary.

[Paweł Hajdan Jr., 2012-05-21 08:36:46]

http://codereview.chromium.org/10388206/diff/8001/net/test/base_test_server.h
File net/test/base_test_server.h (right):

http://codereview.chromium.org/10388206/diff/8001/net/test/base_test_server.h...
net/test/base_test_server.h:174: void SetXmppPort(uint16 xmpp_port);
Please stop this. I asked you people to extract the XMPP bits to a separate
class, both here and in Python code. How about doing that now?

[Raghu Simha, 2012-05-23 02:25:10]

Folks, PTAL.

mmenke: net.gyp only.
phajdan.jr: base_test_server.h only.
akalin: everything else.

http://codereview.chromium.org/10388206/diff/8001/net/test/base_test_server.h
File net/test/base_test_server.h (right):

http://codereview.chromium.org/10388206/diff/8001/net/test/base_test_server.h...
net/test/base_test_server.h:174: void SetXmppPort(uint16 xmpp_port);
On 2012/05/21 08:36:46, Paweł Hajdan Jr. wrote:
> Please stop this. I asked you people to extract the XMPP bits to a separate
> class

Your language here puzzles me. Whom do you mean by "you people" and what are you
suggesting I stop doing? This is the first time I've written a CL that touches
this file. If you wanted to be helpful and offer constructive feedback, a link
to a previous CL would have helped. Your accusatory tone seems somewhat
childish. My 2 cents, FWIW.

> both here and in Python code. How about doing that now?

I had to spend some time digging into the history of this file.

The "here" part is easy. I've separated out the python sync launching code into
a subclass called LocalSyncTestServer. There is no more sync specific stuff in
base_test_server.cc.

The "python" part is harder -- significantly harder. We use testserver.py on
several platforms, and separating things here will cause a whole lot of breakage
all over the place. I've made a note of this in the description and linked to
the bug for this, and plan on dealing with it when I work on extracting the sync
python server later this quarter.

[mmenke, 2012-05-23 03:01:31]

On 2012/05/23 02:25:10, rsimha wrote:
> Folks, PTAL.
> 
> mmenke: net.gyp only.
> phajdan.jr: base_test_server.h only.
> akalin: everything else.
> 
> http://codereview.chromium.org/10388206/diff/8001/net/test/base_test_server.h
> File net/test/base_test_server.h (right):
> 
>
http://codereview.chromium.org/10388206/diff/8001/net/test/base_test_server.h...
> net/test/base_test_server.h:174: void SetXmppPort(uint16 xmpp_port);
> On 2012/05/21 08:36:46, Paweł Hajdan Jr. wrote:
> > Please stop this. I asked you people to extract the XMPP bits to a separate
> > class
> 
> Your language here puzzles me. Whom do you mean by "you people" and what are
you
> suggesting I stop doing? This is the first time I've written a CL that touches
> this file. If you wanted to be helpful and offer constructive feedback, a link
> to a previous CL would have helped. Your accusatory tone seems somewhat
> childish. My 2 cents, FWIW.
> 
> > both here and in Python code. How about doing that now?
> 
> I had to spend some time digging into the history of this file.
> 
> The "here" part is easy. I've separated out the python sync launching code
into
> a subclass called LocalSyncTestServer. There is no more sync specific stuff in
> base_test_server.cc.
> 
> The "python" part is harder -- significantly harder. We use testserver.py on
> several platforms, and separating things here will cause a whole lot of
breakage
> all over the place. I've made a note of this in the description and linked to
> the bug for this, and plan on dealing with it when I work on extracting the
sync
> python server later this quarter.

Why is sync-specific testing code living in net/?  Just convenience, or are
there classes in net/ it's also needed to test?  Is the plan to move it into
chrome/tools/ when http://crbug.com/117559 is resolved?

Not going to hold up the codereview on this, just want a better understanding of
what the plan is.

[akalin, 2012-05-23 03:12:08]

> Why is sync-specific testing code living in net/?  Just convenience, or are
> there classes in net/ it's also needed to test?  Is the plan to move it into
> chrome/tools/ when http://crbug.com/117559 is resolved?
> 
> Not going to hold up the codereview on this, just want a better understanding
of
> what the plan is.

Just accretion over time.  Originally, the sync server was just adding a few
handlers to the net testserver, so it was the most practical thing to do.

But now that the sync server's grown somewhat (adding an XMPP server, etc.) I do
agree that it should be split out.  It would probably live in sync/ somewhere
when it is.

[mmenke, 2012-05-23 03:21:16]

On 2012/05/23 03:12:08, akalin wrote:
> > Why is sync-specific testing code living in net/?  Just convenience, or are
> > there classes in net/ it's also needed to test?  Is the plan to move it into
> > chrome/tools/ when http://crbug.com/117559 is resolved?
> > 
> > Not going to hold up the codereview on this, just want a better
understanding
> of
> > what the plan is.
> 
> Just accretion over time.  Originally, the sync server was just adding a few
> handlers to the net testserver, so it was the most practical thing to do.
> 
> But now that the sync server's grown somewhat (adding an XMPP server, etc.) I
do
> agree that it should be split out.  It would probably live in sync/ somewhere
> when it is.

SGTM, thanks.  net/net.gyp LGTM.

[Raghu Simha, 2012-05-23 05:09:59]

On 2012/05/23 03:01:31, Matt Menke wrote:
> Why is sync-specific testing code living in net/?  Just convenience, or are
> there classes in net/ it's also needed to test?

As akalin@ said, the sync test server started small and has grown over time. I
could have just as easily added local_sync_test_server.{h|cc} under
browser/sync/test, but I chose net/test because the server that it launches
lives under net/tools/testserver and is tightly coupled with testserver.py,
which is for general purpose testing.

> Is the plan to move it into
> chrome/tools/ when http://crbug.com/117559 is resolved?

Yes, the plan is to move it into a directory that makes sense... chrome/tools or
maybe even chrome/browser/sync/test. I definitely agree with Pawel and Fred that
the sync portion of testserver is big enough to live on its own.

> Not going to hold up the codereview on this, just want a better understanding
of
> what the plan is.

Thanks for the review :)

[akalin, 2012-05-23 05:29:21]

LGTM after nits.

http://codereview.chromium.org/10388206/diff/8032/net/test/local_sync_test_se...
File net/test/local_sync_test_server.h (right):

http://codereview.chromium.org/10388206/diff/8032/net/test/local_sync_test_se...
net/test/local_sync_test_server.h:32: virtual void CleanUpWhenStoppingServer()
OVERRIDE;
#include base/compiler_specific.h for OVERRIDE

http://codereview.chromium.org/10388206/diff/8032/net/test/local_sync_test_se...
net/test/local_sync_test_server.h:38:
DISALLOW_COPY_AND_ASSIGN(LocalSyncTestServer);
#include base/basictypes.h for this

http://codereview.chromium.org/10388206/diff/8032/net/tools/testserver/run_te...
File net/tools/testserver/run_testserver.cc (right):

http://codereview.chromium.org/10388206/diff/8032/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:94: std::string port_str = "0";
move port_str inside the if statement and don't initialize it to some value. 
Also maybe do something if the conversion fails? (log + use default value?) 
Something like:

int port = 0;
if (command_line->HasSwitch("port")) {
  const std::string& port_str = command_line->GetSwitchValueASCII("port");
  if (!base::StringToInt(port_str, &port)) {
    port = 0;
    LOG(WARNING) << "...";
  }
}

you may want to decomp it into a helper function since you use it twice, also.

http://codereview.chromium.org/10388206/diff/8032/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:140: test_server.reset(new
net::LocalSyncTestServer((uint16) port_int,
prefer static_cast<uint16>()

http://codereview.chromium.org/10388206/diff/8032/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:140: test_server.reset(new
net::LocalSyncTestServer((uint16) port_int,
in fact, you can probably stick the int parsing code here since it's only needed
for sync stuff.  you may need to put braces around the case block (since you're
defining new variables).  Or decomp it out to a separate function.

[Raghu Simha, 2012-05-23 06:02:58]

akalin: Nits fixed. Thanks for the review.
phajdan.jr: PTAL.

http://codereview.chromium.org/10388206/diff/8032/net/test/local_sync_test_se...
File net/test/local_sync_test_server.h (right):

http://codereview.chromium.org/10388206/diff/8032/net/test/local_sync_test_se...
net/test/local_sync_test_server.h:32: virtual void CleanUpWhenStoppingServer()
OVERRIDE;
On 2012/05/23 05:29:21, akalin wrote:
> #include base/compiler_specific.h for OVERRIDE

Done.

http://codereview.chromium.org/10388206/diff/8032/net/test/local_sync_test_se...
net/test/local_sync_test_server.h:38:
DISALLOW_COPY_AND_ASSIGN(LocalSyncTestServer);
On 2012/05/23 05:29:21, akalin wrote:
> #include base/basictypes.h for this

Done.

http://codereview.chromium.org/10388206/diff/8032/net/tools/testserver/run_te...
File net/tools/testserver/run_testserver.cc (right):

http://codereview.chromium.org/10388206/diff/8032/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:94: std::string port_str = "0";
On 2012/05/23 05:29:21, akalin wrote:
> move port_str inside the if statement and don't initialize it to some value. 
> Also maybe do something if the conversion fails? (log + use default value?) 
> Something like:
> 
> int port = 0;
> if (command_line->HasSwitch("port")) {
>   const std::string& port_str = command_line->GetSwitchValueASCII("port");
>   if (!base::StringToInt(port_str, &port)) {
>     port = 0;
>     LOG(WARNING) << "...";
>   }
> }
> 
> you may want to decomp it into a helper function since you use it twice, also.

Done.

http://codereview.chromium.org/10388206/diff/8032/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:140: test_server.reset(new
net::LocalSyncTestServer((uint16) port_int,
On 2012/05/23 05:29:21, akalin wrote:
> prefer static_cast<uint16>()

Done.

http://codereview.chromium.org/10388206/diff/8032/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:140: test_server.reset(new
net::LocalSyncTestServer((uint16) port_int,
On 2012/05/23 05:29:21, akalin wrote:
> in fact, you can probably stick the int parsing code here since it's only
needed
> for sync stuff.  you may need to put braces around the case block (since
you're
> defining new variables).  Or decomp it out to a separate function.

Done. Moved to a separate function. Added braces in any case.

[akalin, 2012-05-23 06:16:49]

feel free to take care of these in a future CL

http://codereview.chromium.org/10388206/diff/2007/net/test/local_sync_test_se...
File net/test/local_sync_test_server.cc (right):

http://codereview.chromium.org/10388206/diff/2007/net/test/local_sync_test_se...
net/test/local_sync_test_server.cc:38:
BaseTestServer::CleanUpWhenStoppingServer();
why not LocalTestServer:: here instead? (even if it doesn't explicitly override
that method, it will simply fall back to the BaseTestServer:: version. if it
does override it in the future, that may break this class in subtle ways.)

if you do remove this, make sure you remove the test_server.h include, too.

http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
File net/tools/testserver/run_testserver.cc (right):

http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:136: case net::TestServer::TYPE_HTTPS: {
usual style is to leave the braces out unless necessary, but it's not a big deal

[Paweł Hajdan Jr., 2012-05-23 06:53:02]

+jnd, is this going to possibly break Android's RemoteTestServer?

This is better.

http://codereview.chromium.org/10388206/diff/2007/net/test/OWNERS
File net/test/OWNERS (right):

http://codereview.chromium.org/10388206/diff/2007/net/test/OWNERS#newcode5
net/test/OWNERS:5: akalin@chromium.org
I'm not sure if it's worth it just for two small files. How about removing it
for now and adding it later when the need is more obvious?

http://codereview.chromium.org/10388206/diff/2007/net/test/base_test_server.h
File net/test/base_test_server.h (right):

http://codereview.chromium.org/10388206/diff/2007/net/test/base_test_server.h...
net/test/base_test_server.h:185: virtual void CleanUpWhenStoppingServer();
Please find a way to avoid making this virtual. Once we allow protected virtual
methods the contract between classes becomes more complex and fragile.

Would preventing the server from being started again after it's stopped break
anything in the codebase? If not, we could do that, and remove
CleanUpWhenStoppingServer almost completely, at least zeroing-out int member
variables.

http://codereview.chromium.org/10388206/diff/2007/net/test/base_test_server.h...
net/test/base_test_server.h:197: virtual bool
GenerateArguments(base::DictionaryValue* arguments) const;
Do not override this. See how LocalTestServer handles that.

http://codereview.chromium.org/10388206/diff/2007/net/test/local_sync_test_se...
File net/test/local_sync_test_server.cc (right):

http://codereview.chromium.org/10388206/diff/2007/net/test/local_sync_test_se...
net/test/local_sync_test_server.cc:38:
BaseTestServer::CleanUpWhenStoppingServer();
On 2012/05/23 06:16:49, akalin wrote:
> why not LocalTestServer:: here instead? (even if it doesn't explicitly
override
> that method, it will simply fall back to the BaseTestServer:: version. if it
> does override it in the future, that may break this class in subtle ways.)
> 
> if you do remove this, make sure you remove the test_server.h include, too.

+1

http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
File net/tools/testserver/run_testserver.cc (right):

http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:14: #include
"base/string_number_conversions.h"
nit: Sort.

http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:18: #include
"net/test/local_sync_test_server.h"
nit: Sort.

http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:25: printf("       --port and --xmpp-port
only work with the --sync flag.)\n");
Please add code to error out when those switches are used without --sync flag.
This way we're going to avoid a confusion when they're silently ignored.

http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:65: LOG(WARNING) << "Could not extract
port from switch " << switch_name;
How about making the function return bool success, and return port via
out-parameter? That would make the success unambiguous.

http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:136: case net::TestServer::TYPE_HTTPS: {
On 2012/05/23 06:16:49, akalin wrote:
> usual style is to leave the braces out unless necessary, but it's not a big
deal

+1, please do

[Raghu Simha, 2012-05-23 19:54:27]

jnd: This doesn't change BaseTestServer or RemoteTestServer in any way, so I
believe the android tests should be unaffected.

phajdan.jr: I've addressed all your comments. PTAL. Also, I think it's worth
mentioning that the primary purpose of this CL is to make the use of
run_testserver with --sync easier, and is targeted at aiding the development of
chrome prototype work that uses run_testserver with --sync. If you have larger
changes in mind resulting from this CL, we're better off dealing with them at a
later point without blocking the forward progress of the main purpose of this
CL.

http://codereview.chromium.org/10388206/diff/2007/net/test/OWNERS
File net/test/OWNERS (right):

http://codereview.chromium.org/10388206/diff/2007/net/test/OWNERS#newcode5
net/test/OWNERS:5: akalin@chromium.org
On 2012/05/23 06:53:02, Paweł Hajdan Jr. wrote:
> I'm not sure if it's worth it just for two small files. How about removing it
> for now and adding it later when the need is more obvious?

The plan is to add it now and remove it later, when the sync stuff moves out of
net/. This is in keeping with the OWNERS file in net/tools/testserver.

http://codereview.chromium.org/10388206/diff/2007/net/test/base_test_server.h
File net/test/base_test_server.h (right):

http://codereview.chromium.org/10388206/diff/2007/net/test/base_test_server.h...
net/test/base_test_server.h:185: virtual void CleanUpWhenStoppingServer();
On 2012/05/23 06:53:02, Paweł Hajdan Jr. wrote:
> Please find a way to avoid making this virtual. Once we allow protected
virtual
> methods the contract between classes becomes more complex and fragile.
> 
> Would preventing the server from being started again after it's stopped break
> anything in the codebase? If not, we could do that, and remove
> CleanUpWhenStoppingServer almost completely, at least zeroing-out int member
> variables.

Done. 

The sync server is only started once in every place it is used, so
CleanUpWhenStoppingServer is moot when it comes to sync. I've undone this
change.

Changes to how CleanUpWhenStoppingServer works (including its removal) is out of
the scope of this patch.

http://codereview.chromium.org/10388206/diff/2007/net/test/base_test_server.h...
net/test/base_test_server.h:197: virtual bool
GenerateArguments(base::DictionaryValue* arguments) const;
On 2012/05/23 06:53:02, Paweł Hajdan Jr. wrote:
> Do not override this. See how LocalTestServer handles that.

Done.

There are two sets of arguments being strung together here -- the ones passed to
python.exe and the ones passed to testserver.py.
BaseTestServer::GenerateArguments takes care of the args passed to testserver.py
while LocalTestServer::AddCommandLineArguments takes care of the ones passed to
python.exe and then calls AppendArg for the stuff returned by
BaseTestServer::GenerateArguments. (AppendSwitchASCII won't work because the
args will then get passed to python.exe, not testserver.py)

The only way to avoid making BaseTestServer::GenerateArguments virtual is to
make LocalTestServer::AddCommandLineArguments public virtual and call it from
the override, LocalSyncTestServer::AddCommandLineArguments.

http://codereview.chromium.org/10388206/diff/2007/net/test/local_sync_test_se...
File net/test/local_sync_test_server.cc (right):

http://codereview.chromium.org/10388206/diff/2007/net/test/local_sync_test_se...
net/test/local_sync_test_server.cc:38:
BaseTestServer::CleanUpWhenStoppingServer();
On 2012/05/23 06:53:02, Paweł Hajdan Jr. wrote:
> On 2012/05/23 06:16:49, akalin wrote:
> > why not LocalTestServer:: here instead? (even if it doesn't explicitly
> override
> > that method, it will simply fall back to the BaseTestServer:: version. if it
> > does override it in the future, that may break this class in subtle ways.)
> > 
> > if you do remove this, make sure you remove the test_server.h include, too.
> 
> +1

Moot.

http://codereview.chromium.org/10388206/diff/2007/net/test/local_sync_test_se...
net/test/local_sync_test_server.cc:38:
BaseTestServer::CleanUpWhenStoppingServer();
On 2012/05/23 06:16:49, akalin wrote:
> why not LocalTestServer:: here instead? (even if it doesn't explicitly
override
> that method, it will simply fall back to the BaseTestServer:: version. if it
> does override it in the future, that may break this class in subtle ways.)

Good point. It's moot, though.
 
> if you do remove this, make sure you remove the test_server.h include, too.

The include was necessary for TYPE_SYNC and kLocalhost.

http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
File net/tools/testserver/run_testserver.cc (right):

http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:14: #include
"base/string_number_conversions.h"
On 2012/05/23 06:53:02, Paweł Hajdan Jr. wrote:
> nit: Sort.

Done.

http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:18: #include
"net/test/local_sync_test_server.h"
On 2012/05/23 06:53:02, Paweł Hajdan Jr. wrote:
> nit: Sort.

Done.

http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:25: printf("       --port and --xmpp-port
only work with the --sync flag.)\n");
On 2012/05/23 06:53:02, Paweł Hajdan Jr. wrote:
> Please add code to error out when those switches are used without --sync flag.
> This way we're going to avoid a confusion when they're silently ignored.

Done.

http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:65: LOG(WARNING) << "Could not extract
port from switch " << switch_name;
On 2012/05/23 06:53:02, Paweł Hajdan Jr. wrote:
> How about making the function return bool success, and return port via
> out-parameter? That would make the success unambiguous.

Done.

http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:136: case net::TestServer::TYPE_HTTPS: {
On 2012/05/23 06:16:49, akalin wrote:
> usual style is to leave the braces out unless necessary, but it's not a big
deal

Moot.

http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:136: case net::TestServer::TYPE_HTTPS: {
On 2012/05/23 06:53:02, Paweł Hajdan Jr. wrote:
> On 2012/05/23 06:16:49, akalin wrote:
> > usual style is to leave the braces out unless necessary, but it's not a big
> deal
> 
> +1, please do

Moot.

[Johnny(Jianning) Ding, 2012-05-24 03:22:49]

@rsimha, yes, you change should not break android net server based tests.

I just have a question, why you need to launch sync server in a specific port
instead of letting Python socket pick up an available/ephemeral port and send
back to you?
Android net server based tests used to use some specific fixed ports to launch
Python test server. But Ryan told me that Chromium always uses the random
ephemeral port given by OS(via Python) to run the test server based tests in
order to make the test more robust, that is because sometimes you may encounter
weird issue when shutting down the server on the port. In that case, restarting
server on same port later may fail. So now the Android tests don't specify the
port when launching Python test server.


On 2012/05/23 19:54:27, rsimha wrote:
> jnd: This doesn't change BaseTestServer or RemoteTestServer in any way, so I
> believe the android tests should be unaffected.
> 
> phajdan.jr: I've addressed all your comments. PTAL. Also, I think it's worth
> mentioning that the primary purpose of this CL is to make the use of
> run_testserver with --sync easier, and is targeted at aiding the development
of
> chrome prototype work that uses run_testserver with --sync. If you have larger
> changes in mind resulting from this CL, we're better off dealing with them at
a
> later point without blocking the forward progress of the main purpose of this
> CL.
> 
> http://codereview.chromium.org/10388206/diff/2007/net/test/OWNERS
> File net/test/OWNERS (right):
> 
> http://codereview.chromium.org/10388206/diff/2007/net/test/OWNERS#newcode5
> net/test/OWNERS:5: mailto:akalin@chromium.org
> On 2012/05/23 06:53:02, Paweł Hajdan Jr. wrote:
> > I'm not sure if it's worth it just for two small files. How about removing
it
> > for now and adding it later when the need is more obvious?
> 
> The plan is to add it now and remove it later, when the sync stuff moves out
of
> net/. This is in keeping with the OWNERS file in net/tools/testserver.
> 
> http://codereview.chromium.org/10388206/diff/2007/net/test/base_test_server.h
> File net/test/base_test_server.h (right):
> 
>
http://codereview.chromium.org/10388206/diff/2007/net/test/base_test_server.h...
> net/test/base_test_server.h:185: virtual void CleanUpWhenStoppingServer();
> On 2012/05/23 06:53:02, Paweł Hajdan Jr. wrote:
> > Please find a way to avoid making this virtual. Once we allow protected
> virtual
> > methods the contract between classes becomes more complex and fragile.
> > 
> > Would preventing the server from being started again after it's stopped
break
> > anything in the codebase? If not, we could do that, and remove
> > CleanUpWhenStoppingServer almost completely, at least zeroing-out int member
> > variables.
> 
> Done. 
> 
> The sync server is only started once in every place it is used, so
> CleanUpWhenStoppingServer is moot when it comes to sync. I've undone this
> change.
> 
> Changes to how CleanUpWhenStoppingServer works (including its removal) is out
of
> the scope of this patch.
> 
>
http://codereview.chromium.org/10388206/diff/2007/net/test/base_test_server.h...
> net/test/base_test_server.h:197: virtual bool
> GenerateArguments(base::DictionaryValue* arguments) const;
> On 2012/05/23 06:53:02, Paweł Hajdan Jr. wrote:
> > Do not override this. See how LocalTestServer handles that.
> 
> Done.
> 
> There are two sets of arguments being strung together here -- the ones passed
to
> python.exe and the ones passed to testserver.py.
> BaseTestServer::GenerateArguments takes care of the args passed to
testserver.py
> while LocalTestServer::AddCommandLineArguments takes care of the ones passed
to
> python.exe and then calls AppendArg for the stuff returned by
> BaseTestServer::GenerateArguments. (AppendSwitchASCII won't work because the
> args will then get passed to python.exe, not testserver.py)
> 
> The only way to avoid making BaseTestServer::GenerateArguments virtual is to
> make LocalTestServer::AddCommandLineArguments public virtual and call it from
> the override, LocalSyncTestServer::AddCommandLineArguments.
> 
>
http://codereview.chromium.org/10388206/diff/2007/net/test/local_sync_test_se...
> File net/test/local_sync_test_server.cc (right):
> 
>
http://codereview.chromium.org/10388206/diff/2007/net/test/local_sync_test_se...
> net/test/local_sync_test_server.cc:38:
> BaseTestServer::CleanUpWhenStoppingServer();
> On 2012/05/23 06:53:02, Paweł Hajdan Jr. wrote:
> > On 2012/05/23 06:16:49, akalin wrote:
> > > why not LocalTestServer:: here instead? (even if it doesn't explicitly
> > override
> > > that method, it will simply fall back to the BaseTestServer:: version. if
it
> > > does override it in the future, that may break this class in subtle ways.)
> > > 
> > > if you do remove this, make sure you remove the test_server.h include,
too.
> > 
> > +1
> 
> Moot.
> 
>
http://codereview.chromium.org/10388206/diff/2007/net/test/local_sync_test_se...
> net/test/local_sync_test_server.cc:38:
> BaseTestServer::CleanUpWhenStoppingServer();
> On 2012/05/23 06:16:49, akalin wrote:
> > why not LocalTestServer:: here instead? (even if it doesn't explicitly
> override
> > that method, it will simply fall back to the BaseTestServer:: version. if it
> > does override it in the future, that may break this class in subtle ways.)
> 
> Good point. It's moot, though.
>  
> > if you do remove this, make sure you remove the test_server.h include, too.
> 
> The include was necessary for TYPE_SYNC and kLocalhost.
> 
>
http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
> File net/tools/testserver/run_testserver.cc (right):
> 
>
http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
> net/tools/testserver/run_testserver.cc:14: #include
> "base/string_number_conversions.h"
> On 2012/05/23 06:53:02, Paweł Hajdan Jr. wrote:
> > nit: Sort.
> 
> Done.
> 
>
http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
> net/tools/testserver/run_testserver.cc:18: #include
> "net/test/local_sync_test_server.h"
> On 2012/05/23 06:53:02, Paweł Hajdan Jr. wrote:
> > nit: Sort.
> 
> Done.
> 
>
http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
> net/tools/testserver/run_testserver.cc:25: printf("       --port and
--xmpp-port
> only work with the --sync flag.)\n");
> On 2012/05/23 06:53:02, Paweł Hajdan Jr. wrote:
> > Please add code to error out when those switches are used without --sync
flag.
> > This way we're going to avoid a confusion when they're silently ignored.
> 
> Done.
> 
>
http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
> net/tools/testserver/run_testserver.cc:65: LOG(WARNING) << "Could not extract
> port from switch " << switch_name;
> On 2012/05/23 06:53:02, Paweł Hajdan Jr. wrote:
> > How about making the function return bool success, and return port via
> > out-parameter? That would make the success unambiguous.
> 
> Done.
> 
>
http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
> net/tools/testserver/run_testserver.cc:136: case net::TestServer::TYPE_HTTPS:
{
> On 2012/05/23 06:16:49, akalin wrote:
> > usual style is to leave the braces out unless necessary, but it's not a big
> deal
> 
> Moot.
> 
>
http://codereview.chromium.org/10388206/diff/2007/net/tools/testserver/run_te...
> net/tools/testserver/run_testserver.cc:136: case net::TestServer::TYPE_HTTPS:
{
> On 2012/05/23 06:53:02, Paweł Hajdan Jr. wrote:
> > On 2012/05/23 06:16:49, akalin wrote:
> > > usual style is to leave the braces out unless necessary, but it's not a
big
> > deal
> > 
> > +1, please do
> 
> Moot.

[Raghu Simha, 2012-05-24 03:32:19]

On 2012/05/24 03:22:49, Johnny(Jianning) Ding wrote:
> @rsimha, yes, you change should not break android net server based tests.
> 
> I just have a question, why you need to launch sync server in a specific port
> instead of letting Python socket pick up an available/ephemeral port and send
> back to you?
> Android net server based tests used to use some specific fixed ports to launch
> Python test server. But Ryan told me that Chromium always uses the random
> ephemeral port given by OS(via Python) to run the test server based tests in
> order to make the test more robust, that is because sometimes you may
encounter
> weird issue when shutting down the server on the port. In that case,
restarting
> server on same port later may fail. So now the Android tests don't specify the
> port when launching Python test server.

What you say above is true. I do not plan on discontinuing the use of ephemeral
ports for chrome's sync integration tests. This CL is meant purely to ease the
manual running of run_testserver.exe, and is part of a larger effort that
utilizes the python sync server.