[MERGE] [Bug 226769] Disable some strace tests

[Vincent Ladeuil]

>>>>> "robert" == Robert Collins <robertc at robertcollins.net> writes:

    robert> On Mon, 2008-05-05 at 15:13 +0200, Vincent Ladeuil wrote:
    >> >>>>> "martin" == Martin Pool <mbp at sourcefrog.net> writes:
    >> 
    >> <snip/>
    >> 
    martin> I don't think we even have any current code that
    martin> makes use of the strace function.  Perhaps the most
    martin> practical thing is just to disable or delete it?  If
    martin> we do want to retain it for use in testing/profiling,
    martin> I think it would be reasonable to leave it in but
    martin> untested, and allow people to fix it if it is broken
    martin> when they need it.
    >> 
    >> Here is patch that skip the tests with a comment explaining why.

    robert> I make use of the strace function quite regularly. I
    robert> don't want to have to fix it if it bitrots.

I make use of selftest quite regularly and try to improve it. I'm
a bit disappointed to have to fix some unrelated bugs while
improving it or being blocked trying to do so :)

    robert> I'd much prefer that the test remain enabled, and
    robert> cause a failure rather than a hang. This can be done
    robert> by asserting that the active thread count is 1.

    robert> Better yet, *every* test should:
    robert>  - record the active thread count when it starts
    robert>  - fail() if the active thread count when it finishes is different.

I will try that, but I'm pretty sure that tests involving http
1.1 servers will barf, their threads are garbage collected
later. There was some discussion about it around bug #193253. And
of course there is bug #226856 which can be considered as the
origin of this strace saga sequel.

Now, keep in mind that I'm proposing selftest enhancements so
that debugging such problems are easier. I didn't mention the
number of hours I spent isolating the *3* tests that were
interacting ( I was lucky enough to early try a 'bzr selftest ac'
exhibiting the problem with ~2700 tests).

Of course bisecting the test suite doesn't apply in that case,
especially when you have to *kill* hanging selftests :)

Retrospectively I realized that, having known how many tests were
interacting I could have used a more sophisticated bisection
approach and find a way to kill the hanging processes, but, hey,
I was trying to fix the problem :) And the problem was that my
submission exhibited a latent bug not an introduced one !

So, I'm not trying to put the strace bug under the carpet, quite
the contrary, I consider it the real culprit here, more than bug
#226856 which leave one process and one or several threads behind
it. Granted, leaving threads or processes behind is bad, but the
alternative in the http case (which I tried) makes the code
uglier (not to mention harder to debug) and slows down the test
suite.

An option I didn't mention so far, is to make strace a plugin so
that it doesn't put such a burden on the bzr test suite.

If you really want to cleanup the test suite I can point you to
more than a couple of tests that fail when run alone and/or when
specifying a TMPDIR and/or able to evade the '.bzr' safety net
(see _check_safety_net in bzrlib.tests).

I realize that this may sound a lot like whining, it is not :)

I'm just trying to persuade you that I'm serious about having a
solid test suite and that I do not intend to forget about strace
(nor the other problems I mentioned above).

Anyway, enough rambling, time to look at your proposal,

...
...

Trying the simplest first in TestCase.setUp:

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2008-05-05 02:06:10 +0000
+++ bzrlib/tests/__init__.py	2008-05-06 15:29:05 +0000
@@ -42,6 +42,7 @@
 from subprocess import Popen, PIPE
 import sys
 import tempfile
+import threading
 import time
 import unittest
 import warnings
@@ -801,6 +802,14 @@
         self._benchtime = None
         self._clear_hooks()
         self._clear_debug_flags()
+        _active_threads = threading.activeCount()
+
+        def _check_treads():
+            leaked_threads = threading.activeCount() - _active_threads
+            if leaked_threads:
+                print '\n%s leaked %d threads' % (self.id(), leaked_threads)
+
+        self.addCleanup(_check_treads)
 
     def _clear_debug_flags(self):
         """Prevent externally set debug flags affecting tests.

gives (far too much to paste here) 1089 tests reporting between 6
and -9 leaked threads.

The negative numbers are for garbage collected threads, which
means the approach in the patch above is not reliable since some
leaked threads may have been missed, but I think it gives a rough
idea abouth the amount of work required to make your proposal
usable :-/

You're not seriously asking me to fix all of that ?

        Vincent