[MERGE] Fix RemoteBranch to be used correctly in tests using bzr+ssh, to fire off Branch hooks correctly, and improve the branch_implementations tests to check that making a branch gets the right format under test.

[Robert Collins]

On Tue, 2009-02-17 at 12:37 +0100, Vincent Ladeuil wrote:
>    robert> @@ -558,6 +558,9 @@
>     robert> target_repo = target_bzrdir.open_repository()
>     robert> source_branch = bzrlib.branch.Branch.open(
>     robert> self.get_vfs_only_url('source'))
>     robert> +        if isinstance(target_repo, RemoteRepository):
>     robert> +            target_repo._ensure_real()
>     robert> +            target_repo = target_repo._real_repository
>     >> 
>     >> Adapting the comment from the "corresponding" branch test will
> be good.
> 
>     robert> For whatever reason, this test was trivial to
>     robert> understand.  In particular, its not testing quite the
>     robert> same thing, which is probably why it wasn't
>     robert> confusing.
> 
> Adding the comment here will help the reader of *that* test
> without the need to resort to annotate to find the sibling one
> above.

I still don't know what other test you are pointing at.

>     robert> === modified file
> 'bzrlib/tests/per_repository/test_repository.py'
>     robert> ---
> bzrlib/tests/per_repository/test_repository.py  2008-12-17 08:40:10
> +0000
>     robert> +++
> bzrlib/tests/per_repository/test_repository.py  2009-02-13 00:52:18
> +0000
>     robert> @@ -813,8 +813,12 @@
>     robert> if isinstance(repo, remote.RemoteRepository):
>     robert> repo._ensure_real()
>     robert> repo = repo._real_repository
>     robert> +        target_repo = target.open_repository()
>     robert> +        if isinstance(target_repo,
> remote.RemoteRepository):
>     robert> +            target_repo._ensure_real()
>     robert> +            target_repo = target_repo._real_repository
>     >> 
>     >> Adapting the comment from the "corresponding" branch test will
> be good.
> 
>     robert> Which test is that?
> 
> The ones above for the same reason: explaining the intent and the
> effect of the isinstance() call to readers unfamiliar with the
> implementation.

We have a per_repository test under discussion. You say some other tests
need a comment copied to it, but you're not saying which ones :(. 


>     robert> === modified file 'bzrlib/tests/test_remote.py'
>     robert> --- bzrlib/tests/test_remote.py     2009-01-27 07:48:55
> +0000
>     robert> +++ bzrlib/tests/test_remote.py     2009-02-13 00:52:18
> +0000
>     >> 
>     >> <snip/>
>     >> 
>     robert> @@ -178,7 +146,8 @@
>     robert> self.expecting_body = False
>     robert> # if non-None, this is the list of expected calls, with
> only the
>     robert> # method name and arguments included.  the body might be
> hard to
>     robert> -        # compute so is not included
>     robert> +        # compute so is not included. If a call is None,
> that call can
>     robert> +        # be anything.
>     >> 
>     >> s/&/This used to simplify trivial call/responses handling/ ?
> 
>     robert> I don't know if that is the intent or not - one of
>     robert> the original authors might.
> 
> Err, I'm talking about the 'call is None' part. It sounds a bit
> surprising to record calls and their parameters and sometimes
> just don't care about the call itself but still recording
> it. Hence the proposal to update the comment.

Well I don't care about the call at all - in the hpss behaviours under
test we're testing that a particular event 'foo' occurs, but there are
preludes and suffixes to 'foo' whose contents are irrelevant - because
we're testing 'foo' by calling high level methods, not by invoking 'foo'
directly. I added the 'call is None' support not to make handling
trivial call or responses easier, but to make handling complex ones
easier, which I think is why your review comment confused me :).

I'll add an explanation.

Thanks,
Rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090218/0f8c57a0/attachment.pgp