[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.

Tue Feb 17 11:37:12 GMT 2009

>>>>> "robert" == Robert Collins <robertc at robertcollins.net> writes:

    robert> On Mon, 2009-02-16 at 14:29 +0100, Vincent Ladeuil wrote:
    >> Nice refactoring job in the remote tests, 1 test still failing:

    robert> Meta-comment: please delete stuff irrelevant to the
    robert> review; its just noise to wade through. Keeping hunk
    robert> headers and file names is useful to find lines in the
    robert> file.

I tried, but I'll try harder.

    >> FAIL: branch_implementations.test_branch.TestTestCaseWithBranch.test_branch_format_matches_bzrdir_branch_format(RemoteBranchFormat-v2)
    >> <class 'bzrlib.remote.RemoteBranchFormat'> is not <class 'bzrlib.branch.BzrBranchFormat6'>.

    robert> This is very odd, if you have one test failing, because there are two
    robert> versions of this test run. 
    robert> $ ./bzr selftest test_branch_format_matches_bzrdir_branch_format
    robert> testing: format-on-push/bzr
    robert>    format-on-push/bzrlib (1.13dev python2.5.2)

    robert> [9/10 in 0s]
    robert> branch_implementations.test_branch.TestTestCaseWithBranch.test_branch_format_matches_bzrdir_branch_format(RemoteBranchFormat-default)                                                                 
    robert> ----------------------------------------------------------------------
    robert> Ran 10 tests in 0.256s

    robert> OK
    robert> tests passed

    robert> ^ worked for me.

./bzr selftest test_branch_format_matches_bzrdir_branch_format
./bzr selftest -s branch_implementations.test_branch.TestTestCaseWithBranch.test_branch_format_matches_bzrdir_branch_format

Both works for me.

./bzr selftest

fails and outputs:

FAIL: test_branch_format_matches_bzrdir_branch_format (bzrlib.tests.branch_implementations.test_branch.TestTestCaseWithBranch)

vvvv[log from bzrlib.tests.branch_implementations.test_branch.TestTestCaseWithBranch.test_branch_format_matches_bzrdir_branch_format(RemoteBranchFormat-v2)]
317.880  opening working tree '/tmp/testbzr-61uxAi.tmp'

^^^^[log from bzrlib.tests.branch_implementations.test_branch.TestTestCaseWithBranch.test_branch_format_matches_bzrdir_branch_format(RemoteBranchFormat-v2)]
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/net/bigmamac/Volumes/home/vila/src/bzr/reviews/format-on-push/bzrlib/tests/branch_implementations/test_branch.py", line 59, in test_branch_format_matches_bzrdir_branch_format
    bzrdir_branch_format.__class__)
AssertionError: <class 'bzrlib.remote.RemoteBranchFormat'> is not <class 'bzrlib.branch.BzrBranchFormat6'>.

Does it succeed for you ?

<snip/>

    robert> As you say moved code. I haven't even read the body
    robert> of the function, I think its totally out of scope for
    robert> this already over-large branch.

Hmmm. I lost time reviewing it then :-/

    robert> Its transient anyway, we'll get back to this and fix
    robert> the code to init over the wire, but that can wait.

I was unsure about that, thanks for clarifying.

    >> For coherency it would be better to keep the same
    >> parameter names than BzrDir, i.e. _transport and _format
    >> with leading underscores.

    robert> There is a defined factory interface for getting
    robert> control dir objects, I don't think there is any need
    robert> for consistency

