[MERGE] Add some unicode-related tests from the hpss branch, and a few other nits (also from the hpss branch).

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:

>> You also require args to be a tuple. Do you think someone might call it 
>> with a list? The requirement seems more accidental to me, but:
>>
>> smart_protocol.call_with_body_bytes((method, ) + args, body)
>>
>> will fail for anything but args == tuple.
> 
> I think this is a YAGNI.  It's easy to make more flexible later if we decide
> it's worth it, but so far the hpss branch hasn't had trouble with using tuples.
> Perhaps because the current protocol is largely about encoding and decoding
> tuples?  Anyway, I'm not worried about this.
> 

^- I think it is a YAGNI, but I also think it is something that non-uber
hackers could stumble on.... Would they stumble very hard?

>>> x = (1,2,3)
>>> x + [4, 5]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can only concatenate tuple (not "list") to tuple

Is probably sufficient for them to know what to do.



>> Is this more "remote_path_for_transport"?
>> +
>> +    def remote_path_from_transport(self, transport):
>> +        """Convert transport into a path suitable for using in a 
>> request."""
>> +        return unescape(urlparse(transport.base)[2]).encode('utf8')
>>
>> since you are converting transport.base.
>>
>> Maybe just making the docstring indicate that you are using the base 
>> location of the transport.
> 
> "From" seems like the right word to me.  The situation is given a transport for
> some location, what's the remote path (i.e. the path string to send to the
> remote end) to use to operate on that location?  This function calculates that
> remote path from the transport.  I suppose "for" would be ok too, but I don't
> see how it's any better.
> 
> Although it seems almost too obvious to be worth stating, I've expanded the
> docstring like so:
> 
>     def remote_path_from_transport(self, transport):
>         """Convert transport into a path suitable for using in a request.
>         
>         Note that the resulting remote path doesn't encode the host name or
>         anything but path, so it is only safe to use it in requests sent over
>         the medium from the matching transport.
>         """

^- Well, too obvious to you, and understandable to me :) This is a
really nice docstring. Thank you. I honestly didn't correctly understand
the function before this.

> 
>> You don't have any direct tests on SmartClient. And shouldn't it be 
>> called RemoteClient? (maybe I'm missing when and what things were 
>> renamed)
> 
> SmartClient is reasonable name, it is a client for making smart protocol
> requests with.  (Unlike "SmartTransport", SmartClient is an implementation
> detail of the smart protocol code, and doesn't imply that it is an object that
> is operating in a smart manner, which was we changed "SmartTransport" to
> "RemoteTransport": SmartTransports aren't any more efficient, or smarter, than
> e.g. SFTPTransports). 
> 
> The lack of direct tests is a shame, but (in the full hpss branch) this code is
> fully tested indirectly.  SmartClient was a straightforward refactoring of
> existing tested code to remove duplication.  So it does get thoroughly
> exercised, and it is very simple anyhow.
> 
> I'd like there to be direct tests, but at this point it's not a big concern for
> me; I'm confident in this code anyway, and SmartClient doesn't change very much
> so the lack of more focussed tests isn't much of a burden to developing it.  If
> SmartClient evolves more later, that would be a good time to define tests for it
> I think.

^- I really don't like "public" code that isn't tested. Once we have a
release, we have a backwards compatibility statement. (Which we
certainly seem to break regularly, but we try not to). Without failing
tests, you don't even know if you've broken compatibility.

If SmartClient is meant as an implementation detail, then it should be
marked private IMO.

You seem to be writing tests to make sure the code you've written is
correct (which is valid). I also write them to make sure other people
updating the code don't mess it up.

> 
>> +    def test_call_with_body_bytes_unicode_args(self):
>> +        self.assertCallDoesNotBreakMedium('method', (u'args',), 'body')
>>
>> I think it is also reasonable to test that the second argument being 
>> unicode would raise TypeError.
>> +        self.assertCallDoesNotBreakMedium('method', ('a', u'arg'), 
>> 'body')
>>
>> I may be a little overzealous, but I think in a case where you can have 
>> more than 1, you should test for more than 1. I think what I read was 
>> that you should actually test for 1 and 3. (1 could easily be a special 
>> case, IMO)
> 
> Well, in my view the point of testing is to give you confidence that the code
> works.  The extra confidence I would gain by adding arbitrary cases like that is
> minimal here, because it seems to me the code would have to be intentionally
> perverse to fail like that.


Not really, you may not loop over the args, or something (foolish) like
that. Not a big deal. It is just edge-case testing.



> 
> Still, it's just one line, so I've added:
> 
>         self.assertCallDoesNotBreakMedium('method', ('arg1', u'arg2'), 'body')
> 
>> Why not actually raise a specific error here:
>> +    def test_put_bytes_unicode(self):
>> +        # Expect put_bytes to raise AssertionError or 
>> UnicodeEncodeError if
>> +        # given unicode "bytes".  UnicodeEncodeError doesn't really 
>> make sense
>>
>>
>> It also gives you a proper error to raise from MemoryError.
>>
>> Why not raise "TypeError" instead?
> 
> As the comment goes on to say:
> 
>         #                                   ... but we allow it for backwards
>         # compatibility.  At some point we should use a specific exception.
> 
> I agree a specific exception would be better, but because fixing that wasn't the
> point of this change, I was being conservative.  I'm just making the existing
> behaviour consistent (even though it's not ideal); making it better can happen
> separately.
> 
> I've filed ://bugs.launchpad.net/bzr/+bug/106898, and linked to it from this
> comment.
> 
>> Also, I would make sure that these tests pass under "python -O ./bzr 
>> selftest". I generally frown on seeing a test expecting AssertionError 
>> to be raised. Certainly the underlying code could use "if not foo: raise 
>> AssertionError" rather than "assert foo". But it isn't a good exception 
>> to be using, especially when you codify that is should be used by the 
>> test suite.
> 
> Yep, it passes.  Worth checking though! :)
> 
> Any chance of an upgrade to +1?
> 
> -Andrew.

I don't want to get bogged down without getting the real improvements
merged. I think there are some testing philosophy discussions to be had,
but the changes are fine. (+1).

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGI5fqJdeBCYSNAAMRAmt5AJ9uStVH7a+YyTbBYA9bbO2TcmuJlACcD3nz
EzHB3OO6sbNKoDqsZIpvW1A=
=l/q8
-----END PGP SIGNATURE-----