Merge lp:~vila/bzr/failing-lock-tests into lp:~bzr/bzr/trunk-old

[Vincent Ladeuil]

[Vincent Ladeuil (vila)]

This fixed all the remaining failures (~30) encountered when running:

 bzr selftest -Dlock --no-plugins

We may want to wait a bit before switching to unconditional failure (-Dlock transform the warning into a failure for now), so that plugin authors have a bit more time to address their own failures (1.15rc1 is a bit too close to switch IMHO).

[Robert Collins (lifeless)]

 review approve

I think you should invert the sense of the test, so that
selftest -> lock mismatch is fatal
selftest -Dlock -> lock mismatch warns.

That will give plugin authors immediate feedback even if they use build
scripts or whatever.

We can loosen it up again for 1.15 if plugin authors are having
trouble.

-Rob

review: Approve

=== modified file 'NEWS'
--- NEWS	2009-05-12 03:47:18 +0000
+++ NEWS	2009-05-12 08:35:12 +0000
@@ -167,6 +167,12 @@
 
 * Updated the bundled ``ConfigObj`` library to 4.6.0 (Matt Nordhoff)
 
+Testing
+*******
+
+* ``bzr selftest`` will now fail if lock/unlock are not correctly balanced in
+  tests. Using ``-Dlock`` will turn the related failures into warnings.
+  (Vincent Ladeuil, Robert Collins)
 
 bzr 1.14
 ###########
 
=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2009-05-11 06:44:30 +0000
+++ bzrlib/tests/__init__.py	2009-05-12 08:35:12 +0000
@@ -884,7 +884,7 @@
     def _track_locks(self):
         """Track lock activity during tests."""
         self._lock_actions = []
-        self._lock_check_thorough = 'lock' in debug.debug_flags
+        self._lock_check_thorough = 'lock' not in debug.debug_flags
         self.addCleanup(self._check_locks)
         _mod_lock.Lock.hooks.install_named_hook('lock_acquired',
                                                 self._lock_acquired, None)
 
=== modified file 'bzrlib/tests/branch_implementations/test_locking.py'
--- bzrlib/tests/branch_implementations/test_locking.py	2009-04-15 05:39:38 +0000
+++ bzrlib/tests/branch_implementations/test_locking.py	2009-05-12 08:35:12 +0000
@@ -432,6 +432,10 @@
             branch.unlock()
         # We should be unable to relock the repo.
         self.assertRaises(errors.LockContention, branch.lock_write)
+        # Cleanup
+        branch.lock_write(token)
+        branch.dont_leave_lock_in_place()
+        branch.unlock()
 
     def test_dont_leave_lock_in_place(self):
         branch = self.make_branch('b')
 
=== modified file 'bzrlib/tests/per_repository/test_repository.py'
--- bzrlib/tests/per_repository/test_repository.py	2009-04-09 20:23:07 +0000
+++ bzrlib/tests/per_repository/test_repository.py	2009-05-12 08:35:12 +0000
@@ -1046,6 +1046,10 @@
             repo.unlock()
         # We should be unable to relock the repo.
         self.assertRaises(errors.LockContention, repo.lock_write)
+        # Cleanup
+        repo.lock_write(token)
+        repo.dont_leave_lock_in_place()
+        repo.unlock()
 
     def test_dont_leave_lock_in_place(self):
         repo = self.make_repository('r')
 
=== modified file 'bzrlib/tests/per_repository/test_write_group.py'
--- bzrlib/tests/per_repository/test_write_group.py	2009-05-12 01:36:08 +0000
+++ bzrlib/tests/per_repository/test_write_group.py	2009-05-12 08:35:12 +0000
@@ -112,13 +112,13 @@
         self.addCleanup(repo.unlock)
         repo.start_write_group()
         # Damage the repository on the filesystem
-        self.get_transport('').rename('repo', 'foo')
+        t = self.get_transport('')
+        t.rename('repo', 'foo')
+        self.addCleanup(t.rename, 'foo', 'repo')
         # abort_write_group will not raise an error, because either an
         # exception was not generated, or the exception was caught and
         # suppressed.  See also test_pack_repository's test of the same name.
         self.assertEqual(None, repo.abort_write_group(suppress_errors=True))
-        if token is not None:
-            repo.leave_lock_in_place()
 
 class TestGetMissingParentInventories(TestCaseWithRepository):
 
 
