[rfc] [patch] sftp unit tests without ssh
John A Meinel
john at arbash-meinel.com
Mon Jan 23 04:09:47 GMT 2006
Robey Pointer wrote:
> Here's a patch to add a subclass of SFTPServer (called
> SFTPServerWithoutSSH) that sets up a fake sftp server over loopback but
> uses a special ssh vendor ("loopback") which just wraps a plain socket
> instead of setting up both ends of an SSH Transport. What this means is
> that tests using SFTPServerWithoutSSH will use sftp without all the
> overhead of actually setting up a secure channel.
>
> On my ibook, it drops the './bzr selftest' time from about 5 minutes to
> about 3 minutes -- pretty significant.
>
> We discussed having at least one test still use a full sftp-over-ssh
> setup. I didn't do that because I'm not entirely sure how, but it
> should just be a matter of using SFTPServer for a test instead of
> SFTPServerWithoutSSH.
I think just a simple test where we create an SFTPServer, and then use
't = get_transport('sftp://localhost')'.
But I'm not sure where this would fit with Robert's adapting transports
stuff.
>
> I posted the branch here:
> http://www.lag.net/~robey/bzr.dev.sftp/
> but it's only one patch, so that's attached below.
>
> robey
>
>
> === modified file 'bzrlib/tests/stub_sftp.py'
> --- bzrlib/tests/stub_sftp.py
> +++ bzrlib/tests/stub_sftp.py
> @@ -68,6 +68,7 @@
> self.home = home[len(self.root):]
> if (len(self.home) > 0) and (self.home[0] == '/'):
> self.home = self.home[1:]
> + server._test_case.log('sftpserver - new connection')
>
> def _realpath(self, path):
> return self.root + self.canonicalize(path)
> @@ -117,7 +118,7 @@
> try:
> if hasattr(os, 'O_BINARY'):
> flags |= os.O_BINARY
> - if (attr is not None) and hasattr(attr, 'st_mode'):
> + if getattr(attr, 'st_mode', None) is not None:
> fd = os.open(path, flags, attr.st_mode)
> else:
> fd = os.open(path, flags)
I've already put this in jam-integration, but no big deal here.
As an aside, we should change:
if hasattr(os, 'O_BINARY'):
flags |= os.O_BINARY
to
flags |= getattr(os, 'O_BINARY', 0)
But that isn't relevant to Robey.
>
> === modified file 'bzrlib/tests/test_sftp_transport.py'
> --- bzrlib/tests/test_sftp_transport.py
> +++ bzrlib/tests/test_sftp_transport.py
> @@ -97,9 +97,7 @@
>
> def test_multiple_connections(self):
> t = self.get_transport()
> - self.assertEquals(self.server.logs,
> - ['sftpserver - authorizing: foo'
> - , 'sftpserver - channel request: session, 1'])
> + self.assertTrue('sftpserver - new connection' in self.server.logs)
> self.server.logs = []
> # The second request should reuse the first connection
> # SingleListener only allows for a single connection,
>
> === modified file 'bzrlib/transport/sftp.py'
> --- bzrlib/transport/sftp.py
> +++ bzrlib/transport/sftp.py
> @@ -144,6 +144,25 @@
> self.proc.wait()
>
>
> +class LoopbackSFTP(object):
> + """Simple wrapper for a socket that pretends to be a paramiko Channel."""
> +
> + def __init__(self, sock):
> + self.__socket = sock
> +
> + def send(self, data):
> + return self.__socket.send(data)
> +
> + def recv(self, n):
> + return self.__socket.recv(n)
> +
> + def close(self):
> + self.__socket.close()
> +
> + def recv_ready(self):
> + return True
> +
> +
Don't we need recv_ready()? I don't know if the code actually tests
this, but I know it was a problem for SFTPSubprocess.
I thought paramiko expected it to be available if pipelining was enabled.
> SYSTEM_HOSTKEYS = {}
> BZR_HOSTKEYS = {}
>
> @@ -152,6 +171,7 @@
> # sort of expiration policy, such as disconnect if inactive for
> # X seconds. But that requires a lot more fanciness.
> _connected_hosts = weakref.WeakValueDictionary()
> +
>
> def load_host_keys():
> """
> @@ -170,6 +190,7 @@
> mutter('failed to load bzr host keys: ' + str(e))
> save_host_keys()
>
> +
> def save_host_keys():
> """
> Save "discovered" host keys in $(config)/ssh_host_keys/.
> @@ -222,7 +243,6 @@
> except (NoSuchFile,):
> # What specific errors should we catch here?
> pass
> -
>
>
> class SFTPTransport (Transport):
> @@ -671,7 +691,11 @@
> pass
>
> vendor = _get_ssh_vendor()
> - if vendor != 'none':
> + if vendor == 'loopback':
> + sock = socket.socket()
> + sock.connect((self._host, self._port))
> + self._sftp = SFTPClient(LoopbackSFTP(sock))
> + elif vendor != 'none':
> sock = SFTPSubprocess(self._host, vendor, self._port,
> self._username)
> self._sftp = SFTPClient(sock)
> @@ -867,23 +891,21 @@
> s, _ = self._socket.accept()
> # now close the listen socket
> self._socket.close()
> - self._callback(s, self.stop_event)
> -
> + try:
> + self._callback(s, self.stop_event)
> + except Exception, x:
> + # probably a failed test, might be nice to log it somehow
> + pass
> +
can't you use at least 'mutter()' here? Though probably warn() is more
appropriate.
> def stop(self):
> self.stop_event.set()
> - # We should consider waiting for the other thread
> - # to stop, because otherwise we get spurious
> - # bzr: ERROR: Socket exception: Connection reset by peer (54)
> - # because the test suite finishes before the thread has a chance
> - # to close. (Especially when only running a few tests)
> -
> -
> + # use a timeout here, because if the test fails, the server thread may
> + # never notice the stop_event.
> + self.join(5.0)
> +
> +
Thanks for doing this.
> class SFTPServer(Server):
> """Common code for SFTP server facilities."""
> -
> - def _get_sftp_url(self, path):
> - """Calculate a sftp url to this server for path."""
> - return 'sftp://foo:bar@localhost:%d/%s' % (self._listener.port, path)
>
> def __init__(self):
> self._original_vendor = None
> @@ -891,11 +913,16 @@
> self._server_homedir = None
> self._listener = None
> self._root = None
> + self._vendor = 'none'
> # sftp server logs
> self.logs = []
>
> + def _get_sftp_url(self, path):
> + """Calculate an sftp url to this server for path."""
> + return 'sftp://foo:bar@localhost:%d/%s' % (self._listener.port, path)
> +
I do prefer __init__ to be the first entry, so that looks good.
> def log(self, message):
> - """What to do here? do we need this? Its for the StubServer.."""
> + """StubServer uses this to log when a new server is created."""
> self.logs.append(message)
>
> def _run_server(self, s, stop_event):
> @@ -912,15 +939,11 @@
> ssh_server.start_server(event, server)
> event.wait(5.0)
> stop_event.wait(30.0)
> -
> +
> def setUp(self):
> - """See bzrlib.transport.Server.setUp."""
> - # XXX: 20051124 jamesh
> - # The tests currently pop up a password prompt when an external ssh
> - # is used. This forces the use of the paramiko implementation.
> global _ssh_vendor
> self._original_vendor = _ssh_vendor
> - _ssh_vendor = 'none'
> + _ssh_vendor = self._vendor
> self._homedir = os.getcwdu()
> if self._server_homedir is None:
> self._server_homedir = self._homedir
> @@ -937,7 +960,34 @@
> _ssh_vendor = self._original_vendor
>
>
> -class SFTPAbsoluteServer(SFTPServer):
> +class SFTPServerWithoutSSH(SFTPServer):
> + """
> + Common code for an SFTP server over a clear TCP loopback socket,
> + instead of over an SSH secured socket.
> + """
> +
> + def __init__(self):
> + super(SFTPServerWithoutSSH, self).__init__()
> + self._vendor = 'loopback'
> +
> + def _run_server(self, sock, stop_event):
> + class FakeChannel(object):
> + def get_transport(self):
> + return self
> + def get_log_channel(self):
> + return 'paramiko'
> + def get_name(self):
> + return '1'
> + def get_hexdump(self):
> + return False
> +
> + server = paramiko.SFTPServer(FakeChannel(), 'sftp', StubServer(self), StubSFTPServer,
> + root=self._root, home=self._server_homedir)
> + server.start_subsystem('sftp', None, sock)
> + server.finish_subsystem()
> +
> +
> +class SFTPAbsoluteServer(SFTPServerWithoutSSH):
> """A test server for sftp transports, using absolute urls."""
>
> def get_url(self):
> @@ -946,7 +996,7 @@
> urlescape(self._homedir[1:]))
>
>
> -class SFTPHomeDirServer(SFTPServer):
> +class SFTPHomeDirServer(SFTPServerWithoutSSH):
> """A test server for sftp transports, using homedir relative urls."""
>
> def get_url(self):
>
So +1, except I don't think you should swallow the Exception, at a
minimum it needs to be printed.
John
=:->
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 249 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060122/72cdcb1b/attachment.pgp
More information about the bazaar
mailing list