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

Proposed by Vincent Ladeuil on 2009-05-11
Status: Merged
Merged at revision: not available
Proposed branch: lp:~vila/bzr/failing-lock-tests
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 526 lines
To merge this branch: bzr merge lp:~vila/bzr/failing-lock-tests
Reviewer Review Type Date Requested Status
Robert Collins (community) 2009-05-11 Approve on 2009-05-11
Review via email: mp+6418@code.launchpad.net
To post a comment you must log in.
Vincent Ladeuil (vila) wrote :

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) wrote :

 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
lp:~vila/bzr/failing-lock-tests updated on 2009-05-12
4339. By Vincent Ladeuil on 2009-05-12

Fixed as per Robert's review.

* tests/__init__.py:
(TestCase._track_locks): Make unbalanced locks fatal.

* tests/test_selftest.py:
(TestTestCaseWithMemoryTransport.test_dangling_locks_cause_failures):
The failure is now unconditional.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-05-12 03:47:18 +0000
3+++ NEWS 2009-05-12 08:35:12 +0000
4@@ -167,6 +167,12 @@
5
6 * Updated the bundled ``ConfigObj`` library to 4.6.0 (Matt Nordhoff)
7
8+Testing
9+*******
10+
11+* ``bzr selftest`` will now fail if lock/unlock are not correctly balanced in
12+ tests. Using ``-Dlock`` will turn the related failures into warnings.
13+ (Vincent Ladeuil, Robert Collins)
14
15 bzr 1.14
16 ###########
17
18=== modified file 'bzrlib/tests/__init__.py'
19--- bzrlib/tests/__init__.py 2009-05-11 06:44:30 +0000
20+++ bzrlib/tests/__init__.py 2009-05-12 08:35:12 +0000
21@@ -884,7 +884,7 @@
22 def _track_locks(self):
23 """Track lock activity during tests."""
24 self._lock_actions = []
25- self._lock_check_thorough = 'lock' in debug.debug_flags
26+ self._lock_check_thorough = 'lock' not in debug.debug_flags
27 self.addCleanup(self._check_locks)
28 _mod_lock.Lock.hooks.install_named_hook('lock_acquired',
29 self._lock_acquired, None)
30
31=== modified file 'bzrlib/tests/branch_implementations/test_locking.py'
32--- bzrlib/tests/branch_implementations/test_locking.py 2009-04-15 05:39:38 +0000
33+++ bzrlib/tests/branch_implementations/test_locking.py 2009-05-12 08:35:12 +0000
34@@ -432,6 +432,10 @@
35 branch.unlock()
36 # We should be unable to relock the repo.
37 self.assertRaises(errors.LockContention, branch.lock_write)
38+ # Cleanup
39+ branch.lock_write(token)
40+ branch.dont_leave_lock_in_place()
41+ branch.unlock()
42
43 def test_dont_leave_lock_in_place(self):
44 branch = self.make_branch('b')
45
46=== modified file 'bzrlib/tests/per_repository/test_repository.py'
47--- bzrlib/tests/per_repository/test_repository.py 2009-04-09 20:23:07 +0000
48+++ bzrlib/tests/per_repository/test_repository.py 2009-05-12 08:35:12 +0000
49@@ -1046,6 +1046,10 @@
50 repo.unlock()
51 # We should be unable to relock the repo.
52 self.assertRaises(errors.LockContention, repo.lock_write)
53+ # Cleanup
54+ repo.lock_write(token)
55+ repo.dont_leave_lock_in_place()
56+ repo.unlock()
57
58 def test_dont_leave_lock_in_place(self):
59 repo = self.make_repository('r')
60
61=== modified file 'bzrlib/tests/per_repository/test_write_group.py'
62--- bzrlib/tests/per_repository/test_write_group.py 2009-05-12 01:36:08 +0000
63+++ bzrlib/tests/per_repository/test_write_group.py 2009-05-12 08:35:12 +0000
64@@ -112,13 +112,13 @@
65 self.addCleanup(repo.unlock)
66 repo.start_write_group()
67 # Damage the repository on the filesystem
68- self.get_transport('').rename('repo', 'foo')
69+ t = self.get_transport('')
70+ t.rename('repo', 'foo')
71+ self.addCleanup(t.rename, 'foo', 'repo')
72 # abort_write_group will not raise an error, because either an
73 # exception was not generated, or the exception was caught and
74 # suppressed. See also test_pack_repository's test of the same name.
75 self.assertEqual(None, repo.abort_write_group(suppress_errors=True))
76- if token is not None:
77- repo.leave_lock_in_place()
78
79 class TestGetMissingParentInventories(TestCaseWithRepository):
80
81
82=== modified file 'bzrlib/tests/test_lockable_files.py'
83--- bzrlib/tests/test_lockable_files.py 2009-04-04 02:50:01 +0000
84+++ bzrlib/tests/test_lockable_files.py 2009-05-12 08:35:12 +0000
85@@ -56,72 +56,68 @@
86 deprecated_in((1, 5, 0)),
87 self.lockable.get_utf8, 'foo')
88 self.lockable.lock_write()
89- try:
90- unicode_string = u'bar\u1234'
91- self.assertEqual(4, len(unicode_string))
92- byte_string = unicode_string.encode('utf-8')
93- self.assertEqual(6, len(byte_string))
94- self.assertRaises(UnicodeEncodeError,
95- self.applyDeprecated,
96- deprecated_in((1, 6, 0)),
97- self.lockable.put, 'foo',
98- StringIO(unicode_string))
99- self.applyDeprecated(
100- deprecated_in((1, 6, 0)),
101- self.lockable.put,
102- 'foo', StringIO(byte_string))
103- byte_stream = self.applyDeprecated(
104- deprecated_in((1, 5, 0)),
105- self.lockable.get,
106- 'foo')
107- self.assertEqual(byte_string, byte_stream.read())
108- unicode_stream = self.applyDeprecated(
109- deprecated_in((1, 5, 0)),
110- self.lockable.get_utf8,
111- 'foo')
112- self.assertEqual(unicode_string,
113- unicode_stream.read())
114- self.assertRaises(BzrBadParameterNotString,
115- self.applyDeprecated,
116- deprecated_in((1, 6, 0)),
117- self.lockable.put_utf8,
118- 'bar',
119- StringIO(unicode_string))
120- self.applyDeprecated(
121- deprecated_in((1, 6, 0)),
122- self.lockable.put_utf8,
123- 'bar',
124- unicode_string)
125- unicode_stream = self.applyDeprecated(
126- deprecated_in((1, 5, 0)),
127- self.lockable.get_utf8,
128- 'bar')
129- self.assertEqual(unicode_string,
130- unicode_stream.read())
131- byte_stream = self.applyDeprecated(
132- deprecated_in((1, 5, 0)),
133- self.lockable.get,
134- 'bar')
135- self.assertEqual(byte_string, byte_stream.read())
136- self.applyDeprecated(
137- deprecated_in((1, 6, 0)),
138- self.lockable.put_bytes,
139- 'raw', 'raw\xffbytes')
140- byte_stream = self.applyDeprecated(
141- deprecated_in((1, 5, 0)),
142- self.lockable.get,
143- 'raw')
144- self.assertEqual('raw\xffbytes', byte_stream.read())
145- finally:
146- self.lockable.unlock()
147+ self.addCleanup(self.lockable.unlock)
148+ unicode_string = u'bar\u1234'
149+ self.assertEqual(4, len(unicode_string))
150+ byte_string = unicode_string.encode('utf-8')
151+ self.assertEqual(6, len(byte_string))
152+ self.assertRaises(UnicodeEncodeError,
153+ self.applyDeprecated,
154+ deprecated_in((1, 6, 0)),
155+ self.lockable.put, 'foo',
156+ StringIO(unicode_string))
157+ self.applyDeprecated(
158+ deprecated_in((1, 6, 0)),
159+ self.lockable.put,
160+ 'foo', StringIO(byte_string))
161+ byte_stream = self.applyDeprecated(
162+ deprecated_in((1, 5, 0)),
163+ self.lockable.get,
164+ 'foo')
165+ self.assertEqual(byte_string, byte_stream.read())
166+ unicode_stream = self.applyDeprecated(
167+ deprecated_in((1, 5, 0)),
168+ self.lockable.get_utf8,
169+ 'foo')
170+ self.assertEqual(unicode_string,
171+ unicode_stream.read())
172+ self.assertRaises(BzrBadParameterNotString,
173+ self.applyDeprecated,
174+ deprecated_in((1, 6, 0)),
175+ self.lockable.put_utf8,
176+ 'bar',
177+ StringIO(unicode_string))
178+ self.applyDeprecated(
179+ deprecated_in((1, 6, 0)),
180+ self.lockable.put_utf8,
181+ 'bar',
182+ unicode_string)
183+ unicode_stream = self.applyDeprecated(
184+ deprecated_in((1, 5, 0)),
185+ self.lockable.get_utf8,
186+ 'bar')
187+ self.assertEqual(unicode_string,
188+ unicode_stream.read())
189+ byte_stream = self.applyDeprecated(
190+ deprecated_in((1, 5, 0)),
191+ self.lockable.get,
192+ 'bar')
193+ self.assertEqual(byte_string, byte_stream.read())
194+ self.applyDeprecated(
195+ deprecated_in((1, 6, 0)),
196+ self.lockable.put_bytes,
197+ 'raw', 'raw\xffbytes')
198+ byte_stream = self.applyDeprecated(
199+ deprecated_in((1, 5, 0)),
200+ self.lockable.get,
201+ 'raw')
202+ self.assertEqual('raw\xffbytes', byte_stream.read())
203
204 def test_locks(self):
205 self.lockable.lock_read()
206- try:
207- self.assertRaises(ReadOnlyError, self.lockable.put, 'foo',
208- StringIO('bar\u1234'))
209- finally:
210- self.lockable.unlock()
211+ self.addCleanup(self.lockable.unlock)
212+ self.assertRaises(ReadOnlyError, self.lockable.put, 'foo',
213+ StringIO('bar\u1234'))
214
215 def test_transactions(self):
216 self.assertIs(self.lockable.get_transaction().__class__,
217@@ -176,92 +172,80 @@
218
219 def test_lock_write_returns_None_refuses_token(self):
220 token = self.lockable.lock_write()
221- try:
222- if token is not None:
223- # This test does not apply, because this lockable supports
224- # tokens.
225- raise TestNotApplicable("%r uses tokens" % (self.lockable,))
226- self.assertRaises(errors.TokenLockingNotSupported,
227- self.lockable.lock_write, token='token')
228- finally:
229- self.lockable.unlock()
230+ self.addCleanup(self.lockable.unlock)
231+ if token is not None:
232+ # This test does not apply, because this lockable supports
233+ # tokens.
234+ raise TestNotApplicable("%r uses tokens" % (self.lockable,))
235+ self.assertRaises(errors.TokenLockingNotSupported,
236+ self.lockable.lock_write, token='token')
237
238 def test_lock_write_returns_token_when_given_token(self):
239 token = self.lockable.lock_write()
240- try:
241- if token is None:
242- # This test does not apply, because this lockable refuses
243- # tokens.
244- return
245- new_lockable = self.get_lockable()
246- token_from_new_lockable = new_lockable.lock_write(token=token)
247- try:
248- self.assertEqual(token, token_from_new_lockable)
249- finally:
250- new_lockable.unlock()
251- finally:
252- self.lockable.unlock()
253+ self.addCleanup(self.lockable.unlock)
254+ if token is None:
255+ # This test does not apply, because this lockable refuses
256+ # tokens.
257+ return
258+ new_lockable = self.get_lockable()
259+ token_from_new_lockable = new_lockable.lock_write(token=token)
260+ self.addCleanup(new_lockable.unlock)
261+ self.assertEqual(token, token_from_new_lockable)
262
263 def test_lock_write_raises_on_token_mismatch(self):
264 token = self.lockable.lock_write()
265- try:
266- if token is None:
267- # This test does not apply, because this lockable refuses
268- # tokens.
269- return
270- different_token = token + 'xxx'
271- # Re-using the same lockable instance with a different token will
272- # raise TokenMismatch.
273- self.assertRaises(errors.TokenMismatch,
274- self.lockable.lock_write, token=different_token)
275- # A separate instance for the same lockable will also raise
276- # TokenMismatch.
277- # This detects the case where a caller claims to have a lock (via
278- # the token) for an external resource, but doesn't (the token is
279- # different). Clients need a separate lock object to make sure the
280- # external resource is probed, whereas the existing lock object
281- # might cache.
282- new_lockable = self.get_lockable()
283- self.assertRaises(errors.TokenMismatch,
284- new_lockable.lock_write, token=different_token)
285- finally:
286- self.lockable.unlock()
287+ self.addCleanup(self.lockable.unlock)
288+ if token is None:
289+ # This test does not apply, because this lockable refuses
290+ # tokens.
291+ return
292+ different_token = token + 'xxx'
293+ # Re-using the same lockable instance with a different token will
294+ # raise TokenMismatch.
295+ self.assertRaises(errors.TokenMismatch,
296+ self.lockable.lock_write, token=different_token)
297+ # A separate instance for the same lockable will also raise
298+ # TokenMismatch.
299+ # This detects the case where a caller claims to have a lock (via
300+ # the token) for an external resource, but doesn't (the token is
301+ # different). Clients need a separate lock object to make sure the
302+ # external resource is probed, whereas the existing lock object
303+ # might cache.
304+ new_lockable = self.get_lockable()
305+ self.assertRaises(errors.TokenMismatch,
306+ new_lockable.lock_write, token=different_token)
307
308 def test_lock_write_with_matching_token(self):
309 # If the token matches, so no exception is raised by lock_write.
310 token = self.lockable.lock_write()
311- try:
312- if token is None:
313- # This test does not apply, because this lockable refuses
314- # tokens.
315- return
316- # The same instance will accept a second lock_write if the specified
317- # token matches.
318- self.lockable.lock_write(token=token)
319- self.lockable.unlock()
320- # Calling lock_write on a new instance for the same lockable will
321- # also succeed.
322- new_lockable = self.get_lockable()
323- new_lockable.lock_write(token=token)
324- new_lockable.unlock()
325- finally:
326- self.lockable.unlock()
327+ self.addCleanup(self.lockable.unlock)
328+ if token is None:
329+ # This test does not apply, because this lockable refuses
330+ # tokens.
331+ return
332+ # The same instance will accept a second lock_write if the specified
333+ # token matches.
334+ self.lockable.lock_write(token=token)
335+ self.lockable.unlock()
336+ # Calling lock_write on a new instance for the same lockable will
337+ # also succeed.
338+ new_lockable = self.get_lockable()
339+ new_lockable.lock_write(token=token)
340+ new_lockable.unlock()
341
342 def test_unlock_after_lock_write_with_token(self):
343 # If lock_write did not physically acquire the lock (because it was
344 # passed a token), then unlock should not physically release it.
345 token = self.lockable.lock_write()
346- try:
347- if token is None:
348- # This test does not apply, because this lockable refuses
349- # tokens.
350- return
351- new_lockable = self.get_lockable()
352- new_lockable.lock_write(token=token)
353- new_lockable.unlock()
354- self.assertTrue(self.lockable.get_physical_lock_status())
355- finally:
356- self.lockable.unlock()
357+ self.addCleanup(self.lockable.unlock)
358+ if token is None:
359+ # This test does not apply, because this lockable refuses
360+ # tokens.
361+ return
362+ new_lockable = self.get_lockable()
363+ new_lockable.lock_write(token=token)
364+ new_lockable.unlock()
365+ self.assertTrue(self.lockable.get_physical_lock_status())
366
367 def test_lock_write_with_token_fails_when_unlocked(self):
368 # Lock and unlock to get a superficially valid token. This mimics a
369@@ -332,6 +316,11 @@
370 # But should be relockable with a token.
371 self.lockable.lock_write(token=token)
372 self.lockable.unlock()
373+ # Cleanup: we should still be able to get the lock, but we restore the
374+ # behavior to clearing the lock when unlocking.
375+ self.lockable.lock_write(token=token)
376+ self.lockable.dont_leave_in_place()
377+ self.lockable.unlock()
378
379 def test_dont_leave_in_place(self):
380 token = self.lockable.lock_write()
381
382=== modified file 'bzrlib/tests/test_selftest.py'
383--- bzrlib/tests/test_selftest.py 2009-04-30 06:16:30 +0000
384+++ bzrlib/tests/test_selftest.py 2009-05-12 08:35:12 +0000
385@@ -622,9 +622,6 @@
386 self.assertRaises(AssertionError, self._check_safety_net)
387
388 def test_dangling_locks_cause_failures(self):
389- # This is currently only enabled during debug runs, so turn debugging
390- # on.
391- debug.debug_flags.add('lock')
392 class TestDanglingLock(TestCaseWithMemoryTransport):
393 def test_function(self):
394 t = self.get_transport('.')
395
396=== modified file 'bzrlib/tests/test_smart.py'
397--- bzrlib/tests/test_smart.py 2009-04-28 03:55:56 +0000
398+++ bzrlib/tests/test_smart.py 2009-05-12 08:35:12 +0000
399@@ -586,6 +586,8 @@
400 '')
401 self.assertEqual(SuccessfulSmartServerResponse(()), result)
402 self.assertEqual('bar', config.get_option('foo'))
403+ # Cleanup
404+ branch.unlock()
405
406 def test_value_name_section(self):
407 branch = self.make_branch('.')
408@@ -597,6 +599,8 @@
409 'gam')
410 self.assertEqual(SuccessfulSmartServerResponse(()), result)
411 self.assertEqual('bar', config.get_option('foo', 'gam'))
412+ # Cleanup
413+ branch.unlock()
414
415
416 class SetLastRevisionTestBase(TestLockedBranch):
417@@ -915,17 +919,24 @@
418 # with a new branch object.
419 new_branch = repository.bzrdir.open_branch()
420 self.assertRaises(errors.LockContention, new_branch.lock_write)
421+ # Cleanup
422+ request = smart.branch.SmartServerBranchRequestUnlock(backing)
423+ response = request.execute('', branch_nonce, repository_nonce)
424
425 def test_lock_write_on_locked_branch(self):
426 backing = self.get_transport()
427 request = smart.branch.SmartServerBranchRequestLockWrite(backing)
428 branch = self.make_branch('.')
429- branch.lock_write()
430+ branch_token = branch.lock_write()
431 branch.leave_lock_in_place()
432 branch.unlock()
433 response = request.execute('')
434 self.assertEqual(
435 SmartServerResponse(('LockContention',)), response)
436+ # Cleanup
437+ branch.lock_write(branch_token)
438+ branch.dont_leave_lock_in_place()
439+ branch.unlock()
440
441 def test_lock_write_with_tokens_on_locked_branch(self):
442 backing = self.get_transport()
443@@ -941,6 +952,13 @@
444 branch_token, repo_token)
445 self.assertEqual(
446 SmartServerResponse(('ok', branch_token, repo_token)), response)
447+ # Cleanup
448+ branch.repository.lock_write(repo_token)
449+ branch.repository.dont_leave_lock_in_place()
450+ branch.repository.unlock()
451+ branch.lock_write(branch_token)
452+ branch.dont_leave_lock_in_place()
453+ branch.unlock()
454
455 def test_lock_write_with_mismatched_tokens_on_locked_branch(self):
456 backing = self.get_transport()
457@@ -956,17 +974,29 @@
458 branch_token+'xxx', repo_token)
459 self.assertEqual(
460 SmartServerResponse(('TokenMismatch',)), response)
461+ # Cleanup
462+ branch.repository.lock_write(repo_token)
463+ branch.repository.dont_leave_lock_in_place()
464+ branch.repository.unlock()
465+ branch.lock_write(branch_token)
466+ branch.dont_leave_lock_in_place()
467+ branch.unlock()
468
469 def test_lock_write_on_locked_repo(self):
470 backing = self.get_transport()
471 request = smart.branch.SmartServerBranchRequestLockWrite(backing)
472 branch = self.make_branch('.', format='knit')
473- branch.repository.lock_write()
474- branch.repository.leave_lock_in_place()
475- branch.repository.unlock()
476+ repo = branch.repository
477+ repo_token = repo.lock_write()
478+ repo.leave_lock_in_place()
479+ repo.unlock()
480 response = request.execute('')
481 self.assertEqual(
482 SmartServerResponse(('LockContention',)), response)
483+ # Cleanup
484+ repo.lock_write(repo_token)
485+ repo.dont_leave_lock_in_place()
486+ repo.unlock()
487
488 def test_lock_write_on_readonly_transport(self):
489 backing = self.get_readonly_transport()
490@@ -1031,6 +1061,10 @@
491 '', 'branch token', repo_token)
492 self.assertEqual(
493 SmartServerResponse(('TokenMismatch',)), response)
494+ # Cleanup
495+ branch.repository.lock_write(repo_token)
496+ branch.repository.dont_leave_lock_in_place()
497+ branch.repository.unlock()
498
499
500 class TestSmartServerRepositoryRequest(tests.TestCaseWithMemoryTransport):
501@@ -1282,17 +1316,24 @@
502 # object.
503 new_repo = repository.bzrdir.open_repository()
504 self.assertRaises(errors.LockContention, new_repo.lock_write)
505+ # Cleanup
506+ request = smart.repository.SmartServerRepositoryUnlock(backing)
507+ response = request.execute('', nonce)
508
509 def test_lock_write_on_locked_repo(self):
510 backing = self.get_transport()
511 request = smart.repository.SmartServerRepositoryLockWrite(backing)
512 repository = self.make_repository('.', format='knit')
513- repository.lock_write()
514+ repo_token = repository.lock_write()
515 repository.leave_lock_in_place()
516 repository.unlock()
517 response = request.execute('')
518 self.assertEqual(
519 SmartServerResponse(('LockContention',)), response)
520+ # Cleanup
521+ repository.lock_write(repo_token)
522+ repository.dont_leave_lock_in_place()
523+ repository.unlock()
524
525 def test_lock_write_on_readonly_transport(self):
526 backing = self.get_readonly_transport()