=== modified file 'bzrlib/tests/test_lockable_files.py'
--- bzrlib/tests/test_lockable_files.py	2009-04-04 02:50:01 +0000
+++ bzrlib/tests/test_lockable_files.py	2009-05-12 08:35:12 +0000
@@ -56,72 +56,68 @@
             deprecated_in((1, 5, 0)),
             self.lockable.get_utf8, 'foo')
         self.lockable.lock_write()
-        try:
-            unicode_string = u'bar\u1234'
-            self.assertEqual(4, len(unicode_string))
-            byte_string = unicode_string.encode('utf-8')
-            self.assertEqual(6, len(byte_string))
-            self.assertRaises(UnicodeEncodeError,
-                self.applyDeprecated,
-                deprecated_in((1, 6, 0)),
-                self.lockable.put, 'foo',
-                StringIO(unicode_string))
-            self.applyDeprecated(
-                deprecated_in((1, 6, 0)),
-                self.lockable.put,
-                'foo', StringIO(byte_string))
-            byte_stream = self.applyDeprecated(
-                deprecated_in((1, 5, 0)),
-                self.lockable.get,
-                'foo')
-            self.assertEqual(byte_string, byte_stream.read())
-            unicode_stream = self.applyDeprecated(
-                deprecated_in((1, 5, 0)),
-                self.lockable.get_utf8,
-                'foo')
-            self.assertEqual(unicode_string,
-                unicode_stream.read())
-            self.assertRaises(BzrBadParameterNotString,
-                self.applyDeprecated,
-                deprecated_in((1, 6, 0)),
-                self.lockable.put_utf8,
-                'bar',
-                StringIO(unicode_string))
-            self.applyDeprecated(
-                deprecated_in((1, 6, 0)),
-                self.lockable.put_utf8,
-                'bar',
-                unicode_string)
-            unicode_stream = self.applyDeprecated(
-                deprecated_in((1, 5, 0)),
-                self.lockable.get_utf8,
-                'bar')
-            self.assertEqual(unicode_string,
-                unicode_stream.read())
-            byte_stream = self.applyDeprecated(
-                deprecated_in((1, 5, 0)),
-                self.lockable.get,
-                'bar')
-            self.assertEqual(byte_string, byte_stream.read())
-            self.applyDeprecated(
-                deprecated_in((1, 6, 0)),
-                self.lockable.put_bytes,
-                'raw', 'raw\xffbytes')
-            byte_stream = self.applyDeprecated(
-                deprecated_in((1, 5, 0)),
-                self.lockable.get,
-                'raw')
-            self.assertEqual('raw\xffbytes', byte_stream.read())
-        finally:
-            self.lockable.unlock()
+        self.addCleanup(self.lockable.unlock)
+        unicode_string = u'bar\u1234'
+        self.assertEqual(4, len(unicode_string))
+        byte_string = unicode_string.encode('utf-8')
+        self.assertEqual(6, len(byte_string))
+        self.assertRaises(UnicodeEncodeError,
+            self.applyDeprecated,
+            deprecated_in((1, 6, 0)),
+            self.lockable.put, 'foo',
+            StringIO(unicode_string))
+        self.applyDeprecated(
+            deprecated_in((1, 6, 0)),
+            self.lockable.put,
+            'foo', StringIO(byte_string))
+        byte_stream = self.applyDeprecated(
+            deprecated_in((1, 5, 0)),
+            self.lockable.get,
+            'foo')
+        self.assertEqual(byte_string, byte_stream.read())
+        unicode_stream = self.applyDeprecated(
+            deprecated_in((1, 5, 0)),
+            self.lockable.get_utf8,
+            'foo')
+        self.assertEqual(unicode_string,
+            unicode_stream.read())
+        self.assertRaises(BzrBadParameterNotString,
+            self.applyDeprecated,
+            deprecated_in((1, 6, 0)),
+            self.lockable.put_utf8,
+            'bar',
+            StringIO(unicode_string))
+        self.applyDeprecated(
+            deprecated_in((1, 6, 0)),
+            self.lockable.put_utf8,
+            'bar',
+            unicode_string)
+        unicode_stream = self.applyDeprecated(
+            deprecated_in((1, 5, 0)),
+            self.lockable.get_utf8,
+            'bar')
+        self.assertEqual(unicode_string,
+            unicode_stream.read())
+        byte_stream = self.applyDeprecated(
+            deprecated_in((1, 5, 0)),
+            self.lockable.get,
+            'bar')
+        self.assertEqual(byte_string, byte_stream.read())
+        self.applyDeprecated(
+            deprecated_in((1, 6, 0)),
+            self.lockable.put_bytes,
+            'raw', 'raw\xffbytes')
+        byte_stream = self.applyDeprecated(
+            deprecated_in((1, 5, 0)),
+            self.lockable.get,
+            'raw')
+        self.assertEqual('raw\xffbytes', byte_stream.read())
 
     def test_locks(self):
         self.lockable.lock_read()
