[rfc][patch] test-providers branch ready for merge
Robert Collins
robertc at robertcollins.net
Wed Jan 11 02:15:11 GMT 2006
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.
> > + def check_transport_contents(self, content, transport, relpath):
> > + """Check that transport.get(relpath).read() == content."""
> > + self.assertEqual(content, transport.get(relpath).read())
>
> I recommend using assertEqualsDiff when the contents are possibly long;
> it makes failures much easier to debug. (But that method should be
> renamed and perhaps be more accepting of different argument types.)
Yah, This was just factored out from the existing code. I'm happy to
make it use aED
> > + def assertListRaises(self, excClass, func, *args, **kwargs):
> > + """Many transport functions can return generators this makes sure
> > + to wrap them in a list() call to make sure the whole generator
> > + is run, and that the proper exception is raised.
> > + """
>
> Remember PEP-8 says the first line of the docstring should be a
> self-contained summary.
Lifted code.
> > + def test_put_permissions(self):
> > + t = self.get_transport()
> > +
> > + if t.is_readonly():
> > + return
>
> I wonder if transports should declare whether they can handle unix
> modes or not?
>
> (What should this do on Windows filesystems? They can presumably
> support a single read-only bit but no more - or are the permissions
> stored in the posix mode bits, and if so do they have the right effect
> on write access?)
lifted code :)
> > + # Test that copying into a missing directory raises
> > + # NoSuchFile
> > + if t.is_readonly():
> > + os.mkdir('e')
> > + open('e/f', 'wb').write('contents of e')
>
> I realize you may have just moved this code from another test but I'll
> still point out this is a somewhat dangerous pattern. There is no
> guarantee after this st atement that anything has been written to e/f,
> because the file may not have closed and so the contents may still be in
> memory. It needs to be inside a try/finally block (or just call
> build_tree).
In fact its worse than that, as its not transport safe - not all
transports back to disk. I'll definately fix this tomorrow.
> > + 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.
> > === modified file 'bzrlib/branch.py'
> > --- bzrlib/branch.py
> > +++ bzrlib/branch.py
> > @@ -155,7 +155,7 @@
> >
> > def _get_nick(self):
> > cfg = self.tree_config()
> > - return cfg.get_option(u"nickname", default=self.base.split('/')[-1])
> > + return cfg.get_option(u"nickname", default=self.base.split('/')[-2])
> >
> > def _set_nick(self, nick):
> > cfg = self.tree_config()
>
> Why? Is this an intentional change in behaviour? If it changes the
> default tree nick it should be in the NEWS file shouldn't it? (Maybe
> this change was merged in indirectly but I'd think there should still be
> a comment about it.)
It does not change the 'nick' interface at all - it preserves it. See my
reply to John about transport.base ending with '/'.
> > === modified file 'bzrlib/tests/HTTPTestUtil.py'
> > --- bzrlib/tests/HTTPTestUtil.py
> > +++ bzrlib/tests/HTTPTestUtil.py
>
> > def setUp(self):
> > - TestCaseInTempDir.setUp(self)
> > - import threading, os
> > - self._http_starting = threading.Lock()
> > - self._http_starting.acquire()
> > - self._http_running = True
> > - self._http_base_url = None
> > - self._http_thread = threading.Thread(target=self._http_start)
> > - self._http_thread.setDaemon(True)
> > - self._http_thread.start()
> > - self._http_proxy = os.environ.get("http_proxy")
> > - if self._http_proxy is not None:
> > - del os.environ["http_proxy"]
> > + super(TestCaseWithWebserver, self).setUp()
> > + self.server = HttpServer()
> > + self.server.setUp()
> >
> > def tearDown(self):
> > - self._http_running = False
> > - self._http_thread.join()
> > - if self._http_proxy is not None:
> > - import os
> > - os.environ["http_proxy"] = self._http_proxy
> > - TestCaseInTempDir.tearDown(self)
> > -
> > + try:
> > + self.server.tearDown()
> > + finally:
> > + super(TestCaseWithWebserver, self).tearDown()
>
> 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.
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 ?
> > + def assertMode(self, transport, path, mode):
> > + """Fail if a path does not have mode mode.
> > +
> > + If modes are not supported on this platform, the test is skipped.
> > + """
> > + if sys.platform == 'win32':
> > + return
> > + path_stat = transport.stat(path)
> > + actual_mode = stat.S_IMODE(path_stat.st_mode)
> > + self.assertEqual(mode, actual_mode,
> > + 'mode of %r incorrect (%o != %o)' % (path, mode, actual_mode))
>
> assertMode seems like an overly generic name; perhaps call it
> assertTransportFileMode()?
Sure.
> > +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.
> > +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.
> > + def test__remote_path(self):
> > + t = self.get_transport()
> > + # try what is currently used:
> > + # remote path = self._abspath(relpath)
> > + self.assertEqual(self._root + '/relative', t._remote_path('relative'))
> > + # we dont os.path.join because windows gives us the wrong path
> > + root_segments = self._root.split('/')
> > + root_parent = '/'.join(root_segments[:-1])
> > + # .. should be honoured
> > + self.assertEqual(root_parent + '/sibling', t._remote_path('../sibling'))
> > + # / should be illegal ?
> > + ### FIXME decide and then test for all transports. RBC20051208
>
> 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.
> > @@ -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?
> > +class Server(object):
> > + """Basic server API for paths."""
>
> More explanation please? This is not a bzr server but rather used to
> manage protocol servers for testing purposes. It can start and stop
> them, and tell the url to access the cwd through this server. I
> realize there are docs on the methods, which is good, but such a broad
> name as 'Server' needs some context.
Sure. It is a real bzr server though - they have to be to meet the
implementation tests requirements.
> > +class TransportTestProviderAdapter(object):
> > + """A class which can setup transport interface tests."""
>
> doc: This generates a TestSuite in which a test is repeated against all
> the implementations of a protocol's clients and servers?
I will improve it.
> > + def is_readonly(self):
> > + """See Transport.is_readonly."""
> > + return True
> > +
>
> Please let's not use those docstrings in new code: either a comment, or
> a description of the behaviour in this subclass, or perhaps a no-op
> decorator.
Given the previous conversation about these - that these are statements
of intent, I'd rather keep doing them until such a decorator is done,
otherwise we are losing information.
> +1 from me modulo those small notes and well done.
Thanks, I'll do the ones that I haven't quibbled about above tomorrow,
hopefully we can have those cleared up for then too.
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/541875b9/attachment.pgp
More information about the bazaar
mailing list