[MERGE] Updates to test_smart_protocol for protocol v3 (protocol v3 patch 7/7)

[John Arbash Meinel]

John Arbash Meinel has voted tweak.
Status is now: Conditionally approved
Comment:
Overall, my biggest concern with the full set of 7 patches is 
compatibility
with existing clients. I've mentioned it before, but will a v1.4 client 
be able
to work with a v1.5 server? It seems like the initial ('ok', '3') is 
going to
confuse them.

I would probably say resubmit, but I'm not sure I want to review the 
whole patch again...


+    def make_client_protocol(self, input_bytes=None):
+        result = 
self.make_client_protocol_and_output(input_bytes=input_bytes)
+        requester, response_handler, output = result
+        return requester, response_handler


^- Should this assert something about 'output' since it is being thrown 
away?
(empty, etc?) maybe not.


+
+    def make_client_protocol_and_output(self, input_bytes=None):
+        """
+        :returns: a Request
+        """
+        # This is very similar to
+        # bzrlib.smart.client._SmartClient._build_client_protocol
+        if input_bytes is None:
+            input = StringIO()
+        else:
+            input = StringIO(input_bytes)
+        output = StringIO()
+        client_medium = medium.SmartSimplePipesClientMedium(input, 
output)
+        request = client_medium.get_request()
+        if self.client_protocol_class is not None:
+            client_protocol = self.client_protocol_class(request)
+            return client_protocol, client_protocol, output
+        else:
+            assert self.request_encoder is not None
+            assert self.response_decoder is not None

^- Plain 'assert' again. Since this is a test class, I think these 
should be
"self.assertIsNot(None, self.request_encoder)"


You have what appear to be a whole lot of moved code, but these 
functions
appear to only be deleted:
-    def 
test__send_response_with_body_stream_sets_finished_reading(self):
-    def 
test__send_response_with_body_stream_sets_finished_reading(self):
-    def 
test_client_read_response_tuple_raises_UnknownSmartMethod(self):



+class TestProtocolThree(TestSmartProtocol):

You seem to do a good job about testing how things *work*. But you don't 
have
many tests here about how things *break*. Such as "prefix" of length 5 
but only
4 bytes on the wire. Or length == 10, and 12 bytes on the wire, etc.

I really prefer it when things break in predictable (tested) ways.

I'm not sure what sort of breaks you can safely test. For example, the 
code
seems *designed* to block on a socket until all the data comes in. So if 
you
pass less than the expected amount, you'll might get an endless loop 
(assuming
you would get a connection close to break you out of it in RL).


...
+    def make_response_decoder(self):
+        """Make v3 response decoder using a test response handler."""
+        response_handler = LoggingMessageHandler()
+        decoder = protocol.ProtocolThreeDecoder(response_handler)
+        return decoder, response_handler

^- Should this be "make_logging_response_decoder" ?

+    def test_call_smoke_test(self):
+        """A smoke test for ProtocolThreeRequester.call.
+
+        This test checks that a particular simple invocation of call 
emits the
+        correct bytes for that invocation.
+        """
+        requester, output = self.make_client_encoder_and_output()
+        requester.call('one arg', headers={'header name': 'header 
value'})
+        self.assertEquals(
+            'bzr message 3 (bzr 1.3)\n' # protocol version
+            '\x00\x00\x00\x1fd11:header name12:header valuee' # headers
+            's\x00\x00\x00\x0bl7:one arge' # args
+            'e', # end
+            output.getvalue())

^- Again with '1.3' showing up.


+
+    def test_call_default_headers(self):
+        """ProtocolThreeRequester.call by default sends a 'Software
+        version' header.
+        """
+        requester, output = self.make_client_encoder_and_output()
+        requester.call('foo')
+        # XXX: using assertContainsRe is a pretty poor way to assert 
this.
+        self.assertContainsRe(output.getvalue(), 'Software version')
+

^- I think I would like this to check that __version__ is being used 
properly,
and not just 'Software version' being present.



For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C20080423232617.GK9546%40steerpike.home.puzzling.org%3E