[merge] win32- more test suite fixes

[Aaron Bentley]

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

John Arbash Meinel wrote:
> Aaron Bentley wrote:
> I suppose we could wrap the 'fancy_rename()' in a try/catch and do:
> 
> try:
>   fancy_rename(...)
> except OSError, e:
>   if e.errno == errno.EPERM:
>     os.lstat(old)
>   raise
> 
> That should translate an EPERM into a ENOENT when appropriate, without
> the extra overhead.

That's what I had in mind, but if you'd rather do it the simpler way, okay.

>>>What bugs me is that if we enter the finally block due to an exception,
>>>and then self.tt.finalize throws, we'll never see the original
>>>exception.  I'm fine with doing it in this case, but we should find a
>>>more general solution.

> Well, I can restore what you originally had.

No, your change is definitely an improvement.

> Or I could just restore it to the old code. I removed it to track down a
> different bug, which is now fixed, so I don't *need* to see the tt
> exceptions.

No, as I said, I'm fine with doing it in this case.  Generally, though,
I think we should look at having an Exception class that bundles
multiple exceptions together.

>>>I don't understand what the following changes are all about:
>>>
>>>@@ -67,6 +71,7 @@
>>>         # J changes: 'b-file-id-2006-01-01-defg'
>>>         # K changes: 'c-funky<file-id> quiji%bo'
>>>
>>>+        self.branch = None
>>>         main_wt = self.make_branch_and_tree('main')
>>>         main_branch = main_wt.branch
>>>         self.build_tree(["main/a","main/b","main/c"])
> 
> 
> ^- this is done to handle passing/skipping the test.
> 
> 
>>>set(self.branch.repository.get_ancestry(new)).difference(set(self.branch.repository.get_ancestry(old)))
>>>
>>>-        self.assertEqual(
>>>+        self.assertDictsEqual(
>>>             {'b-file-id-2006-01-01-defg':set(['rev-J']),
>>>              'c-funky<file-id> quiji%bo':set(['rev-K'])
>>>              },
> 
> 
> The old code was calling assertEqual() with two dictionaries. I thought
> that wasn't allowed, since order isn't preserved, but I just realizeh it
> probably only isn't allowed if you are just looking at the keys.
> I can revert this.

It just didn't seem to be explained anywhere, and there was a lot of it.
I wasn't sure whether it meant that dictionaries behaved differently
under Windows.  It seems positive in general, so keep it.

>>>Shouldn't this be a TestSkipped?
> 
> 
> There were other comments in the code that said:
> # this is not a skip because newer formats do support this,
> # and nothin can done to correct this test - its not a bug.
> 
> I know Robert has the feeling that TestSkipped is meant for things that
> can be fixed and run eventually. Not just for things that are known
> broken and we aren't fixing (testing old formats)

My feeling is that a test shouldn't pass unless it was run to completion
successfully.  E.g. a test that cannot be performed on Windows may still
be a failing test, and should be run on another platform to ensure that
it passes.

I can think of three alternatives to pass/fail.
1. expected failure
2. cannot be performed on this platform
3. requirements not satisfied

1 can be considered a 'pass', in a sense.  It doesn't do what we want,
but it does what we're expecting.  At least it didn't fail for a
different reason!

2 means 'please rerun me on another platform'

3 means 'please install (paramiko|pycurl|...)'

> If self.branch is None, that means we couldn't setup the test. Primarily
> because we are on Win32, and one of the files is using <> in the
> file_id, which we didn't escape before knits.

Oh, right.

> This is known broken, and we aren't planning to fix it, we have newer
> stuff that does it correctly. If we are okay with TestSkipped, I can
> raise that inside setUp() and make my life easier. In setUp() I already
> know I need to exit, but calling return doesn't do anything, you have to
> 'return' from the actual test function.

>>>How about this:
>>>
>>>    f = open(self._limbo_name(trans_id), 'wb')
>>>    try:
>>>        try:
>>>            unique_add(self._new_contents, trans_id, 'file')
>>>            for segment in contents:
>>>                f.write(segment)
>>>        finally:
>>>            f.close()
>>>    except:
>>>        os.unlink(self._limbo_name(trans_id))
>>>        raise
>>>
>>>        f.close()

> Your final 'f.close()' will never get called. You raise before you reach
> it. Though I think it was just a typo.

It was.

>  Also, with your proposal, we will
> double-cleanup in the case that segment in contents raises.

Drat.  Yeah, you got me.

> I think I prefer this, as double closing a file is never an error, and
> it does the right thing:

> But what about turning it around, and not creating the file if we can't
> add a new contents:

> I realize this opens up the other problem, where we now have a
> _new_contents entry, but no file.

Right, that's why I didn't want to do it that way.

> So I would probably go with the first
> one, it is the most correct, and the ugliest :(

TreeTransform tends that way anyhow.  Ugly, but right.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFEpWAG0F+nu1YWqI0RAqwvAJsGzcjcjKvG8aKk6ML7h9xYZaj9twCfQBIS
FyRT50rbp5SkuW7R8dvHfTE=
=XLhp
-----END PGP SIGNATURE-----