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

Revision history for this message
Vincent Ladeuil (vila) wrote :

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

> Perhaps it's an IPv6 vs. IPv4 issue?

Exactly (and thanks to fullermd for confirming :-)

> Does the test work if you change it to pass "--port=127.0.0.1:0"?

Perfectly.

> The test
> assumes 127.0.0.1 later, so it's reasonable to explicitly pass it here.

Done.

> This change isn't controversial at all :)

Not in this incarnation :-)

It was discussed with Robert *before* submission and modified *during* submission,
so it ends up trivial instead of controversial :)

The whole patch is now:

=== modified file 'bzrlib/tests/blackbox/test_serve.py'
--- bzrlib/tests/blackbox/test_serve.py 2009-08-27 22:17:35 +0000
+++ bzrlib/tests/blackbox/test_serve.py 2009-09-08 03:31:02 +0000
@@ -255,14 +255,15 @@
         t = self.get_transport()
         t.mkdir('server-root')
         self.run_bzr_serve_then_func(
- ['--port', '0', '--directory', t.local_abspath('server-root'),
+ ['--port', '127.0.0.1:0',
+ '--directory', t.local_abspath('server-root'),
              '--allow-writes'],
             self.when_server_started)
         # The when_server_started method issued a find_repositoryV3 that should
         # fail with 'norepository' because there are no repositories inside the
         # --directory.
         self.assertEqual(('norepository',), self.client_resp)
-
+
     def run_bzr_serve_then_func(self, serve_args, func, *func_args,
             **func_kwargs):
         """Run 'bzr serve', and run the given func in a thread once the server

=== modified file 'bzrlib/tests/test_smart_transport.py'
--- bzrlib/tests/test_smart_transport.py 2009-07-08 07:03:38 +0000
+++ bzrlib/tests/test_smart_transport.py 2009-09-07 09:03:41 +0000
@@ -732,7 +732,7 @@
         client_sock.sendall(rest_of_request_bytes)
         server._serve_one_request(server_protocol)
         server_sock.close()
- self.assertEqual(expected_response, client_sock.recv(50),
+ self.assertEqual(expected_response, osutils.recv_all(client_sock, 50),
                          "Not a version 2 response to 'hello' request.")
         self.assertEqual('', client_sock.recv(1))

Is that ok with you ?

« Back to merge proposal