Merge lp:~gz/bzr/lazy_hook_test_cleanup_785054 into lp:bzr

[Martin Packman]

This branch isn't ready to land (it doesn't fix the bug), but I need some help working out what's going on with the hooks isolation logic.

The branch is motivated by this mistake in the existing code, which looks like a simple fix:

=== modified file 'bzrlib/tests/__init__.py'
--- old/bzrlib/tests/__init__.py 2011-05-17 17:03:58 +0000
+++ new/bzrlib/tests/__init__.py 2011-05-19 14:53:38 +0000
@@ -1699,7 +1699,7 @@
     def _restoreHooks(self):
         for klass, (name, hooks) in self._preserved_hooks.items():
             setattr(klass, name, hooks)
- hooks._lazy_hooks = self._preserved_lazy_hooks
+ bzrlib.hooks._lazy_hooks = self._preserved_lazy_hooks

     def knownFailure(self, reason):
         """This test has failed for some known reason."""

As the _preserved_lazy_hooks dict is never restored, I'd expect the added test_lazy_hooks_unregistered_lock to fail, but it passes with or without the change. Also, the two test_*_artificial cases fail to do what I expect them to whatever.

Not restoring the dictionary doesn't seem to break much in practice at the moment, but is another testcase lifetime complication.

[Vincent Ladeuil (vila)]

Oh hmm, I thought I commented here but we mostly discussed on IRC, don't hesitate to ping if you need more help.

[Martin Packman (gz)]

I've discussed this on IRC with John and Vincent, and will reduce the tests to a sane subset and try removing hook cleanup code and see if things stay working.

Unmerged revisions

5896. By Martin Packman on 2011-05-19

Split hook isolation tests into several cases in new class

5895. By Martin Packman on 2011-05-19

Add test for broken selftest lazy lock cleanup

=== modified file 'bzrlib/tests/test_selftest.py'
--- bzrlib/tests/test_selftest.py	2011-05-18 19:49:22 +0000
+++ bzrlib/tests/test_selftest.py	2011-05-19 15:13:33 +0000
@@ -43,6 +43,7 @@
     branchbuilder,
     bzrdir,
     errors,
+    hooks,
     lockdir,
     memorytree,
     osutils,
@@ -1005,6 +1006,149 @@
         self.assertEquals(2, result.count)
 
 
