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

[Martin Pool]

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.

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

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

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

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

> +        # 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).

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

> === modified file 'NEWS'
> --- NEWS	
> +++ NEWS	
> @@ -196,6 +196,21 @@
>        (Martin Pool) (NB: TestSkipped should only be raised for correctable
>        reasons - see the wiki spec ImprovingBzrTestSuite).
>  
> +    * Test sftp with relative, absolute-in-homedir and absolute-not-in-homedir
> +      paths for the transport tests. Introduce blackbox remote sftp tests that
> +      test the same permutations. (Robert Collins, Robey Pointer)
> +
> +    * Transport implementation tests are now independent of the local file
> +      system, which allows tests for esoteric transports, and for features
> +      not available in the local file system. They also repeat for variations
> +      on the URL scheme that can introduce issues in the transport code,
> +      see bzrlib.transport.TransportTestProviderAdapter() for this.
> +      (Robert Collins).
> +
> +    * TestCase.build_tree uses the transport interface to build trees, pass
> +      in a transport parameter to give it an existing connection.
> +      (Robert Collins).
> +
>    INTERNALS:
>  
>      * WorkingTree.pull has been split across Branch and WorkingTree,
> 
> === 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.)

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

> +    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()?

> +class TestTransportProviderAdapter(TestCase):

This could definitely do with some documentation of its purpose, as
could some of the methods.

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

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

> @@ -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 /\.["']$/

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

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

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

+1 from me modulo those small notes and well done.

-- 
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/ce266cc3/attachment.pgp