HPSS: Other test changes

[Andrew Bennetts]

John Arbash Meinel wrote:
> Andrew Bennetts wrote:
[...]
> >  
> > -    def __init__(self, transport_server, transport_readonly_server, formats):
> > +    def __init__(self, transport_server, transport_readonly_server, formats,
> > +        vfs_transport_factory=None):
> 
> ^- I'm pretty sure PEP8 says that this should be indented to the (
>     def __init__(self, transport_server, transport_readonly_server, formats,
>                  vfs_transport_factory=None):

Actually, PEP 8 doesn't specify, it just says "Make sure to indent the continued
line appropriately."  It then gives examples that do what you say, but they're
just examples...

> I know *I* prefer it that way.

This is harder to ignore ;)

> Especially when you don't have a doc string, it makes it more obvious
> where the code starts.

Sure.  I don't mind either way, so I've made the change.

> >      def adapt(self, test):
> > @@ -1715,6 +1719,8 @@
> >              new_test = deepcopy(test)
> >              new_test.transport_server = self._transport_server
> >              new_test.transport_readonly_server = self._transport_readonly_server
> > +            if self._vfs_transport_factory:
> > +                new_test.vfs_transport_factory = self._vfs_transport_factory
> >              new_test.bzrdir_format = bzrdir_format
> >              new_test.repository_format = repository_format
> >              def make_new_test_id():
> 
> ^- Why do you need the "if()" check here. And why are you setting ".vfs"
> from "._vfs" ? It seems like you would set "new_test._vfs... = self._vfs..."

Setting "new_test.vfs..." is intentional.  On the parameterised TestCase
instances, 'vfs_transport_factory' is the attribute that is used for getting VFS
transports, like 'transport_server' and 'repository_format'.  The underscore on
the attribute in the RepositoryTestProviderAdapter instance is just because
nothing but RepositoryTestProviderAdapter should need to access the state of
these objects.

The "if" check is so that we don't override the TestCase's default value of
vfs_transport_factory unless we have a value to replace it with.  It's possible
to not pass the vfs_transport_factory to RepositoryTestProviderAdapter, and that
means "don't override the test's default vfs_transport_factory."  I've added a
comment.

[...]
> Do you really want to do another import here? Shouldn't they at least
> all be done at the beginning of the function

Moved to the top of the module.  They were only there for hysterical raisins.

> > === modified file 'bzrlib/tests/branch_implementations/test_parent.py'
> > --- bzrlib/tests/branch_implementations/test_parent.py	2007-04-13 07:18:57 +0000
> > +++ bzrlib/tests/branch_implementations/test_parent.py	2007-04-16 05:57:08 +0000
> > @@ -19,6 +19,7 @@
> >  import os
> >  import sys
> >  
> > +from bzrlib import urlutils
> >  from bzrlib.branch import Branch
> >  import bzrlib.errors
> >  from bzrlib.osutils import abspath, realpath, getcwd
> 
> ^- You do the import, but don't actually use it. (At least there is no
> change to the file itself)

pyflakes points out there's a bunch of unused imports (plus the urlutils one,
which is actually a dupe).  I've cleaned them all up.

> > @@ -242,7 +243,7 @@
> >          tree.bzrdir.open_branch().set_revision_history([])
> >          tree.set_parent_trees([])
> >          tree.commit('revision 2', rev_id='2')
> > -        tree.bzrdir.open_repository().copy_content_into(shared_repo)
> > +        tree.branch.bzrdir.open_repository().copy_content_into(shared_repo)
> 
> ^- This looks rather bogus no matter which form it was using. Why not just
> 
> tree.branch.repository.copy_content_into()?

No idea.  Changed.

> v- Same later on

Fixed all the other occurrences I could find in that file.

Thanks for the review!

-Andrew.