-        try:
-            self.assertRaises(ReadOnlyError, self.lockable.put, 'foo',
-                              StringIO('bar\u1234'))
-        finally:
-            self.lockable.unlock()
+        self.addCleanup(self.lockable.unlock)
+        self.assertRaises(ReadOnlyError, self.lockable.put, 'foo',
+                          StringIO('bar\u1234'))
 
     def test_transactions(self):
         self.assertIs(self.lockable.get_transaction().__class__,
@@ -176,92 +172,80 @@
 
     def test_lock_write_returns_None_refuses_token(self):
         token = self.lockable.lock_write()
-        try:
-            if token is not None:
-                # This test does not apply, because this lockable supports
-                # tokens.
-                raise TestNotApplicable("%r uses tokens" % (self.lockable,))
-            self.assertRaises(errors.TokenLockingNotSupported,
-                              self.lockable.lock_write, token='token')
-        finally:
-            self.lockable.unlock()
+        self.addCleanup(self.lockable.unlock)
+        if token is not None:
+            # This test does not apply, because this lockable supports
+            # tokens.
+            raise TestNotApplicable("%r uses tokens" % (self.lockable,))
+        self.assertRaises(errors.TokenLockingNotSupported,
+                          self.lockable.lock_write, token='token')
 
     def test_lock_write_returns_token_when_given_token(self):
         token = self.lockable.lock_write()
-        try:
-            if token is None:
-                # This test does not apply, because this lockable refuses
-                # tokens.
-                return
-            new_lockable = self.get_lockable()
-            token_from_new_lockable = new_lockable.lock_write(token=token)
-            try:
-                self.assertEqual(token, token_from_new_lockable)
-            finally:
-                new_lockable.unlock()
-        finally:
-            self.lockable.unlock()
+        self.addCleanup(self.lockable.unlock)
+        if token is None:
+            # This test does not apply, because this lockable refuses
+            # tokens.
+            return
+        new_lockable = self.get_lockable()
+        token_from_new_lockable = new_lockable.lock_write(token=token)
+        self.addCleanup(new_lockable.unlock)
+        self.assertEqual(token, token_from_new_lockable)
 
     def test_lock_write_raises_on_token_mismatch(self):
         token = self.lockable.lock_write()
-        try:
-            if token is None:
-                # This test does not apply, because this lockable refuses
-                # tokens.
-                return
-            different_token = token + 'xxx'
-            # Re-using the same lockable instance with a different token will
-            # raise TokenMismatch.
-            self.assertRaises(errors.TokenMismatch,
-                              self.lockable.lock_write, token=different_token)
-            # A separate instance for the same lockable will also raise
-            # TokenMismatch.
-            # This detects the case where a caller claims to have a lock (via
-            # the token) for an external resource, but doesn't (the token is
-            # different).  Clients need a separate lock object to make sure the
-            # external resource is probed, whereas the existing lock object
-            # might cache.
-            new_lockable = self.get_lockable()
-            self.assertRaises(errors.TokenMismatch,
-                              new_lockable.lock_write, token=different_token)
-        finally:
-            self.lockable.unlock()
+        self.addCleanup(self.lockable.unlock)
+        if token is None:
+            # This test does not apply, because this lockable refuses
+            # tokens.
+            return
+        different_token = token + 'xxx'
+        # Re-using the same lockable instance with a different token will
+        # raise TokenMismatch.
+        self.assertRaises(errors.TokenMismatch,
+                          self.lockable.lock_write, token=different_token)
+        # A separate instance for the same lockable will also raise
+        # TokenMismatch.
+        # This detects the case where a caller claims to have a lock (via
+        # the token) for an external resource, but doesn't (the token is
+        # different).  Clients need a separate lock object to make sure the
+        # external resource is probed, whereas the existing lock object
+        # might cache.
+        new_lockable = self.get_lockable()
+        self.assertRaises(errors.TokenMismatch,
+                          new_lockable.lock_write, token=different_token)
 
     def test_lock_write_with_matching_token(self):
         # If the token matches, so no exception is raised by lock_write.
         token = self.lockable.lock_write()
-        try:
-            if token is None:
-                # This test does not apply, because this lockable refuses
-                # tokens.
-                return
-            # The same instance will accept a second lock_write if the specified
-            # token matches.
-            self.lockable.lock_write(token=token)
-            self.lockable.unlock()
-            # Calling lock_write on a new instance for the same lockable will
-            # also succeed.
-            new_lockable = self.get_lockable()
-            new_lockable.lock_write(token=token)
-            new_lockable.unlock()
-        finally:
-            self.lockable.unlock()
+        self.addCleanup(self.lockable.unlock)
+        if token is None:
+            # This test does not apply, because this lockable refuses
+            # tokens.
+            return
+        # The same instance will accept a second lock_write if the specified
+        # token matches.
+        self.lockable.lock_write(token=token)
+        self.lockable.unlock()
+        # Calling lock_write on a new instance for the same lockable will
+        # also succeed.
+        new_lockable = self.get_lockable()
+        new_lockable.lock_write(token=token)
+        new_lockable.unlock()
 
     def test_unlock_after_lock_write_with_token(self):
         # If lock_write did not physically acquire the lock (because it was
         # passed a token), then unlock should not physically release it.
         token = self.lockable.lock_write()
-        try:
-            if token is None:
-                # This test does not apply, because this lockable refuses
-                # tokens.
-                return
-            new_lockable = self.get_lockable()
-            new_lockable.lock_write(token=token)
-            new_lockable.unlock()
-            self.assertTrue(self.lockable.get_physical_lock_status())
-        finally:
-            self.lockable.unlock()
+        self.addCleanup(self.lockable.unlock)
+        if token is None:
+            # This test does not apply, because this lockable refuses
+            # tokens.
+            return
+        new_lockable = self.get_lockable()
+        new_lockable.lock_write(token=token)
+        new_lockable.unlock()
+        self.assertTrue(self.lockable.get_physical_lock_status())
 
     def test_lock_write_with_token_fails_when_unlocked(self):
         # Lock and unlock to get a superficially valid token.  This mimics a
@@ -332,6 +316,11 @@
         # But should be relockable with a token.
         self.lockable.lock_write(token=token)
         self.lockable.unlock()
+        # Cleanup: we should still be able to get the lock, but we restore the
+        # behavior to clearing the lock when unlocking.
+        self.lockable.lock_write(token=token)
+        self.lockable.dont_leave_in_place()
+        self.lockable.unlock()
 
     def test_dont_leave_in_place(self):
         token = self.lockable.lock_write()
 
=== modified file 'bzrlib/tests/test_selftest.py'
--- bzrlib/tests/test_selftest.py	2009-04-30 06:16:30 +0000
+++ bzrlib/tests/test_selftest.py	2009-05-12 08:35:12 +0000
@@ -622,9 +622,6 @@
         self.assertRaises(AssertionError, self._check_safety_net)
 
     def test_dangling_locks_cause_failures(self):
-        # This is currently only enabled during debug runs, so turn debugging
-        # on.
-        debug.debug_flags.add('lock')
         class TestDanglingLock(TestCaseWithMemoryTransport):
             def test_function(self):
                 t = self.get_transport('.')
 
=== modified file 'bzrlib/tests/test_smart.py'
--- bzrlib/tests/test_smart.py	2009-04-28 03:55:56 +0000
+++ bzrlib/tests/test_smart.py	2009-05-12 08:35:12 +0000
@@ -586,6 +586,8 @@
             '')
         self.assertEqual(SuccessfulSmartServerResponse(()), result)
         self.assertEqual('bar', config.get_option('foo'))
