[MERGE] convert locking problems in selftest at win32 into known errors

[Robert Collins]

bb:comment

I'm not quite ready to vote, would like to discuss a little so I
understand better.

On Tue, 2007-10-30 at 12:27 +0200, Alexander Belchenko wrote:
> I'm sick to see locking errors in selftest output again and agin.
> Here the patch, that adds 2 new methods: expectError (to convert known
> [locking] errors to knownFailure) and get_xbit (to read executable bit
> in the right way depending on underlying platform).

 
> +    def expectError(self, reason, excClass, callableObj, *args, **kwargs):
> +        """Invoke test, expecting it to raise error for the given reason.
> +
> +        See docstring for expectFailure and assertRaises for details.
> +
> +        :param  reason: expected error reason.
> +        :param excClass: As for the except statement, this may be either an
> +            exception class, or a tuple of classes.
> +        :param callableObj: A callable, will be passed ``*args`` and
> +            ``**kwargs``.
> +        """
> +        try:
> +            self.assertRaises(excClass, callableObj, *args, **kwargs)
> +        except self.failureException:
> +            self.fail('Unexpected success.  Should raise error: %s' % reason)
> +        else:
> +            raise KnownFailure(reason)

This looks nice. ^^

You might even want a expectLockContentionWin32 helper to factor out the
common code more.

Looking at it though, I think a variation might be more useful - a
maybeExpectError(reason, excClass, error_expected, callableObj, *args,
**kwargs):
...
:param error_expected: If False and no exception was raised, return the
result of callableObj, otherwise raise an exception..

or something like that. I say this because the pattern of
if is_win_32(): errorExpected(... callable)
callable()

further down in the code is very duplicative and its possible for that
to introduce skew between the expected-error calls and the actual calls.



> @@ -2120,6 +2138,22 @@
>          else:
>              self.assertIs(tree.path2id(path), None, path+' in working tree.')
>  
> +    def _get_xbit_posix(self, tree, file_id):
> +        return tree.is_executable(file_id)
> +
> +    def _get_xbit_win32(self, tree, file_id):
> +        tree.lock_read()
> +        try:
> +            xbit = tree.is_executable(file_id)
> +        finally:
> +            tree.unlock()
> +        return xbit
> +
> +    if sys.platform != 'win32':
> +        get_xbit = _get_xbit_posix
> +    else:
> +        get_xbit = _get_xbit_win32
> +

_get_xbit_win32 looks like it will work on posix too - so why not a
single defintion ?


The following bundle changes I don't understand. Why is bundle running
into locking errors? I realise you just want to make them expected
errors, and with the duplication removed I'll be happy for that - but
I'd really like to know that there is a bug containing whatever analysis
you have done on this already (e.g. does this affect users or just the
test suite?)

> === modified file 'bzrlib/tests/test_bundle.py'
> --- bzrlib/tests/test_bundle.py	2007-09-24 08:46:35 +0000
> +++ bzrlib/tests/test_bundle.py	2007-10-29 12:24:19 +0000
> @@ -349,6 +349,8 @@
 


> === modified file 'bzrlib/tests/test_merge.py'
> --- bzrlib/tests/test_merge.py	2007-09-14 09:51:38 +0000
> +++ bzrlib/tests/test_merge.py	2007-10-29 13:21:49 +0000
> @@ -16,6 +16,7 @@
>  
>  import os
>  from StringIO import StringIO
> +import sys
>  
>  from bzrlib import (
>      conflicts,
> @@ -138,7 +139,13 @@
>          tree_b = dir_b.open_workingtree()
>          tree_a.commit(message="hello again")
>          log = StringIO()
> -        merge_inner(tree_b.branch, tree_a, tree_b.basis_tree(), 
> +        if sys.platform == 'win32':
> +            self.expectError('on win32 OS locks are exclusive within process',
> +                errors.LockContention,
> +                merge_inner,
> +                tree_b.branch, tree_a, tree_b.basis_tree(),
> +                this_tree=tree_b, ignore_zero=True)
> +        merge_inner(tree_b.branch, tree_a, tree_b.basis_tree(),
>                      this_tree=tree_b, ignore_zero=True)

And the same for the above

>          log = self._get_log(keep_log_file=True)
>          self.failUnless('All changes applied successfully.\n' not in log)


...

In fact looking at all the following expected error changes it really
looks strange, because even on linux we can't open the dirstate twice
safely.

Is there a bug open about this (I'm sure there probably is :)).

-Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20071113/1c7aabf0/attachment.pgp