[MERGE] fixes for selftest on win32 (part 2)

[Alexander Belchenko]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

John Arbash Meinel пишет:
> Alexander Belchenko wrote:
>> Next portion of fixes for selftest on win32:
> 
>>  2332 Alexander Belchenko       2007-03-11
>>       test_sftp_transport: fix 1 test, skip 1 test; now all tests pass on win32

I apologize for this comment. 'all tests pass' related to test_sftp_transport.py

> === modified file bzrlib/errors.py
> --- bzrlib/errors.py
> +++ bzrlib/errors.py
> @@ -647,7 +647,7 @@
> 
>  class LockError(BzrError):
> 
> -    _fmt = "Lock error: %(message)s"
> +    _fmt = "Lock error: %(msg)s"
> 
>      internal_error = True
> 
> @@ -657,7 +657,7 @@
>      #
>      # New code should prefer to raise specific subclasses
>      def __init__(self, message):
> -        self.message = message
> +        self.msg = message
> 
> 
> ^- Was this specifically needed? You seem to just be changing the
> variable, without actually changing the effect. But then again, children
> might be doing something different.

Yes, this needed for Python 2.5.
It works OK on Python 2.4, but not on Python 2.5.
In Python 2.5 basic class of all our errors (StandardError)
has attribute .message, that don't reflected in __dict__.
I'm not sure why, but you can test it with simple code:

class BzrError(StandardError):
    def __init__(self, message):
        self.message = message

err = BzrError('spam')
print err.message
print dir(err)
print err.__dict__
print dir(StandardError)

Here the output on my Windows XP + Python2.5:

spam
['__class__', '__delattr__', '__dict__', '__doc__', '__getattribute__', '__getitem__', '__hash__',
'__init__', '__module__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__',
'__setstate__', '__str__', '__weakref__', 'args', 'message']
{}
['__class__', '__delattr__', '__dict__', '__doc__', '__getattribute__', '__getitem__', '__hash__',
'__init__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__setstate__',
'__str__', 'args', 'message']

As you can see __dict__ is empty and therefore make LockError unprintable:

Unprintable exception LockError: dict={}, fmt='Lock error: %(message)s', error='message'



> ...
> 
> v- This should be in a try/finally
> f = file(path, 'r')
> try:
>   s = f.read()
> finally:
>   f.close()
> self.assertEqualDiff(content, s)

OK.

> v- Rather than directly importing 'TestSkipped' you can use
> 'tests.TestSkipped', since 'tests' is already imported.

Simple TestSkipped is more readable and clear for me.
But if you wish...

> # I'm okay with skipping them for now. I think the idea was that we
> would hopefully find a way to keep symlinks in the inventory (similar to
> how we currently handle the executable bit).

For me the implementation way is clear.
Main question is 'when?', rather than 'how?'.

> v- # bzr doesn't support fake symlinks on windows, yet.

Thank you.

> -        # be represented on windows.
> +        # bzr yet don't have support for fake symlinks on windows
> +        if not has_symlinks():
> +            raise TestSkipped("No symlink support")
>          os.symlink('target', 'a link')
>          stat = os.lstat('a link')
>          expected_entries = [


>      def test__remote_path(self):
> 
> v- I don't think this test should be skipped. We can use:
> 
> if sys.platform == 'win32':
>     # On win32 an initial '/' is inserted before the drive letter.
>     test_dir = '/' + self.test_dir
> else:
>     test_dir = self.test_dir
> ...

This is ugly. We try to test internal method t._remote_path() that implemented
as unix-only. Ideally we should mockup the base URL of transport,
but I don't have enough courage to write new mock server only for 1 test.

When I fix bzr+ssh I use this approach to create unix-like absolute URL:

unix_like = os.path.splitdrive(win_abs_path)[1]

in this case 'C:/some/path' becomes '/some/path' and still usable as absolute URL
(within the same local drive). But changing testing server to this URL
leads me to pass this 1 tests and fails 3 another tests.

> 
> +        if sys.platform == 'win32':
> +            raise TestSkipped('This test require unix-like absolute path')
>          t = self.get_transport()
>          # try what is currently used:
>          # remote path = self._abspath(relpath)
> 
> === modified file bzrlib/transport/sftp.py
> --- bzrlib/transport/sftp.py
> +++ bzrlib/transport/sftp.py
> @@ -1106,7 +1106,10 @@
> 
>      def get_url(self):
>          """See bzrlib.transport.Server.get_url."""
> -        return self._get_sftp_url(urlutils.escape(self._homedir[1:]))
> +        if sys.platform == 'win32':
> +            return self._get_sftp_url(urlutils.escape(self._homedir))
> +        else:
> +            return self._get_sftp_url(urlutils.escape(self._homedir[1:]))
> 
> ^- I think this one would be clearer as
> 
> homedir = self._homedir
> if sys.platform != 'win32':
>     # Remove the initial '/' on all platforms but win32
>     homedir = homedir[1:]
> return self._get_sftp_url(urlutils.escape(homedir))

Actually, this code is copy pasted from another similar SFTPServer in this module.
I'll change in both places.

> Otherwise +1 from me.

Thank you for review.

[µ]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFF9d8XzYr338mxwCURAuZ0AJ9VMHg1u/CPRl4hPSQ4TW7gok49QQCfXo+q
Z2pXnrWnId++8A1HEgUyw5s=
=Rq1V
-----END PGP SIGNATURE-----