[MERGE] "Branch.open()" should re-use the connection for stacked branches

[John Arbash Meinel]

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

Vincent Ladeuil wrote:
>>>>>> "jam" == John Arbash Meinel <john at arbash-meinel.com> writes:
> 
> <snip/>
>     jam> === modified file 'bzrlib/tests/branch_implementations/test_stacking.py'
>     jam> --- bzrlib/tests/branch_implementations/test_stacking.py	2008-12-01 23:50:52 +0000
>     jam> +++ bzrlib/tests/branch_implementations/test_stacking.py	2009-01-08 19:15:20 +0000
>     jam> @@ -23,11 +23,17 @@
> 
> <snip/>
>  
>  
>     jam> +old_format_errors = (
>     jam> +    errors.UnstackableBranchFormat,
>     jam> +    errors.UnstackableRepositoryFormat,
>     jam> +    )
>     jam> +
>     jam> +
> 
> Hmm, 'old' and 'new' tends to bit-rot quite quickly, how about
> unstackable_format_errors ? I know you just factored that out
> but...
>  

well, 'new' tends to bit-rot, but 'old' is always old, just sometimes
gets *even older* :).

I'm fine with "unstackable_format_errors", it is just a search and
replace away.

I'll do that on the follow up patch, though.

> If I remember correctly, raising TestNotApplicable during setUp
> is bad because tearDown is not called ? It may not matter that
> much here but I think it's better to avoid that idiom.
> 
> Defining a helper should solve that problem but forces you to
> call it in every test.

I've used the idiom before, and didn't think it was a problem.

Certainly I see this:
    def _addSkipped(self, test, skip_excinfo):
....
        try:
            test.tearDown()
        except KeyboardInterrupt:
            raise

So if we get a "skipped" test, we explicitly call tearDown. Looking at
the rest of the code, it seems that unittest.TestCase.run() does:

try:
    self.setUp()
except KeyboardInterrupt:
    raise
except:
    result.addError(self, self._exc_info())
    return

And then in bzrlib.tests.ExtendedTestResult we override addError with:

if isinstance(err[1], TestSkipped):
    return self._addSkipped(test, err)

And in self._addSkipped() we call tearDown() as I mentioned.

So I'm pretty sure it is allowed. I would be fine to hear other opinions
if we should really not be using TestSkipped/TestNotApplicable during
setUp().

John
=:->


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

iEYEARECAAYFAklmarsACgkQJdeBCYSNAANmhwCfSjzJHWfSvnbZLnb8xayS3WOp
s34An39wPXiyeXR0g9T2qMvqeiwo/f8i
=Bwa8
-----END PGP SIGNATURE-----