Merge lp:~mbp/bzr/626679-strace into lp:bzr

[Martin Pool]

disable strace tests broken under maverick

Ubuntu recently changed to stop users tracing their own processes by default as a security measure, which means that 'bzr selftest -s bt.test_strace' fails on maverick.

Unfortunately strace doesn't give you a clear error code to tell you that you hit this.

We don't actually use strace in any other tests at the moment, but it has been useful in the past for ad-hoc performance investigation.

[Robert Collins (lifeless)]

+1

review: Approve

[Martin Pool (mbp)]

sent to pqm by email

[Andrew Bennetts (spiv)]

PQM seems to have hung while processing this. The circumstantial evidence from spm is that it was probably executing the strace tests at the time: there was an strace subprocess, and killing it caused another strace subprocess to be spawned. Hopefully some informative tracebacks/errors are on their way back to Martin from PQM.

[Martin Pool (mbp)]

Thanks.

That actually pushes me a bit more towards deleting/disabling the
tests, but leaving the facility. It's useful to have it, but it seems
silly to have test hangs for something that's not actively used other
than in the tests.

--
Martin

[Andrew Bennetts (spiv)]

Martin Pool wrote:
> Thanks.
>
> That actually pushes me a bit more towards deleting/disabling the
> tests, but leaving the facility. It's useful to have it, but it seems
> silly to have test hangs for something that's not actively used other
> than in the tests.

I think that's a reasonable compromise. Maybe leave the tests there but
skipped? It doesn't seem like something worth spending large amounts of
time on. The next person to work on the strace helpers can look at
fixing their tests if they like.

[lp:~mbp/bzr/626679-strace]

5400. By Martin Pool on 2010-09-01

Remove TestStraceFeature that just duplicates the code it tests

5401. By Martin Pool on 2010-09-01

Just skip the strace tests

