Code review comment for lp:~vila/bzr/controversial

 review needs-info

I don't want to merge this as-is, but I have a guess about what's going on...

[...]
> I'd like some smart server expert advice there for the TestCmdServeChrooting.test_serve_tcp.
[...]
> + if sys.platform.startswith('freebsd'):
> + # Something fishy is going on there, the server is able to publish
> + # the port it is listening too, yet the client can't connect...
> + # The assertion below should fail when the problem is fixed or the
> + # test rewritten.
> + self.assertRaises(AttributeError, getattr, self, 'client_resp')

That is odd. By this point bind and listen have been called, which I would have
thought would be enough.

Perhaps it's an IPv6 vs. IPv4 issue? Currently bzr doesn't specify the address
family, it passes AF_UNSPEC to getaddrinfo and uses the address family (and
socktype and proto and address) returned from that. And it'll use
bzrlib.smart.medium.BZR_DEFAULT_INTERFACE (i.e. None) as the host for
getaddrinfo, hmm...

Does the test work if you change it to pass "--port=127.0.0.1:0"? The test
assumes 127.0.0.1 later, so it's reasonable to explicitly pass it here.

If not, does 'netstat -l' (or the equivalent?) report when you do "bzr serve --port
0"?

[...]
> === modified file 'bzrlib/tests/test_smart_transport.py'
[...]
> - self.assertEqual(expected_response, client_sock.recv(50),
> + self.assertEqual(expected_response, osutils.recv_all(client_sock, 50),

This change isn't controversial at all :)

-Andrew.