Code review comment for lp:~vila/bzr/leaking-tests-catch-them-all

>>>>> Robert Collins <email address hidden> writes:

    > Review: Needs Fixing
    > === modified file 'bzrlib/tests/__init__.py'
    > 16 --- bzrlib/tests/__init__.py 2010-06-23 18:37:20 +0000
    > 17 +++ bzrlib/tests/__init__.py 2010-06-23 18:37:22 +0000
    > 18 @@ -2431,6 +2431,15 @@
    > 19
    > 20 def setUp(self):
    > 21 super(TestCaseWithMemoryTransport, self).setUp()
    > 22 + # Ensure that ConnectedTransport doesn't leak sockets
    > 23 + def get_transport_with_cleanup(*args, **kwargs):
    > 24 + t = self._orig_get_transport(*args, **kwargs)
    > 25 + if isinstance(t, _mod_transport.ConnectedTransport):
    > 26 + self.addCleanup(t.disconnect)
    > 27 + return t
    > 28 +
    > 29 + self._orig_get_transport = self.overrideAttr(
    > 30 + _mod_transport, 'get_transport', get_transport_with_cleanup)
    > 31 self._make_test_root()
    > 32 self.addCleanup(os.chdir, os.getcwdu())
    > 33 self.makeAndChdirToTestDir()

    > This is pretty ugly; it appears to be on the class that defines
    > get_transport, so why not just change self.get_transport directly?

Ugly but working :)

It helped me track all the tests that wasn't calling
transport.get_transport (for no good reasons !).

Doing that I discovered many small bugs here and there, mostly about
using the right kind of transport (nothing serious as it uncovered no
bugs in the code, only in test coverage which was important to fix
anyway).

A hook called when a transport *really* connect was a suggestion
discussed but not implemented (and would also have addressed the need
but would have missed the opportunity to catch the coverage bugs).

Again, I chose to not overload the submission, can fix it, etc.

    > That is:

    > a) define a self.get_abs_transport which calls self._orig_mod_transport

What means 'abs' here ? abstract ? absolute ? Do you intend to rename
get_transport ?

    > b) tests for self.get_abs_transport

Right, I had a huge test coverage to find any problem here but not at
the right level, bad vila, no sugar ;)

    > c) overrideAttr in setUp

Or a hook.

    > Secondly, all the disconnect implementations are the same; why
    > isn't it in a base class?

No good reason :) Good review :)

    > I know this has been a lot of work for you so it can be easy to
    > miss details; I will be looking for such duplicate code blocks in
    > the other threads now - perhaps you should have a re-review first
    > ? :)

Hehe, right, sorry for that, I've been working too long on that and get
doscouraged several times so I really needed to get the whole lot
submitted as my eyes were bleeding which doesn't help :)

So I *did* re{>n}-review but not enough...

    > I'd like to see this after those things are fixed.

Cool, thanks a lot.