+class _TestingHooks(hooks.Hooks):
+
+    def __init__(self):
+        hooks.Hooks.__init__(self, "bzrlib.tests.test_selftest", "_test_hooks")
+        self.add_hook("go", "Called with a identifier string", (2, 4))
+
+
+_test_hooks = _TestingHooks()
+
+
+class TestHookIsolation(tests.TestCase):
+    """Make sure use of hooks in tests stays within the testcase"""
+
+    def test_methods_called(self):
+        """Ensure the basic hook isolation methods are called"""
+        calls = []
+        class InstrumentedTestCase(tests.TestCase):
+            def _clear_hooks(self):
+                super(InstrumentedTestCase, self)._clear_hooks()
+                calls.append("_clear_hooks")
+            def _restoreHooks(self):
+                super(InstrumentedTestCase, self)._restoreHooks()
+                calls.append("_restoreHooks")
+            def test_pass(self):
+                pass
+        result = tests.ExtendedTestResult(StringIO(), None, None, None)
+        InstrumentedTestCase("test_pass").run(result)
+        self.assertEqual(calls, ["_clear_hooks", "_restoreHooks"])
+
+    def test_dict__lazy_hooks_restored(self):
+        """The hooks._lazy_hooks dict should be different during the test run
+
+        Would prefer not to test this isolation implementation detail, but haven't
+        found another way to write a failing test for an existing issue.
+        """
+        from bzrlib import hooks as _mod_hooks
+        hook_dicts = {"before": _mod_hooks._lazy_hooks}
+        class Test(tests.TestCase):
+            def test_record__lazy_hooks(self):
+                hook_dicts["during"] = _mod_hooks._lazy_hooks
+        result = tests.ExtendedTestResult(StringIO(), None, None, None)
+        Test("test_record__lazy_hooks").run(result)
+        hook_dicts["after"] = _mod_hooks._lazy_hooks
+        self.assertNotEqual(id(hook_dicts["before"]), id(hook_dicts["during"]))
+        self.assertNotEqual(id(hook_dicts["after"]), id(hook_dicts["during"]))
+        self.assertEqual(id(hook_dicts["before"]), id(hook_dicts["after"]))
+
+    def test_normal_hooks_unregistered_artificial(self):
+        """A normal testing hook should be limited to the testcase lifetime
+
+        This fails because the hook never seems to get installed
+        """
+        def _trigger_hook(identifier):
+            for hook in _test_hooks["go"]:
+                hook(identifier)
+        callback_log = StringIO()
+        def _hook_callback(string):
+            callback_log.write(string + "\n")
+        class TestThatRegistersLazyHook(tests.TestCase):
+            def test_lock_hook(inner_self):
+                _test_hooks.install_named_hook(
+                    "go", _hook_callback, "test_lock_hook")
+                _trigger_hook("inner")
+        result = tests.ExtendedTestResult(StringIO(), None, None, None)
+        TestThatRegistersLazyHook('test_lock_hook').run(result)
+        _trigger_hook("outer")
+        self.assertContainsString(callback_log.getvalue(), "inner")
+        self.assertNotContainsString(callback_log.getvalue(), "outer")
+
+    def test_lazy_hooks_unregistered_artificial(self):
+        """A lazy testing hook should be limited to the testcase lifetime
+
+        This fails because the hook escapes the inner test case
+        """
+        def _trigger_hook(identifier):
+            for hook in _test_hooks["go"]:
+                hook(identifier)
+        callback_log = StringIO()
+        def _hook_callback(string):
+            callback_log.write(string + "\n")
+        class TestThatRegistersLazyHook(tests.TestCase):
+            def test_lock_hook(inner_self):
+                hooks.install_lazy_named_hook("bzrlib.tests.test_selftest",
+                    "_test_hooks", "go", _hook_callback, "test_lock_hook")
+                _trigger_hook("inner")
+        result = tests.ExtendedTestResult(StringIO(), None, None, None)
+        TestThatRegistersLazyHook('test_lock_hook').run(result)
+        _trigger_hook("outer")
+        self.assertContainsString(callback_log.getvalue(), "inner")
+        self.assertNotContainsString(callback_log.getvalue(), "outer")
+
+    @staticmethod
+    def _trigger_lock_hooks(lock_url):
+        """Pretend to acquire and release a generic lock"""
+        from bzrlib.lock import Lock, LockResult
+        lock_result = LockResult(lock_url)
+        for hook in Lock.hooks["lock_acquired"]:
+            hook(lock_result)
+        for hook in Lock.hooks["lock_released"]:
+            hook(lock_result)
+
+    def test_normal_hooks_unregistered_lock(self):
+        """A normal lock hook should be limited to the testcase lifetime"""
+        from bzrlib.lock import Lock
+        _trigger_hook = self._trigger_lock_hooks
+        callback_log = StringIO()
+        def _hook_callback(result):
+            callback_log.write("lock %s\n" % (result.lock_url,))
+        class TestThatRegistersLazyHook(tests.TestCase):
+            def test_lock_hook(inner_self):
+                Lock.hooks.install_named_hook(
+                    "lock_released", _hook_callback, "test_lock_hook")
+                _trigger_hook("invalid:hook-inner")
+        result = tests.ExtendedTestResult(StringIO(), None, None, None)
+        TestThatRegistersLazyHook('test_lock_hook').run(result)
+        self.assertContainsString(callback_log.getvalue(), "lock")
+        callback_log.truncate(0)
+        _trigger_hook("invalid:hook-outer")
+        self.assertEqual("", callback_log.getvalue())
+
+    def test_lazy_hooks_unregistered_lock(self):
+        """A lazy lock hook should be limited to the testcase lifetime
+
+        Somehow this test doesn't fail even if _restoreHooks doesn't set the
+        hooks._lazy_hooks dictionary back to its previous instance.
+        """
+        _trigger_hook = self._trigger_lock_hooks
+        callback_log = StringIO()
+        def _hook_callback(result):
+            callback_log.write("lock %s\n" % (result.lock_url,))
+        class TestThatRegistersLazyHook(tests.TestCase):
+            def test_lock_hook(inner_self):
+                hooks.install_lazy_named_hook("bzrlib.lock", "Lock.hooks",
+                    "lock_released", _hook_callback, "test_lock_hook")
+                _trigger_hook("invalid:hook-inner")
+        result = tests.ExtendedTestResult(StringIO(), None, None, None)
+        TestThatRegistersLazyHook('test_lock_hook').run(result)
+        self.assertContainsString(callback_log.getvalue(), "lock")
+        callback_log.truncate(0)
+        _trigger_hook("invalid:hook-outer")
+        self.assertEqual("", callback_log.getvalue())
+
+
 class TestUnicodeFilenameFeature(tests.TestCase):
 
     def test_probe_passes(self):