[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.
Vincent Ladeuil
v.ladeuil+lp at free.fr
Mon Feb 16 13:29:47 GMT 2009
Nice refactoring job in the remote tests, 1 test still failing:
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'>.
Some very nice and clear comments, which prompted some requests for more :-)
BB:tweak
>>>>> "robert" == Robert Collins <robertc at robertcollins.net> writes:
robert> This branch is needed to let a test of 'bzr push bzr+ssh://host/foo'
robert> work. Its needed because we were not actually using the smart server for
robert> new branches, and when we dove down into that we found rather more rot
robert> than expected. So this branch:
robert> - fixes RemoteBranch to fire hooks as other Branch implementations do
robert> - fixes branch_implementation tests to handle RemoteBranch
robert> - fixes branch_implementation tests to break if self.make_branch() does
robert> not give back the format under test.
robert> -Rob
robert> # Bazaar merge directive format 2 (Bazaar 0.90)
robert> # revision_id: robertc at robertcollins.net-20090213005218-\
robert> # yxrgiq7j1du2vldm
robert> # target_branch: http://bazaar-vcs.org/bzr/bzr.dev
robert> # testament_sha1: 6ff883a2b427ab9c7e1861ff05f650103d1518b5
robert> # timestamp: 2009-02-13 11:52:28 +1100
robert> # source_branch: http://people.ubuntu.com/~robertc/baz2.0/format-on-\
robert> # push
robert> # base_revision_id: pqm at pqm.ubuntu.com-20090212154913-cleqwmh36ss3gswk
robert> #
robert> # Begin patch
robert> === modified file 'NEWS'
robert> --- NEWS 2009-02-11 22:25:18 +0000
robert> +++ NEWS 2009-02-13 00:52:18 +0000
robert> @@ -28,6 +28,11 @@
robert> * ``bzr unshelve`` gives a more palatable error if passed a non-integer
robert> shelf id. (Daniel Watkins)
robert> + * Many Branch hooks would not fire with ``bzr://`` and ``bzr+ssh://``
robert> + branches, and this was not noticed due to a bug in the test logic
robert> + for branches. This is now fixed and a test added to prevent it
robert> + reoccuring. (Robert Collins, Andrew Bennetts)
robert> +
robert> DOCUMENTATION:
robert> * The documentation for ``shelve`` and ``unshelve`` has been clarified.
robert> @@ -44,6 +49,10 @@
robert> command object before the command is run (or help generated from
robert> it), without overriding the command. (Robert Collins)
robert> + * Some methods have been pulled up from ``BzrBranch`` to ``Branch``
robert> + to aid branch types that are not bzr branch objects (like
robert> + RemoteBranch). (Robert Collins, Andrew Bennetts)
robert> +
robert> bzr 1.12rc1 "1234567890" 2009-02-10
robert> -----------------------------------
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.
<snip/>
robert> === modified file 'bzrlib/bzrdir.py'
robert> --- bzrlib/bzrdir.py 2009-02-10 05:47:45 +0000
robert> +++ bzrlib/bzrdir.py 2009-02-13 00:52:18 +0000
robert> @@ -64,6 +64,7 @@
robert> do_catching_redirections,
robert> get_transport,
robert> local,
robert> + remote as remote_transport,
robert> )
robert> from bzrlib.weave import Weave
robert> """)
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 ?
robert> + if isinstance(transport, remote_transport.RemoteTransport):
robert> + return self.open(transport)
robert> + else:
robert> + return self.open(transport, _found=True)
robert> def is_supported(self):
robert> """Is this format supported?
robert> @@ -1781,6 +1787,9 @@
robert> raise AssertionError("%s was asked to open %s, but it seems to need "
robert> "format %s"
robert> % (self, transport, found_format))
robert> + # Allow subclasses - use the found format.
robert> + self._supply_sub_formats_to(found_format)
robert> + return found_format._open(transport)
robert> return self._open(transport)
robert> def _open(self, transport):
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)).
<snip/>
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.
<snip/>
robert> === modified file 'bzrlib/tests/branch_implementations/test_hooks.py'
robert> --- bzrlib/tests/branch_implementations/test_hooks.py 2008-09-23 03:21:07 +0000
robert> +++ bzrlib/tests/branch_implementations/test_hooks.py 2009-02-13 00:52:18 +0000
<snip/>
robert> @@ -76,36 +120,17 @@
robert> Branch.hooks.install_named_hook('set_rh', self.capture_set_rh_hook,
robert> None)
robert> branch.set_revision_history([])
<snip/>
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 ?
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).
<snip/>
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.
And this seems to be duplicated and can possibly be factored out
in ChangeBranchTipTestCase ?
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).
<snip/>
robert> @@ -292,8 +334,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)
Ditto.
<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 :)
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> + self.assertIs(result_format.__class__,
robert> + target._format.__class__)
robert> def test_sprout_partial(self):
robert> # test sprouting with a prefix of the revision-history.
robert> === modified file 'bzrlib/tests/bzrdir_implementations/test_bzrdir.py'
robert> --- bzrlib/tests/bzrdir_implementations/test_bzrdir.py 2009-02-10 03:24:28 +0000
robert> +++ bzrlib/tests/bzrdir_implementations/test_bzrdir.py 2009-02-13 00:52:18 +0000
robert> @@ -58,7 +58,7 @@
robert> from bzrlib.transport import get_transport
robert> from bzrlib.transport.local import LocalTransport
robert> from bzrlib.upgrade import upgrade
robert> -from bzrlib.remote import RemoteBzrDir
robert> +from bzrlib.remote import RemoteBzrDir, RemoteRepository
robert> from bzrlib.repofmt import weaverepo
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> 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 ?
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> +
robert> # The repository format is preserved.
robert> - self.assertEqual(repo._format, target.open_repository()._format)
robert> + self.assertEqual(repo._format, target_repo._format)
robert> def test__make_parents_provider(self):
robert> """Repositories must have a _make_parents_provider method that returns
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> 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 ?
<snip/>
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/ ?
<snip/>
And That's all I have to say about that ;)
Vincent
More information about the bazaar
mailing list