Well, consistency and coherency both help the reader more than
the writer, while reviewing, I first didn't find the BzrDir
__init__ (I don't know it by heart yet) because, unlike the most
used idiom in the code base, it's in the middle of the class and
not at the start, so I wrongly assumed there was *no*
__init__. If the same parameter names have been used, a grep
would have found it for me.

    robert> Well, it doesn't quite fit at the moment. I'd suggest
    robert> revisiting it on further changes.

Are you implying that *you* will revisit or that someone else
will ? The later one doesn't sound very convincing, but hey, my
vote was a tweak anyway so in the end, you're the judge.

    robert> def test_branch_is_locked(self):
    robert> branch = self.make_branch('source')
    robert> @@ -215,8 +253,12 @@
    robert> self.assertIsNot(hook_calls_1, hook_calls_2)
    robert> branch.set_last_revision_info(0, NULL_REVISION)
    robert> # Both hooks are called.
    robert> -        self.assertEqual(len(hook_calls_1), 1)
    robert> -        self.assertEqual(len(hook_calls_2), 1)
    robert> +        if isinstance(branch, RemoteBranch):
    robert> +            count = 2
    robert> +        else:
    robert> +            count = 1
    robert> +        self.assertEqual(len(hook_calls_1), count)
    robert> +        self.assertEqual(len(hook_calls_2), count)
    >> 
    >> You swapped expected and actual. 

    robert> Actually I preserved the order used in the code I was
    robert> modifying :). This is trivial to fix if we end up
    robert> altering the patch.

I tend to fix that sort of things when I encounter them, just to
improve coherency :)

    >> And this seems to be duplicated and can possibly be
    >> factored out in ChangeBranchTipTestCase ?

    robert> The name of the test gives its intent - there is one
    robert> for Pre and one for Post, it seems unredundant to me.

I wasn't referring to the test name, only to the assertions.

    >> There is already a assertHookCalls but it seems to have a
    >> different intent or precision so I'm not sure if it's usable
    >> here (explanation welcome).

    robert> No idea: am fixing things that were mechanically
    robert> broken not checking the meaning of each thing: the
    robert> pattern of breakage was 'hooks are not invoked by
    robert> RemoteBranch consistently nor tested to behave'.

That's surprising, from outside, you seemed to enhance the
readability of the tests by making their intent clearer.

    robert> === modified file 'bzrlib/tests/branch_implementations/test_sprout.py'
    robert> --- bzrlib/tests/branch_implementations/test_sprout.py	2008-12-01 23:50:52 +0000
    robert> +++ bzrlib/tests/branch_implementations/test_sprout.py	2009-02-13 00:52:18 +0000

<snip/>

    >> This is were you raised the bar prompting most of my comment
    >> requests :-) I mean, *this* is clear, the rest has no excuse for
    >> being less clear :)

    robert> This could be a disincentive to make anything clearer
    robert> :).

Depends on the reviewed I think :)

    robert> +        if isinstance(target, remote.RemoteBranch):
    robert> +            # we have to look at the real branch to see whether RemoteBranch
    robert> +            # did the right thing.
    robert> +            target._ensure_real()
    robert> +            target = target._real_branch
    >> 
    >> This comment is good :) No double reading necessary
    >> here. Can you adapt it for repo below. Please ?

What I mean here is that the comment explain why the isinstance()
call is needed and how it modifies the test.

    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.

    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.

    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.

    robert> self._expected_calls = None
    robert> _SmartClient.__init__(self, FakeMedium(self._calls, fake_medium_base))
    >> 
    robert> @@ -194,6 +163,8 @@
    >> 
    robert> def add_success_response_with_body(self, body, *args):
    robert> self.responses.append(('success', args, body))
    robert> +        if self._expected_calls is not None:
    robert> +            self._expected_calls.append(None)
    >> 
    >> Like here ?

    robert> Thats the implementation of the change I documented
    robert> before, not a simplification :).

And that's why I said 'like here' as in 'this is used to simplify
trivial call/response handling' :)

    robert> +        # XXX: Multiple calls are bad, this second call documents what is
    robert> +        # today.
    >> 
    >> s/is/is done/ ? s/today/& and should be fixed/ ?

    robert> s/is/happens/ if you like, though I was intending to use 'is', as the
    robert> present tense of etre.

Haaa, that 'is' :)

    robert> Thanks for the review. I'd like to know what the
    robert> "corresponding" tests are - that wasn't clear. I'll
    robert> switch the order of those test parameters you called
    robert> out around when you point me at the right place. I'll
    robert> also note that doing a really long comment and
    robert> copying it all over smells like bad duplication to me
    robert> - so I definitely don't want to do that.

Gee I never ask about copying a really long comment, just the
one-line one :) 

i.e. look at the real XXX to see whether the RemoteXXX did the right thing