Summary: | Add tests for TLS server channels | ||
---|---|---|---|
Product: | Telepathy | Reporter: | Cosimo Cecchi <cosimoc> |
Component: | gabble | Assignee: | Cosimo Cecchi <cosimoc> |
Status: | RESOLVED FIXED | QA Contact: | Telepathy bugs list <telepathy-bugs> |
Severity: | normal | ||
Priority: | medium | CC: | will |
Version: | git master | Keywords: | patch |
Hardware: | Other | ||
OS: | All | ||
URL: | http://git.collabora.co.uk/?p=user/cosimoc/telepathy-gabble.git;a=shortlog;h=refs/heads/tls-tests-2 | ||
Whiteboard: | review+ | ||
i915 platform: | i915 features: |
Description
Cosimo Cecchi
2010-08-25 08:01:25 UTC
Pleases rename the test to tls/server-tls-channel.py (removing the 'test-' prefix)? It's obviously a test. It's in the directory full of tests. (I know other tests violate this.) + def auth(self, auth): + assert (base64.b64decode(str(auth)) == + '\x00%s\x00%s' % (self.username, self.password)) + + success = domish.Element((ns.NS_XMPP_SASL, 'success')) + self.xmlstream.send(success) + self.xmlstream.reset() + self.sasl_authenticated = True Can't this subclass XmppAuthenticator and chain up to the superclass rather than re-implementing it all? Ditto the streamStarted() method; it seems to copy-paste a lot of code. bindIq() is literally a verbatim copy. + file = open(CA_KEY, 'rb') + pem_key = file.read() + file.close() + pkey = crypto.load_privatekey(crypto.FILETYPE_PEM, pem_key, "") Better written as: with open(CA_KEY, 'rb') as file: pkey = crypto.load_privatekey(crypto.FILETYPE_PEM, pem_key, "") + q.expect_many( + EventPattern('dbus-signal', signal='Rejected'), + EventPattern('dbus-signal', signal='Closed'), + EventPattern('dbus-signal', signal='ChannelClosed'), + EventPattern('dbus-signal', signal='ConnectionError', + args=[cs.CERT_UNTRUSTED, {}]), + EventPattern('dbus-signal', signal='StatusChanged', + args=[cs.CONN_STATUS_DISCONNECTED, cs.CSR_CERT_UNTRUSTED]) + ) Don't we expect these to happen in a particular order? expect_many is for ignoring the order of events. + # we expect the fallback verification process to connect successfully There should be a test for when it doesn't, presumably by setting { 'require-encryption': True, 'ignore-ssl-errors': False }. And, why do we expect the fallback verification process to connect successfully in this case? I think we should document exactly what domain the certificate is for, probably in this test, rather than relying on later readers asking you on IRC or digging out a tool to examine certificates. + assertEquals (len(rejections), 0) assertLength(0, rejections) Python coding style doesn't put a space before the opening paren. I fixed my branch according to your comments; another round of review welcome :) + with open(CA_KEY, 'rb') as file: + pem_key = file.read() + file.close() + pkey = crypto.load_privatekey(crypto.FILETYPE_PEM, pem_key, "") You don't need to call close(). That's one of the points of with: it's closed when you leave that scope. As we discussed on IRC, don't avoid UTF-8, just add coding=utf-8 to the top of the comment. http://git.collabora.co.uk/?p=user/cosimoc/telepathy-gabble.git;a=commitdiff;h=1fada095b5fb9215a75a5c3a5b18f604ee3abf5b quite visibly shows a spaces vs. tabs mix-up towards the end of the diff. (In reply to comment #3) Thanks for the review, I just updated my branch according to your comments. Cool, ship it. (In future, you could squash the remove-close() and go-back-to-UTF-8 patches into the relevant previous patches.) (In reply to comment #5) > Cool, ship it. > > (In future, you could squash the remove-close() and go-back-to-UTF-8 patches > into the relevant previous patches.) Thanks, merged to master. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.