[rfc][patch] test-providers branch ready for merge

[Robert Collins]

On Wed, 2006-01-11 at 13:49 +1100, Martin Pool wrote:
> On 11 Jan 2006, Robert Collins <robertc at robertcollins.net> wrote:
> > > > +    def test_stat(self):
> > > > +        # TODO: Test stat, just try once, and if it throws, stop testing
> > > 
> > > I wonder if Transport.stat should raise TransportNotPossible rather than
> > > NotImplementedError.  To me NotImpl suggests it will be done but the
> > > code's just not written yet.  Then this function can just catch that.
> > 
> > Thats possible - depending on how we resolve the 'should transports
> > declare unix permission support' question you raised above.
> 
> So depending on what is desired for windows we can either do
> 
>  - supports unix modes
> 
> or
> 
>  - supports marking files readonly
>  - supports setting x-bit

Yes. BTW, this isn't new code here, its part of the permissions work
John did, so I'd like to defer decisions on it too that. 

> > It does not change the 'nick' interface at all - it preserves it. See my
> > reply to John about transport.base ending with '/'.
> 
> Oh I see.  I'd tend to add an assert that the final character is '/' by
> way of showing why we want -2.

The transport implementation tests test this. 

> > > I'd much prefer to use addCleanup for cases where there is cleanup that
> > > really must be done.  It seems less prone to problems of people
> > > forgetting to call the right superclass tearDown function, etc.
> 
> I wish I could better articulate my dislike of repeatedly overriding
> setUp and tearDown in TestCase.  I think partly it seems more like
> OnceAndOnlyOnce or SameThingsTogether to declare that the resource must
> be freed at or near the point they're allocated.

So there are I think two orthogonal things that setUp and tearDown do:
one is resource initialisation and removal. My TestResources python
module is aimed squarely at this, and would certainly help by making
such things clearly orthogonal.
the second this is setting up the environment for the test, and removing
that environment  - which may or may not involve resource management.
(it might for example just involve deleting some data the test generates
that isn't needed and is rather large - i.e. logs or statistics etc.


> > Ah, I forgot about that. This is kindof different though, this is
> > cleanup that could fail that must not stop the super() being called.
> > Does it still fit ?
> 
> I think it will still fit, because the cleanups will generally run after
> the tearDown routines.  But perhaps the code that runs the cleanups
> should attempt to run all of them even if one fails.
> 
> > > > +class TestTransportProviderAdapter(TestCase):
> > > 
> > > This could definitely do with some documentation of its purpose, as
> > > could some of the methods.
> > 
> > err, ok. I thought the method names were clear enough - but if its not,
> > its not.
> 
> It's a pretty good name but what it does is a bit subtle, in being a 

???


> > > > +class TestCaseWithSFTPServer(TestCaseInTempDir):
> > > > +    """A test case base class that provides a sftp server on localhost."""
> > > >  
> > > >      def tearDown(self):
> > > >          try:
> > > > -            self._listener.stop()
> > > > -        except AttributeError:
> > > > -            pass
> > > > -        TestCaseInTempDir.tearDown(self)
> > > 
> > > Again, I'd far rather just keep track of the cleanups that need to be
> > > run than count on superclass tearDown calls being run properly.
> > 
> > uhm, seems to me you have reversed the meaning of the method again -
> > this is not depending on tearDown being run, its a tearDown that ensures
> > its parent tearDown gets run.
> 
> No, I understand the meaning of the method; I'm just saying in general I
> think it's better to release resources (or shutdown servers, etc)
> through addCleanup rather than overriding tearDown.

Ok, I'll look at that.
 
> > > > @@ -195,7 +226,10 @@
> > > >          As with other listing functions, only some transports implement this,.
> > > >          you may check via is_listable to determine if it will.
> > > >          """
> > > > -        raise NotImplementedError
> > > > +        raise errors.TransportNotPossible("This transport has not "
> > > > +                                          "implemented iter_files_recursive."
> > > > +                                          "(but must claim to be listable "
> > > > +                                          "to trigger this error).")
> > > 
> > > The formatting is a bit wierd because of the period before the
> > > close-quote; suggest you replace it with a space.  There are a few; grep
> > > for /\.["']$/
> > 
> > sorry, I'm confused here - do you mean the first or second, or both
> > periods before quotes?
> 
> The message at the moment is
> 
>   This transport has not implemented iter_files_recursive.(but must claim to be listable to trigger this error).
> 
> which is slightly ugly.  Better punctuation would be
> 
>   This transport has not implemented iter_files_recursive (but must claim to be listable to trigger this error).

ok, will do.

Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060111/aff0dfbf/attachment.pgp