+        # Cleanup
+        branch.unlock()
 
     def test_value_name_section(self):
         branch = self.make_branch('.')
@@ -597,6 +599,8 @@
             'gam')
         self.assertEqual(SuccessfulSmartServerResponse(()), result)
         self.assertEqual('bar', config.get_option('foo', 'gam'))
+        # Cleanup
+        branch.unlock()
 
 
 class SetLastRevisionTestBase(TestLockedBranch):
@@ -915,17 +919,24 @@
         # with a new branch object.
         new_branch = repository.bzrdir.open_branch()
         self.assertRaises(errors.LockContention, new_branch.lock_write)
+        # Cleanup
+        request = smart.branch.SmartServerBranchRequestUnlock(backing)
+        response = request.execute('', branch_nonce, repository_nonce)
 
     def test_lock_write_on_locked_branch(self):
         backing = self.get_transport()
         request = smart.branch.SmartServerBranchRequestLockWrite(backing)
         branch = self.make_branch('.')
-        branch.lock_write()
+        branch_token = branch.lock_write()
         branch.leave_lock_in_place()
         branch.unlock()
         response = request.execute('')
         self.assertEqual(
             SmartServerResponse(('LockContention',)), response)
+        # Cleanup
+        branch.lock_write(branch_token)
+        branch.dont_leave_lock_in_place()
+        branch.unlock()
 
     def test_lock_write_with_tokens_on_locked_branch(self):
         backing = self.get_transport()
