Bazaar

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

  1. lazy_hook_test_cleanup_785054
  2. Merge into bzr.dev
Proposed by Martin Packman
Status: Work in progress
Proposed branch: lp:~gz/bzr/lazy_hook_test_cleanup_785054
Merge into: lp:bzr
Diff against target: 161 lines (+144/-0)
1 file modified
bzrlib/tests/test_selftest.py (+144/-0)
To merge this branch: bzr merge lp:~gz/bzr/lazy_hook_test_cleanup_785054
Reviewer Review Type Date Requested Status
bzr-core Pending
Review via email: mp+61586@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Vincent Ladeuil (vila) wrote :

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

Reply
Revision history for this message
Martin Packman (gz) wrote :

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.

Reply

Unmerged revisions

5896. By Martin Packman

Split hook isolation tests into several cases in new class

5895. By Martin Packman

Add test for broken selftest lazy lock cleanup

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
43 branchbuilder,43 branchbuilder,
44 bzrdir,44 bzrdir,
45 errors,45 errors,
46 hooks,
46 lockdir,47 lockdir,
47 memorytree,48 memorytree,
48 osutils,49 osutils,
@@ -1005,6 +1006,149 @@
1005 self.assertEquals(2, result.count)1006 self.assertEquals(2, result.count)
10061007
10071008
1009class _TestingHooks(hooks.Hooks):
1010
1011 def __init__(self):
1012 hooks.Hooks.__init__(self, "bzrlib.tests.test_selftest", "_test_hooks")
1013 self.add_hook("go", "Called with a identifier string", (2, 4))
1014
1015
1016_test_hooks = _TestingHooks()
1017
1018
1019class TestHookIsolation(tests.TestCase):
1020 """Make sure use of hooks in tests stays within the testcase"""
1021
1022 def test_methods_called(self):
1023 """Ensure the basic hook isolation methods are called"""
1024 calls = []
1025 class InstrumentedTestCase(tests.TestCase):
1026 def _clear_hooks(self):
1027 super(InstrumentedTestCase, self)._clear_hooks()
1028 calls.append("_clear_hooks")
1029 def _restoreHooks(self):
1030 super(InstrumentedTestCase, self)._restoreHooks()
1031 calls.append("_restoreHooks")
1032 def test_pass(self):
1033 pass
1034 result = tests.ExtendedTestResult(StringIO(), None, None, None)
1035 InstrumentedTestCase("test_pass").run(result)
1036 self.assertEqual(calls, ["_clear_hooks", "_restoreHooks"])
1037
1038 def test_dict__lazy_hooks_restored(self):
1039 """The hooks._lazy_hooks dict should be different during the test run
1040
1041 Would prefer not to test this isolation implementation detail, but haven't
1042 found another way to write a failing test for an existing issue.
1043 """
1044 from bzrlib import hooks as _mod_hooks
1045 hook_dicts = {"before": _mod_hooks._lazy_hooks}
1046 class Test(tests.TestCase):
1047 def test_record__lazy_hooks(self):
1048 hook_dicts["during"] = _mod_hooks._lazy_hooks
1049 result = tests.ExtendedTestResult(StringIO(), None, None, None)
1050 Test("test_record__lazy_hooks").run(result)
1051 hook_dicts["after"] = _mod_hooks._lazy_hooks
1052 self.assertNotEqual(id(hook_dicts["before"]), id(hook_dicts["during"]))
1053 self.assertNotEqual(id(hook_dicts["after"]), id(hook_dicts["during"]))
1054 self.assertEqual(id(hook_dicts["before"]), id(hook_dicts["after"]))
1055
1056 def test_normal_hooks_unregistered_artificial(self):
1057 """A normal testing hook should be limited to the testcase lifetime
1058
1059 This fails because the hook never seems to get installed
1060 """
1061 def _trigger_hook(identifier):
1062 for hook in _test_hooks["go"]:
1063 hook(identifier)
1064 callback_log = StringIO()
1065 def _hook_callback(string):
1066 callback_log.write(string + "\n")
1067 class TestThatRegistersLazyHook(tests.TestCase):
1068 def test_lock_hook(inner_self):
1069 _test_hooks.install_named_hook(
1070 "go", _hook_callback, "test_lock_hook")
1071 _trigger_hook("inner")
1072 result = tests.ExtendedTestResult(StringIO(), None, None, None)
1073 TestThatRegistersLazyHook('test_lock_hook').run(result)
1074 _trigger_hook("outer")
1075 self.assertContainsString(callback_log.getvalue(), "inner")
1076 self.assertNotContainsString(callback_log.getvalue(), "outer")
1077
1078 def test_lazy_hooks_unregistered_artificial(self):
1079 """A lazy testing hook should be limited to the testcase lifetime
1080
1081 This fails because the hook escapes the inner test case
1082 """
1083 def _trigger_hook(identifier):
1084 for hook in _test_hooks["go"]:
1085 hook(identifier)
1086 callback_log = StringIO()
1087 def _hook_callback(string):
1088 callback_log.write(string + "\n")
1089 class TestThatRegistersLazyHook(tests.TestCase):
1090 def test_lock_hook(inner_self):
1091 hooks.install_lazy_named_hook("bzrlib.tests.test_selftest",
1092 "_test_hooks", "go", _hook_callback, "test_lock_hook")
1093 _trigger_hook("inner")
1094 result = tests.ExtendedTestResult(StringIO(), None, None, None)
1095 TestThatRegistersLazyHook('test_lock_hook').run(result)
1096 _trigger_hook("outer")
1097 self.assertContainsString(callback_log.getvalue(), "inner")
1098 self.assertNotContainsString(callback_log.getvalue(), "outer")
1099
1100 @staticmethod
1101 def _trigger_lock_hooks(lock_url):
1102 """Pretend to acquire and release a generic lock"""
1103 from bzrlib.lock import Lock, LockResult
1104 lock_result = LockResult(lock_url)
1105 for hook in Lock.hooks["lock_acquired"]:
1106 hook(lock_result)
1107 for hook in Lock.hooks["lock_released"]:
1108 hook(lock_result)
1109
1110 def test_normal_hooks_unregistered_lock(self):
1111 """A normal lock hook should be limited to the testcase lifetime"""
1112 from bzrlib.lock import Lock
1113 _trigger_hook = self._trigger_lock_hooks
1114 callback_log = StringIO()
1115 def _hook_callback(result):
1116 callback_log.write("lock %s\n" % (result.lock_url,))
1117 class TestThatRegistersLazyHook(tests.TestCase):
1118 def test_lock_hook(inner_self):
1119 Lock.hooks.install_named_hook(
1120 "lock_released", _hook_callback, "test_lock_hook")
1121 _trigger_hook("invalid:hook-inner")
1122 result = tests.ExtendedTestResult(StringIO(), None, None, None)
1123 TestThatRegistersLazyHook('test_lock_hook').run(result)
1124 self.assertContainsString(callback_log.getvalue(), "lock")
1125 callback_log.truncate(0)
1126 _trigger_hook("invalid:hook-outer")
1127 self.assertEqual("", callback_log.getvalue())
1128
1129 def test_lazy_hooks_unregistered_lock(self):
1130 """A lazy lock hook should be limited to the testcase lifetime
1131
1132 Somehow this test doesn't fail even if _restoreHooks doesn't set the
1133 hooks._lazy_hooks dictionary back to its previous instance.
1134 """
1135 _trigger_hook = self._trigger_lock_hooks
1136 callback_log = StringIO()
1137 def _hook_callback(result):
1138 callback_log.write("lock %s\n" % (result.lock_url,))
1139 class TestThatRegistersLazyHook(tests.TestCase):
1140 def test_lock_hook(inner_self):
1141 hooks.install_lazy_named_hook("bzrlib.lock", "Lock.hooks",
1142 "lock_released", _hook_callback, "test_lock_hook")
1143 _trigger_hook("invalid:hook-inner")
1144 result = tests.ExtendedTestResult(StringIO(), None, None, None)
1145 TestThatRegistersLazyHook('test_lock_hook').run(result)
1146 self.assertContainsString(callback_log.getvalue(), "lock")
1147 callback_log.truncate(0)
1148 _trigger_hook("invalid:hook-outer")
1149 self.assertEqual("", callback_log.getvalue())
1150
1151
1008class TestUnicodeFilenameFeature(tests.TestCase):1152class TestUnicodeFilenameFeature(tests.TestCase):
10091153
1010 def test_probe_passes(self):1154 def test_probe_passes(self):