Spin up XMPP server for testservers of type sync.

Spin up XMPP server for testservers of type sync.

The XMPP server will be used by the sync integration tests in a future CL.

BUG=53934
TEST=Sync integration tests still work

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

[akalin, 2010-11-17 23:26:58]

+cbentzel for review

[cbentzel, 2010-11-18 11:51:49]

Added Pawel - do you want someone else on the sync team to also take a look?

[Paweł Hajdan Jr., 2010-11-18 11:58:45]

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

http://codereview.chromium.org/5104004/diff/1/net/tools/testserver/testserver...
net/tools/testserver/testserver.py:197: time.sleep(poll_interval)
hmmm, why? Can we wait for an event that would make either of r, w, e non-empty?

[cbentzel, 2010-11-18 11:59:09]

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

http://codereview.chromium.org/5104004/diff/1/net/tools/testserver/testserver...
net/tools/testserver/testserver.py:138: self._xmpp_server =
xmppserver.XmppServer(
Should xmpp_server be run a separate thread so you don't have to combine
asyncore's main looping mechanism with httpserver's?

http://codereview.chromium.org/5104004/diff/1/net/tools/testserver/testserver...
net/tools/testserver/testserver.py:1384: server_data['port'] =
server.server_port
Should this move to unique key per server type?

[akalin, 2010-11-18 23:32:58]

Addressed all comments, simplified select loop.  Everyone, please take another
look.

+ncarter for sync stuff.

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

http://codereview.chromium.org/5104004/diff/1/net/tools/testserver/testserver...
net/tools/testserver/testserver.py:138: self._xmpp_server =
xmppserver.XmppServer(
On 2010/11/18 11:59:09, cbentzel wrote:
> Should xmpp_server be run a separate thread so you don't have to combine
> asyncore's main looping mechanism with httpserver's? 

I think the complexity of this select loop (especially since I've cleaned it up
a bit more) is far less than the complexity of using threading in testserver.

http://codereview.chromium.org/5104004/diff/1/net/tools/testserver/testserver...
net/tools/testserver/testserver.py:197: time.sleep(poll_interval)
On 2010/11/18 11:58:45, Paweł Hajdan Jr. wrote:
> hmmm, why? Can we wait for an event that would make either of r, w, e
non-empty?

Actually, this case should never happen.  Removed this check and rewrote loop to
make it more obvious that r is never empty.

http://codereview.chromium.org/5104004/diff/1/net/tools/testserver/testserver...
net/tools/testserver/testserver.py:1384: server_data['port'] =
server.server_port
On 2010/11/18 11:59:09, cbentzel wrote:
> Should this move to unique key per server type? 

Hmm I don't think so.  Then run_testserver would have to look up a
per-server-type key, which seems like needless complexity at this point.

[cbentzel, 2010-11-19 15:39:29]

Pretty close to LGTM. 

I will be on vacation next week so feel free to land without further
confirmation from me, assuming ncarter + phajdan.jr are happy.

http://codereview.chromium.org/5104004/diff/7001/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/5104004/diff/7001/net/tools/testserver/testser...
net/tools/testserver/testserver.py:172: r = [ self.fileno() ]
Nit: are r,w,e too short? [maybe at least r,w,x since python's documentation for
select has arguments as rlist, wlist, xlist, although that may lead people to
believe this is read/write/execute]

I'd prefer read_fds, write_fds, exceptional_fds, but I understand it may make
the call to select.select expand to more than one line.

http://codereview.chromium.org/5104004/diff/7001/net/tools/testserver/testser...
net/tools/testserver/testserver.py:176: for fd, obj in
self._xmpp_socket_map.items():
xmpp_connection instead of obj?

http://codereview.chromium.org/5104004/diff/7001/net/tools/testserver/testser...
net/tools/testserver/testserver.py:188: r, w, e = select.select(r, w, e,
select_timeout)
Why do you need a timeout here? The tests that spawn testserver will likely have
timeouts for the test-on-the-whole.

http://codereview.chromium.org/5104004/diff/7001/net/tools/testserver/testser...
net/tools/testserver/testserver.py:1352: server_data = {}
Maybe server_ports instead of server_data?

[akalin, 2010-11-19 18:06:16]

Addressed all comments.  Please take another look.

http://codereview.chromium.org/5104004/diff/7001/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/5104004/diff/7001/net/tools/testserver/testser...
net/tools/testserver/testserver.py:172: r = [ self.fileno() ]
On 2010/11/19 15:39:29, cbentzel wrote:
> Nit: are r,w,e too short? [maybe at least r,w,x since python's documentation
for
> select has arguments as rlist, wlist, xlist, although that may lead people to
> believe this is read/write/execute]
> 
> I'd prefer read_fds, write_fds, exceptional_fds, but I understand it may make
> the call to select.select expand to more than one line.
> 

Done.

http://codereview.chromium.org/5104004/diff/7001/net/tools/testserver/testser...
net/tools/testserver/testserver.py:176: for fd, obj in
self._xmpp_socket_map.items():
On 2010/11/19 15:39:29, cbentzel wrote:
> xmpp_connection instead of obj? 

Done.

http://codereview.chromium.org/5104004/diff/7001/net/tools/testserver/testser...
net/tools/testserver/testserver.py:188: r, w, e = select.select(r, w, e,
select_timeout)
On 2010/11/19 15:39:29, cbentzel wrote:
> Why do you need a timeout here? The tests that spawn testserver will likely
have
> timeouts for the test-on-the-whole. 

Done.

http://codereview.chromium.org/5104004/diff/7001/net/tools/testserver/testser...
net/tools/testserver/testserver.py:1352: server_data = {}
On 2010/11/19 15:39:29, cbentzel wrote:
> Maybe server_ports instead of server_data? 

Hmm, it's a JSON grab-bag, so I think we shouldn't limit it to ports.  May want
to transmit other info later.

[cbentzel, 2010-11-19 18:09:02]

LGTM

[Paweł Hajdan Jr., 2010-11-19 21:03:16]

LGTM

[akalin, 2010-11-20 09:12:59]

ncarter already LGTMed a version of this in
http://codereview.chromium.org/3532014/ so I'll just submit it.  Nick if you
have any comments, I can take care of them later.

On 2010/11/19 21:03:16, Paweł Hajdan Jr. wrote:
> LGTM