[merge] win32- all tests pass but 1

John Arbash Meinel john at arbash-meinel.com
Sat Jul 1 03:12:55 BST 2006


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

Aaron Bentley wrote:
> John Arbash Meinel wrote:
>>> The attached patch is the rest of the cleanups that get win32 so that
>>> almost all tests are passing.
> 
> 
>      def __init__(self, transport, filename, mode, create=False,
> file_mode=None):
>          _KnitComponentFile.__init__(self, transport, filename, mode)
> -        self._file = None
>          self._checked = False
>          if create:
>              self._transport.put(self._filename, StringIO(''),
> mode=file_mode)
> @@ -1286,12 +1292,11 @@
>          pass
> 
>      def _open_file(self):
> -        if self._file is None:
> -            try:
> -                self._file = self._transport.get(self._filename)
> -            except NoSuchFile:
> -                pass
> -        return self._file
> +        try:
> +            return self._transport.get(self._filename)
> +        except NoSuchFile:
> +            pass
> +        return None
> 
> 
> This is so that you can require callers to do close()?
> 

Right. _KnitData was saving a reference to the file, but never
referenced it again. And there was only one place that was calling it
(copy_to).

And we were getting EPERM in all of the reconcile operations, because
they expect to be able to replace the inventory and revision knits.

It may not even be required for the caller to close() the file handle,
as long as _KnitData doesn't hold it open.

And the code was buggy anyway, because it called transport.put(path,
_data._open_file(), but it didn't seek(0) on the file handle, so after
the first call, any subsequent calls would get an empty file.

> 
> @@ -134,6 +136,11 @@
>          self.branch = main_branch
> 
>      def test_fileids_altered_between_two_revs(self):
> +        if self.branch is None:
> +            # Could not create the branching structure
> +            # for this repository format
> +            return
> 
> Still don't agree with this.  If Martin or Rob say it's okay, then fine,
> but I think we shouldn't pass unless we run to completion successfully.
> 

These are the 'KnownFailures'. I would be okay with creating a new
exception class, like TestSkippe, maybe TestKnownFailed, or something
like that.
I return here only because a plain return in setUp doesn't do anything.

> @@ -20,8 +20,8 @@
> 
> 
> -class MergeTest(TestCase):
> +class MergeTest(TestCaseWithTransport):
> 
>      def test_change_name(self):
>          """Test renames"""
> -        builder = MergeBuilder()
> +        builder = MergeBuilder(getcwd())
> 
> Are these changes for win32 compatibility or just general neatness?  (By
> the way, these test cases are weird because they came in with the
> original merge implementation from BaZing.)
> 

General neatness. There was a bug in the Diff3Merger, which made me
check out this file. And while I was here, I saw that it was creating
lots of temporary directories in TEMP. (And since the test wasn't
passing, it was leaving them there).

I don't know if you ever look, but you'll find quite a bit of cruft in
/tmp if you run the test suite a lot. (Maybe only with failures, but I
frequently have 50+ /tmp/bzr-* /tmp/merge-* /tmp/testbzr-* files and dirs).


> 
> === modified file
> 'bzrlib/tests/workingtree_implementations/test_workingtree.py'
> --- bzrlib/tests/workingtree_implementations/test_workingtree.py
> 2006-06-27 08:47:32 +0000
> +++ bzrlib/tests/workingtree_implementations/test_workingtree.py
> 2006-06-30 16:56:52 +0000
> @@ -17,6 +17,7 @@
> 
>  from cStringIO import StringIO
>  import os
> +import sys
> 
>  import bzrlib
>  from bzrlib import branch, bzrdir, errors, urlutils, workingtree
> @@ -30,6 +31,7 @@
>                                  WorkingTree)
>  from bzrlib.conflicts import ConflictList
> 
> +
>  class TestWorkingTree(TestCaseWithWorkingTree):
> 
>      def test_list_files(self):
> @@ -80,7 +82,10 @@
>          wt, relpath = WorkingTree.open_containing('./foo')
>          self.assertEqual('foo', relpath)
>          self.assertEqual(wt.basedir + '/',
> urlutils.local_path_from_url(branch.base))
> -        wt, relpath = WorkingTree.open_containing('file://' + getcwd()
> + '/foo')
> +        if sys.platform == 'win32':
> +            wt, relpath = WorkingTree.open_containing('file:///' +
> getcwd() + '/foo')
> 
> This code looks like it could fail because it's joining a url and a
> local filesystem path.  Am I wrong about that?

It could fail if the working tree was a non ascii path.
I could easily call local_path_to_url() instead, but I was thinking we
were actually asserting that you can use the cwd() in a simple file://
url. I'd be happy to switch to a real url if you prefer.

> 
> So I'm +0.9 on this right now.  The TestSkipped thing needs to be
> resolved one way or another, and I'd also like to know about this
> 'file://' + getcwd thing.
> 
> Aaron

I'd be okay either way with the TestSkipped stuff. I was just trying to
avoid Skip when we really mean
NotGoingToRunBecauseItDoesntWorkAndItIsOldAndUnsupported.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFEpdonJdeBCYSNAAMRAnoNAKC9gbiZdtlZ3foQxK9HR+Usl5cUGQCghe2U
cOjGnmI+/+8/BQtrPsIZXsA=
=b4nC
-----END PGP SIGNATURE-----




More information about the bazaar mailing list