[MERGE] Shelf 1 / 5: TreeTransform serialization

[John Arbash Meinel]

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

Aaron Bentley wrote:
> John Arbash Meinel wrote:
>> Aaron Bentley wrote: 
>> +    def test_roundtrip_creation(self):
>> +        tree = self.make_branch_and_tree('.')
>> +        tt, tt2 = self.get_two_previews(tree)
>> +        tt.new_file(u'foo\u1234', tt.root, 'bar', 'baz', True)
>> +        tt.new_directory('qux', tt.root, 'quxx')
>> +        self.reserialize(tt, tt2)
>> +        self.assertEqual(3, tt2._id_number)
>> +        self.assertEqual({'new-1': u'foo\u1234',
>> +                          'new-2': 'qux'}, tt2._new_name)
>>
>> ^- Question, is it better to compare the exact contents of attributes
>> like "_new_name" or would it be better to do:
>> self.assertEqual(tt._new_name, tt2._new_name)
> 
> I think that if TT's implementation changed enough that the contents of
> these dicts changed, we would want to know about it.  Otherwise, we
> might unwittingly create shelves that could not be restored.

So, I feel like you are either testing the details to closely, or not
extensively enough. You are testing *some* of the dict members, but not
all of them. So while changing the content of these will break the test,
a change to a different dict won't.

Maybe I'm full of it, but if you are testing the exact state of the TT,
shouldn't it be testing more than just a couple of members?

> 
>> It also seems like you should be asserting that the other dicts are
>> empty. Though just having "self.assertEqual(tt, tt2)" might cover that
>> best.
> 
> If I was going to define __eq__ on TT, I'd expect it to cover
> everything-- dicts, files, symlinks, etc.  But I think that's too
> heavyweight to provide as an equality operator.
> 
>> Also, it seems like it would be good to test non-ascii as part of these
>> tests. Either by default, or as a separate test. (Mostly non-ascii
>> paths, and possibly symlink target, though that isn't very well
>> supported by the rest of bzr)
> 
> I do test non-ascii paths.  test_roundtrip_creation creates a file named
> u'foo\u1234'.   test_roundtrip_destruction deletes a file with the same
> name,  I'm happy to add a test for non-ascii symlink targets, though.
> 
> Aaron
> 

Yeah, I just misread the function, to be honest:
tt.new_file(u'foo\u1234', tt.root, 'bar', 'baz', True)

Is a bit confusing to me. Doing something more like:
tt.new_file(u'foo\u1234', tt.root, contents='bar', file_id='baz',
            executable=True)

Would be a bit more obvious. Or just doing
tt.new_file(u'foo\u1234', tt.root, 'content\n', 'foo-id', True)

As it makes it obvious what is the content field, and what is the file
id field.

When I read it, I actually read it as content='foo\u1234',
filename='bar' file_id='baz'.

I'm fine with the unicode-ness of your test :).

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

iEYEARECAAYFAkj9/mMACgkQJdeBCYSNAAMzNwCeOsuldsm9e0ySg9Vy7t6mwGAK
8coAoKtz5ydsChNhgko6bV9vHV+PHrV0
=dBAf
-----END PGP SIGNATURE-----