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

[John Arbash Meinel]

John Arbash Meinel has voted +0.
Status is now: Semi-approved
Comment:
Naming the functions 'call' and 'call2' isn't very obvious. Certainly 
from the name alone I don't know what the difference is.

Maybe "call()" and "call_with_protocol()".

Or maybe the other way around "call_no_body()" and "call()". (doesn't 
really work because you have 'call_with_body' to indicate you are 
sending data).


Is "type(X) is str" a good way to do it? I've also see "x.__class__ == 
str" and "isinstance(x, str)". (since basestring includes unicode).

I would like us to at least be consistent about it. I'm pretty sure 
these are just meant to trap against Unicode (because non-string objects 
would fail for other reasons)

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.


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.


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)


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


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?

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.


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