[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
robertc at robertcollins.net
Mon Feb 16 20:55:13 GMT 2009
On Mon, 2009-02-16 at 14:29 +0100, Vincent Ladeuil wrote:
> Nice refactoring job in the remote tests, 1 test still failing:
Meta-comment: please delete stuff irrelevant to the review; its just
noise to wade through. Keeping hunk headers and file names is useful to
find lines in the file.
> 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'>.
This is very odd, if you have one test failing, because there are two
versions of this test run.
$ ./bzr selftest test_branch_format_matches_bzrdir_branch_format
testing: format-on-push/bzr
format-on-push/bzrlib (1.13dev python2.5.2)
[9/10 in 0s]
branch_implementations.test_branch.TestTestCaseWithBranch.test_branch_format_matches_bzrdir_branch_format(RemoteBranchFormat-default)
----------------------------------------------------------------------
Ran 10 tests in 0.256s
OK
tests passed
^ worked for me.
> Some very nice and clear comments, which prompted some requests for more :-)
>
> BB:tweak
> robert> === modified file 'bzrlib/branch.py'
> robert> --- bzrlib/branch.py 2009-02-02 05:47:08 +0000
> robert> +++ bzrlib/branch.py 2009-02-13 00:52:18 +0000
> robert> @@ -174,6 +174,33 @@
> robert> def is_locked(self):
> robert> raise NotImplementedError(self.is_locked)
>
> robert> + def _lefthand_history(self, revision_id, last_rev=None,
> robert> + other_branch=None):
>
> A doc string will help. I realize it's moved code, but yet if you
> gain some knowledge playing with it, it's a good time to clean it
> up.
>
> robert> +
> robert> + if 'evil' in debug.debug_flags:
> robert> + mutter_callsite(4, "_lefthand_history scales
> robert> + with history.")
> robert> + # stop_revision must be a descendant of last_revision
>
> Needs update, there is no stop_revision nor last_revision variables.
>
> robert> + graph = self.repository.get_graph()
> robert> + if last_rev is not None:
> robert> + if not graph.is_ancestor(last_rev, revision_id):
> robert> + # our previous tip is not merged into stop_revision
>
> Ditto.
As you say moved code. I haven't even read the body of the function, I
think its totally out of scope for this already over-large branch.
> <snip/>
>
> robert> @@ -1736,7 +1737,12 @@
> robert> mode=file_mode)
> robert> finally:
> robert> control_files.unlock()
> robert> - return self.open(transport, _found=True)
> robert> + # If we initialized using VFS methods on a RemoteTransport, return a
> robert> + # Remote object: No need for it to be slower than necessary.
>
> Meh. The comment implies that the remote case is optimized, yet,
> I thought that using _found=True was the way to optimize
> open... What do I miss here ? Can the comment be updated to make
> that clearer ?
_found=True avoids probing, but it would return a VFS object, and that
is slower than probing over the network and returning a Remote proxy
object, when actual operations are made. Its transient anyway, we'll get
back to this and fix the code to init over the wire, but that can wait.
> robert> @@ -1826,6 +1835,18 @@
> robert> # Trim the newline
> robert> return self.get_format_string().rstrip()
>
> robert> + def _supply_sub_formats_to(self, other_format):
> robert> + """Give other_format the same values for sub formats as this has.
> robert> +
> robert> + This method is expected to be used when parameterising a
> robert> + RemoteBzrDirFormat instance with the parameters from a
> robert> + BzrDirMetaFormat1 instance.
> robert> +
> robert> + :param other_format: other_format is a format which should be
> robert> + compatible with whatever sub formats are supported by self.
> robert> + :return: None.
> robert> + """
>
> A simple comment saying that doing nothing is the intended
> default behavior will avoid casual readers to suspect missing
> code (a simple 'pass' will be ok with me (I know it's not needed,
> just clearer)).
We use just a docstring elsewhere in the code for the same idiom; I'd
rather not start doing it two different ways.
> robert> === modified file 'bzrlib/remote.py'
> robert> --- bzrlib/remote.py 2009-01-27 07:48:55 +0000
> robert> +++ bzrlib/remote.py 2009-02-13 00:52:18 +0000
> robert> @@ -71,13 +71,13 @@
> robert> class RemoteBzrDir(BzrDir, _RpcHelper):
> robert> """Control directory on a remote server, accessed via bzr:// or similar."""
>
> robert> - def __init__(self, transport, _client=None):
> robert> + def __init__(self, transport, format, _client=None):
> robert> """Construct a RemoteBzrDir.
>
> robert> :param _client: Private parameter for testing. Disables probing and the
> robert> use of a real bzrdir.
> robert> """
>
> For coherency it would be better to keep the same parameter names
> than BzrDir, i.e. _transport and _format with leading
> underscores.
There is a defined factory interface for getting control dir objects, I
don't think there is any need for consistency in the constructors
(because the factory interface is not klass(...), but
klass.probe_on_transport(transport).
> robert> + expected_calls = [('set_rh', branch, [], True),
> robert> + ('set_rh', branch, [], True),
> robert> + ]
> robert> + if isinstance(branch, RemoteBranch):
> robert> + # For a remote branch, both the server and the client will raise
> robert> + # set_rh, and the server will do so first because that is where
> robert> + # the change takes place.
> robert> + self.assertEqual(expected_calls, self.hook_calls[2:])
> robert> + self.assertEqual(4, len(self.hook_calls))
> robert> + else:
> robert> + self.assertEqual(expected_calls, self.hook_calls)
>
> Why not use assertHookCalls here ?
Well, it doesn't quite fit at the moment. I'd suggest revisiting it on
further changes.
> robert> class TestOpen(TestCaseWithMemoryTransport):
> robert> @@ -120,25 +145,38 @@
> robert> def test_create(self):
> robert> self.install_hook()
> robert> b = self.make_branch('.')
> robert> - self.assertEqual([b], self.hook_calls)
> robert> + if isinstance(b, RemoteBranch):
> robert> + # RemoteBranch creation:
> robert> + # - creates the branch via the VFS
> robert> + # - does a branch open (by creating a RemoteBranch object)
> robert> + # - this has the same behaviour as simple branch opening, with an
> robert> + # additional VFS open at the front.
> robert> + self.assertEqual(b._real_branch, self.hook_calls[0])
> robert> + self.assertOpenedRemoteBranch(self.hook_calls[1:], b)
> robert> + else:
> robert> + self.assertEqual([b], self.hook_calls)
>
> And here ? (Even if you need to add assertOpenedRemoteBranch anyway).
Again, it doesn't quite fit at the moment; I think the tests are pretty
clear, and making the helper fit cases it doesn't will make it less
clear (at least in the short term) - its a bit of an unstable
equilibrium.
> 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.
Actually I preserved the order used in the code I was modifying :). This
is trivial to fix if we end up altering the patch.
> And this seems to be duplicated and can possibly be factored out
> in ChangeBranchTipTestCase ?
The name of the test gives its intent - there is one for Pre and one for
Post, it seems unredundant to me.
> 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).
No idea: am fixing things that were mechanically broken not checking the
meaning of each thing: the pattern of breakage was 'hooks are not
invoked by RemoteBranch consistently nor tested to behave'.
> <snip/>
>
> 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
> robert> @@ -39,16 +39,38 @@
> robert> self.assertEqual(source.bzrdir.root_transport.base, target.get_parent())
>
> robert> def test_sprout_uses_bzrdir_branch_format(self):
> robert> + # branch.sprout(bzrdir) is defined as using the branch format selected
> robert> + # by bzrdir; format preservation is achieved by parameterising the
> robert> + # bzrdir during bzrdir.sprout, which is where stacking compatibility
> robert> + # checks are done. So this test tests that each implementation of
> robert> + # Branch.sprout delegates appropriately to the bzrdir which the
> robert> + # branch is being created in, rather than testing that the result is
> robert> + # in the format that we are testing (which is what would happen if
> robert> + # the branch did not delegate appropriately).
>
> 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 :)
This could be a disincentive to make anything clearer :).
> robert> if isinstance(self.branch_format, _mod_branch.BranchReferenceFormat):
> robert> raise tests.TestNotApplicable('cannot sprout to a reference')
> robert> # Start with a format that is unlikely to be the target format
> robert> + # We call the super class to allow overriding the format of creation)
> robert> source = tests.TestCaseWithTransport.make_branch(self, 'old-branch',
> robert> format='metaweave')
> robert> target_bzrdir = self.make_bzrdir('target')
> robert> target_bzrdir.create_repository()
> robert> + result_format = self.branch_format
> robert> + if isinstance(target_bzrdir, remote.RemoteBzrDir):
> robert> + # for a remote bzrdir, we need to parameterise it with a branch
> robert> + # format, as, after creation, the newly opened remote objects
> robert> + # do not have one unless a branch was created at the time.
> robert> + # We use branch format 6 because its not the default, and its not
> robert> + # metaweave either.
> robert> + target_bzrdir._format.set_branch_format(_mod_branch.BzrBranchFormat6())
> robert> + result_format = target_bzrdir._format.get_branch_format()
> robert> target = source.sprout(target_bzrdir)
> robert> -
> robert> - self.assertIs(self.branch_format.__class__, target._format.__class__)
> 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 ?
> 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.
For whatever reason, this test was trivial to understand. In
particular, its not testing quite the same thing, which is probably why
it wasn't confusing.
> robert> self.assertEqual(target_repo._format, source_branch.repository._format)
>
> robert> def test_revert_inventory(self):
>
> robert> === modified file 'bzrlib/tests/per_repository/test_add_fallback_repository.py'
> robert> --- bzrlib/tests/per_repository/test_add_fallback_repository.py 2008-12-10 20:31:13 +0000
> robert> +++ bzrlib/tests/per_repository/test_add_fallback_repository.py 2009-02-13 00:52:18 +0000
> robert> @@ -61,6 +61,8 @@
> robert> repo.get_graph().get_parent_map([revision_id]))
> robert> # ... or on the repository's graph, when there is an other repository.
> robert> other = self.make_repository('other')
> robert> + other.lock_read()
> robert> + self.addCleanup(other.unlock)
>
> Amazing, Just out of curiosity how come the test was passing
> without the lock ?
Because of the use of real rather than remote objects probably.
> 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.
Which test is that?
> 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/ ?
I don't know if that is the intent or not - one of the original authors
might.
> 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 ?
Thats the implementation of the change I documented before, not a
simplification :).
> robert> def test_get_stacked_on_invalid_url(self):
> robert> - raise tests.KnownFailure('opening a branch requires the server to open the fallback repository')
> robert> - transport = FakeRemoteTransport('fakeremotetransport:///')
> robert> + # test that asking for a stacked on url the server can't access works.
> robert> + # This isn't perfect, but then as we're in the same process there
> robert> + # really isn't anything we can do to be 100% sure that the server
> robert> + # doesn't just open in - this test probably needs to be rewritten using
> robert> + # a spawn()ed server.
> robert> + stacked_branch = self.make_branch('stacked', format='1.9')
> robert> + memory_branch = self.make_branch('base', format='1.9')
> robert> + vfs_url = self.get_vfs_only_url('base')
> robert> + stacked_branch.set_stacked_on_url(vfs_url)
> robert> + transport = stacked_branch.bzrdir.root_transport
> robert> client = FakeClient(transport.base)
> robert> client.add_expected_call(
> robert> - 'Branch.get_stacked_on_url', ('.',),
> robert> - 'success', ('ok', 'file:///stacked/on'))
> robert> - bzrdir = RemoteBzrDir(transport, _client=client)
> robert> - branch = RemoteBranch(bzrdir, None, _client=client)
> robert> + 'Branch.get_stacked_on_url', ('stacked/',),
> robert> + 'success', ('ok', vfs_url))
> robert> + # XXX: Multiple calls are bad, this second call documents what is
> robert> + # today.
>
> s/is/is done/ ? s/today/& and should be fixed/ ?
s/is/happens/ if you like, though I was intending to use 'is', as the
present tense of etre. I tend to avoid writing promises in code though,
if it should be fixed and its worth noting that, a bug report is better
IMO. And in fact we'll be coming back to that anyway so it would just be
noise.
> <snip/>
>
> And That's all I have to say about that ;)
>
> Vincent
Thanks for the review. I'd like to know what the "corresponding" tests
are - that wasn't clear. I'll switch the order of those test parameters
you called out around when you point me at the right place. I'll also
note that doing a really long comment and copying it all over smells
like bad duplication to me - so I definitely don't want to do that.
-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/20090217/8bf336ab/attachment-0001.pgp
More information about the bazaar
mailing list