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

[Martin Pool]

On 11 Jan 2006, Robert Collins <robertc at robertcollins.net> wrote:
> On Wed, 2006-01-11 at 11:42 +1100, Martin Pool wrote:
> > On 10 Jan 2006, Robert Collins <robertc at robertcollins.net> wrote:
> > > http://people.ubuntu.com/~robertc/baz2.0/test-providers
> > 
> > Good work.  I'd be happy to get this in, and to generalize the idea of
> > repeating tests against various providers of an interface.
> 
> Yup, good.
> 
> > > +def _append(fn, txt):
> > > +    """Append the given text (file-like object) to the supplied filename."""
> > > +    f = open(fn, 'ab')
> > > +    f.write(txt.read())
> > > +    f.flush()
> > > +    f.close()
> > > +    del f
> > 
> > Note that flush just flushes the internal buffer, so is completely
> > redundant with close.  Better to just put the close() in a finally
> > block and get rid of the flush and del.  (Presumably this was just
> > lifted from somewhere.)
> 
> Lifted :). Generally I'd like to only add TODO's for lifted code, as my
> goal wasn't an audit of the tests, rather a translation.

Sure, that's fine.  Sometimes rearranged code just happens to get an
opportunistic review.  I think for small things like this it's simpler
just to fix it but for more than a few lines better to just add a TODO>

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

> 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.

> > 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.
 
> 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.

> > What does the 'test__' do?
> 
> The method its testing is '_remote_path'. I prefixed that with 'test_'
> to get the test method name. There may also be a 'remote_path' method,
> and using 'test_remote_path' for that would be nice.

OK

> 
> > > @@ -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).

-- 
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060111/d3d182e3/attachment.pgp