transport implementation test scenarios and rabbit-holing

[Vincent Ladeuil]

>>>>> "Michael" == Michael Hudson <michael.hudson at canonical.com> writes:

    Michael> In
    Michael> bzrlib.tests.test_transport.TestTransportImplementation.get_transport
    Michael> we find this chunk of code:

    Michael>         # try getting the transport via the regular interface:
    Michael>         t = get_transport(url)
    Michael>         # vila--20070607 if the following are commented out the test suite
    Michael>         # still pass. Is this really still needed or was it a forgotten
    Michael>         # temporary fix ?
    Michael>         if not isinstance(t, self.transport_class):
    Michael>             # we did not get the correct transport class type. Override the
    Michael>             # regular connection behaviour by direct construction.
    Michael>             t = self.transport_class(url)
    Michael>         return t

    Michael> As this code caused me minor inconvenience

Which one ?

    Michael> when arranging to test a transport internal to
    Michael> Launchpad with the bzrlib implementation tests, I
    Michael> was inspired to check whether vila's comment is
    Michael> still correct, and it is: you can comment the check
    Michael> out or even make the test explode if 't' is not an
    Michael> instance of self.transport_class and all the tests
    Michael> still pass.

EPARSE_ERROR: did the test explode or did all the tests pass ?

I don't remember the details, but this was meant as an help for
devs who messed up in their get_test_permutations
implementations, I'm pretty sure it saved me at least once since
I wrote that comment (in a plugin or another so I never updated
the comment).

The point is, it only matters if you want to test several
transports against the same server, which we don't actually. The
only cases where we may want to do that are http and ftp.

We explicitly define specific servers for each http transport by
defining specific get_test_permutations (so the check is useless
for http).

But for ftp it is needed as you diagnose below.

    Michael> So, it seemed to me that one might as well delete
    Michael> the check, which I did.  Then I found that this was
    Michael> the only use of transport_class in the tests, so I
    Michael> killed it and chased upwards, removing it from the
    Michael> scenario multiplcation code

I think a better fix will be:

'bzrlib/tests/test_transport_implementations.py'
--- bzrlib/tests/test_transport_implementations.py	2008-06-09 07:20:06 +0000
+++ bzrlib/tests/test_transport_implementations.py	2008-09-01 07:14:23 +0000
@@ -89,7 +89,8 @@
                 permutations = self.get_transport_test_permutations(
                     reduce(getattr, (module).split('.')[1:], __import__(module)))
                 for (klass, server_factory) in permutations:
-                    scenario = (server_factory.__name__,
+                    scenario = ('%s,%s' % (klass.__name__,
+                                           server_factory.__name__),
                         {"transport_class":klass,
                          "transport_server":server_factory})
                     result.append(scenario)

That make the test ids clearer.

    Michael> and in the end, the various get_test_permutations
    Michael> functions.  Then I found this, in
    Michael> bzrlib.transport.ftp._gssapi:

    Michael> def get_test_permutations():
    Michael>     """Return the permutations to be used in testing."""
    Michael>     from bzrlib import tests
    Michael>     if tests.FTPServerFeature.available():
    Michael>         from bzrlib.tests import ftp_server
    Michael>         return [(GSSAPIFtpTransport, ftp_server.FTPServer)]
    Michael>     else:
    Michael>         return []

    Michael> If you know or check how the pieces fit together,
    Michael> you'll see that this looks like a case that depends
    Michael> on the check I talked about deleting above.  But how
    Michael> can the tests pass even if I put a "1/0" in the
    Michael> 'then' part of the now-deleted if statement? 
    Michael> Well, because this function is never called, it
    Michael> seems.

If it is not called, then, either there is something broken in
the transport registration or you miss a dependency for the
module (in which case the permutation is silently discarded).

I do miss such a dependency so I can't diagnose further, but this
is clearly a use case that invalidate my comment :D

Jelmer, I installed python-kerberos but I still fail to import
ftp._gssapi because of:

if getattr(kerberos, "authGSSClientWrap", None) is None:
    raise errors.DependencyNotPresent('kerberos', 
        "missing encryption function authGSSClientWrap")

Any hint ?

    Michael> And now I've run out of time for today, and possibly
    Michael> energy for this problem, but I've attached the
    Michael> bundle of as far as I've gotten so far.  But it
    Michael> seems like a bug that the _gssapi stuff is not
    Michael> tested, does it not?

Well, you first need fulfil the dependencies to use it and test
it.

Then, AFAIR (Jelmer ?), the _gssapi stuff is only tested against
a server that doesn't support the GSSAPI, ensuring that we
fallback to "normal" ftp.

Patch implementing a ftp server with GSSAPI welcome :-D

At which point we will want to test both ftp clients against both
ftp servers.

    Vincent