@@ -941,6 +952,13 @@
                                    branch_token, repo_token)
         self.assertEqual(
             SmartServerResponse(('ok', branch_token, repo_token)), response)
+        # Cleanup
+        branch.repository.lock_write(repo_token)
+        branch.repository.dont_leave_lock_in_place()
+        branch.repository.unlock()
+        branch.lock_write(branch_token)
+        branch.dont_leave_lock_in_place()
+        branch.unlock()
 
     def test_lock_write_with_mismatched_tokens_on_locked_branch(self):
         backing = self.get_transport()
@@ -956,17 +974,29 @@
                                    branch_token+'xxx', repo_token)
         self.assertEqual(
             SmartServerResponse(('TokenMismatch',)), response)
+        # Cleanup
+        branch.repository.lock_write(repo_token)
+        branch.repository.dont_leave_lock_in_place()
+        branch.repository.unlock()
+        branch.lock_write(branch_token)
+        branch.dont_leave_lock_in_place()
+        branch.unlock()
 
     def test_lock_write_on_locked_repo(self):
         backing = self.get_transport()
         request = smart.branch.SmartServerBranchRequestLockWrite(backing)
         branch = self.make_branch('.', format='knit')
-        branch.repository.lock_write()
-        branch.repository.leave_lock_in_place()
-        branch.repository.unlock()
+        repo = branch.repository
+        repo_token = repo.lock_write()
+        repo.leave_lock_in_place()
+        repo.unlock()
         response = request.execute('')
         self.assertEqual(
             SmartServerResponse(('LockContention',)), response)
+        # Cleanup
+        repo.lock_write(repo_token)
+        repo.dont_leave_lock_in_place()
+        repo.unlock()
 
     def test_lock_write_on_readonly_transport(self):
         backing = self.get_readonly_transport()
@@ -1031,6 +1061,10 @@
             '', 'branch token', repo_token)
         self.assertEqual(
             SmartServerResponse(('TokenMismatch',)), response)
+        # Cleanup
+        branch.repository.lock_write(repo_token)
+        branch.repository.dont_leave_lock_in_place()
+        branch.repository.unlock()
 
 
 class TestSmartServerRepositoryRequest(tests.TestCaseWithMemoryTransport):
@@ -1282,17 +1316,24 @@
         # object.
         new_repo = repository.bzrdir.open_repository()
         self.assertRaises(errors.LockContention, new_repo.lock_write)
+        # Cleanup
+        request = smart.repository.SmartServerRepositoryUnlock(backing)
+        response = request.execute('', nonce)
 
     def test_lock_write_on_locked_repo(self):
         backing = self.get_transport()
         request = smart.repository.SmartServerRepositoryLockWrite(backing)
         repository = self.make_repository('.', format='knit')
-        repository.lock_write()
+        repo_token = repository.lock_write()
         repository.leave_lock_in_place()
         repository.unlock()
         response = request.execute('')
         self.assertEqual(
             SmartServerResponse(('LockContention',)), response)
+        # Cleanup
+        repository.lock_write(repo_token)
+        repository.dont_leave_lock_in_place()
+        repository.unlock()
 
     def test_lock_write_on_readonly_transport(self):
         backing = self.get_readonly_transport()