[Martin Pool (mbp)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-09-01 08:02:42 +0000
+++ NEWS	2010-09-01 08:49:39 +0000
@@ -142,6 +142,10 @@
   fetching from repositories with affected revisions.
   (Andrew Bennetts, #522637)
 
+* strace test-helper tests cope with the new Ubuntu policy of not allowing
+  users to attach to their own processes by default.
+  (Martin Pool, #626679)
+
 * ``Transport.stat`` on a symlink, including a transport pointing directly
   to a symlink, now returns information about the symlink.
   (Martin Pool)

=== modified file 'bzrlib/strace.py'
--- bzrlib/strace.py	2009-03-23 14:59:43 +0000
+++ bzrlib/strace.py	2010-09-01 08:49:39 +0000
@@ -23,6 +23,9 @@
 import subprocess
 import tempfile
 
+from bzrlib import errors
+
+
 # this is currently test-focused, so importing bzrlib.tests is ok. We might
 # want to move feature to its own module though.
 from bzrlib.tests import Feature
@@ -46,14 +49,17 @@
     # capture strace output to a file
     log_file = tempfile.NamedTemporaryFile()
     log_file_fd = log_file.fileno()
+    err_file = tempfile.NamedTemporaryFile()
     pid = os.getpid()
     # start strace
     strace_cmd = ['strace', '-r', '-tt', '-p', str(pid), '-o', log_file.name]
     if follow_children:
         strace_args.append('-f')
+    # need to catch both stdout and stderr to work around
+    # bug 627208
     proc = subprocess.Popen(strace_cmd,
                             stdout=subprocess.PIPE,
-                            stderr=subprocess.STDOUT)
+                            stderr=err_file.fileno())
     # Wait for strace to attach
     attached_notice = proc.stdout.readline()
     # Run the function to strace
@@ -65,18 +71,31 @@
     log_file.seek(0)
     log = log_file.read()
     log_file.close()
-    return result, StraceResult(log)
+    # and stderr
+    err_file.seek(0)
+    err_messages = err_file.read()
+    err_file.close()
+    # and read any errors
+    if err_messages.startswith("attach: ptrace(PTRACE_ATTACH,"):
+        raise StraceError(err_messages=err_messages)
+    return result, StraceResult(log, err_messages)
+
+
+class StraceError(errors.BzrError):
+    
+    _fmt = "strace failed: %(err_messages)s"
 
 
 class StraceResult(object):
     """The result of stracing a function."""
 
-    def __init__(self, raw_log):
+    def __init__(self, raw_log, err_messages):
         """Create a StraceResult.
 
         :param raw_log: The output that strace created.
         """
         self.raw_log = raw_log
+        self.err_messages = err_messages
 
 
 class _StraceFeature(Feature):

=== modified file 'bzrlib/tests/test_strace.py'
--- bzrlib/tests/test_strace.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/test_strace.py	2010-09-01 08:49:39 +0000
@@ -1,5 +1,4 @@
-# Copyright (C) 2007 Canonical Ltd
-#   Authors: Robert Collins <[e-mail]>
+# Copyright (C) 2007, 2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -22,34 +21,26 @@
 import threading
 
 from bzrlib import (
+    strace,
     tests,
     )
 from bzrlib.strace import StraceFeature, strace_detailed, StraceResult
 
 
-class TestStraceFeature(tests.TestCaseWithTransport):
-
-    def test_strace_detection(self):
-        """Strace is available if its runnable."""
-        try:
-            proc = subprocess.Popen(['strace'],
-                stderr=subprocess.PIPE,
-                stdout=subprocess.PIPE)
-            proc.communicate()
-            found_strace = True
-        except OSError, e:
-            if e.errno == errno.ENOENT:
-                # strace is not installed
-                found_strace = False
-            else:
-                raise
-        self.assertEqual(found_strace, StraceFeature.available())
-
-
 class TestStrace(tests.TestCaseWithTransport):
 
     _test_needs_features = [StraceFeature]
 
+    def setUp(self):
+        # NB: see http://pad.lv/626679 and
+        # <https://code.launchpad.net/~mbp/bzr/626679-strace/+merge/34157>:
+        # testing strace by connecting to ourselves has repeatedly caused
+        # hangs in running the test suite; these are fixable given enough
+        # determination but given that strace is not used by any other tests
+        # at the moment and that it's only test-support code, we just leave it
+        # untested -- mbp 20100901
+        raise tests.TestSkipped("strace selftests are broken and disabled")
+
     def _check_threads(self):
         # For bug #226769, it was decided that the strace tests should not be
         # run when more than one thread is active. A lot of tests are currently
@@ -61,14 +52,26 @@
             raise tests.KnownFailure(
                 '%d active threads, bug #103133 needs to be fixed.' % active)
 
+    def strace_detailed_or_skip(self, *args, **kwargs):
+        """Run strace, but cope if it's not allowed"""
+        try:
+            return strace_detailed(*args, **kwargs)
+        except strace.StraceError, e:
+            if e.err_messages.startswith(
+                    "attach: ptrace(PTRACE_ATTACH, ...): Operation not permitted"):
+                raise tests.TestSkipped("ptrace not permitted")
+            else:
+                raise
+
     def test_strace_callable_is_called(self):
         self._check_threads()
 
         output = []
         def function(positional, *args, **kwargs):
             output.append((positional, args, kwargs))
-        strace_detailed(function, ["a", "b"], {"c": "c"},
-                        follow_children=False)
+        self.strace_detailed_or_skip(
+            function, ["a", "b"], {"c": "c"},
+            follow_children=False)
         self.assertEqual([("a", ("b",), {"c":"c"})], output)
 
     def test_strace_callable_result(self):
@@ -76,7 +79,7 @@
 
         def function():
             return "foo"
-        result, strace_result = strace_detailed(function,[], {},
+        result, strace_result = self.strace_detailed_or_skip(function,[], {},
                                                 follow_children=False)
         self.assertEqual("foo", result)
         self.assertIsInstance(strace_result, StraceResult)
@@ -87,6 +90,6 @@
 
         def function():
             self.build_tree(['myfile'])
-        unused, result = strace_detailed(function, [], {},
+        unused, result = self.strace_detailed_or_skip(function, [], {},
                                          follow_children=False)
         self.assertContainsRe(result.raw_log, 'myfile')