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

[Andrew Bennetts]

John Arbash Meinel wrote:
> John Arbash Meinel has voted tweak.
[...]
> +    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.

I don't think there's any need to do that.

The best way to test test infrastructure is with explicit tests for it, rather
than putting lots of asserts inside the helper functions.  That way the
infrastructure can be thoroughly tested without tests that use paying for
running the same assertion as 50 other tests.

That said, I don't think these helpers are complicated enough to warrant
explicit testing.

> +    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)"

Done.

> 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):

These are moved into a mixin class, TestSmartProtocolTwoSpecificsMixin.  So the
same test method is now used for testing both v2 and v3.

> -    def test_client_read_response_tuple_raises_UnknownSmartMethod(self):

This one is still defined on TestVersionOneFeaturesInProtocolOne, and
TestClientDecodingProtocolThree has test_read_response_tuple_unknown_request.
I've renamed the latter to test_read_response_tuple_raises_UnknownSmartMethod to
make it a little more obvious that it has the same purpose.

It seems that there's no equivalent test for version two, so I'll add the v1
test to v2's TestCase... ah, and guess what?  It was broken.  Trivial fix made
as well :)

> +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.

Well, I can add an explicit test for e.g. length 5 on the wire, but only 4
received, but it's not really a "break": the protocol decoder will simply be not
finished yet.  So any medium or test looking at its next_read_size() will see
that wants more bytes (so in practice it will cause a blocking read() call).  It
is worth explicitly testing that an incomplete message is correctly handled, and
that the pathological case of receiving a message one byte a time doesn't break
the state machine.

Here's the test I've added:

    def test_incomplete_messge(self):
        """A decoder will keep signalling that it needs more bytes via
        next_read_size() != 0 until it has seen a complete message, regardless
        which state it is in.
        """
        # Define a simple response that uses all possible message parts.
        headers = '\0\0\0\x02de'  # length-prefixed, bencoded empty dict
        response_status = 'oS' # success
        args = 's\0\0\0\x02le' # length-prefixed, bencoded empty list
        body = 'b\0\0\0\x04BODY' # a body: 'BODY'
        end = 'e' # end marker
        simple_response = headers + response_status + args + body + end
        # Feed the request to the decoder one byte at a time.
        decoder, response_handler = self.make_response_decoder()
        for byte in simple_response:
            self.assertNotEqual(0, decoder.next_read_size())
            decoder.accept_bytes(byte)
        # Now the response is complete
        self.assertEqual(0, decoder.next_read_size())

Similarly, a length prefix saying 10, followed by 12 bytes, is not a "break"  It
means the decoder should treat the next 10 bytes the contents of the current
message part, and the next 2 bytes as the start of the next message part.  Which
isn't really a useful test; that behaviour is pretty thoroughly covered already
I think.  Practically every test involving the smart server relies on this
working.

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

I agree.

> 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).

Well, it'll just block indefinitely in a read/recv call rather than
busy-waiting, but yes.

> ...
> +    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" ?

Ok.

> +    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.

Ok, done.  This is easier to do now thanks to some changes made earlier thanks
to feedback from you and Martin.

Thanks for all your reviews.  I'm glad to have finally dealt with them all.

-Andrew.