[merge] remove test_remote tests for deprecated methods
Martin Pool
mbp at canonical.com
Mon Mar 23 07:22:21 GMT 2009
2009/3/18 Robert Collins <robert.collins at canonical.com>:
> On Tue, 2009-03-17 at 20:52 +1100, Martin Pool wrote:
>> So in the course of removing code for things deprecated prior to 1.6,
>> I found I had to delete these two tests to make them pass. But I'm
>> not sure if that's really the best thing to do, and whether it'll
>> cause trouble for old clients or in connecting to old servers.
>>
>> I guess in general by removing deprecated code we will be breaking old
>> clients that depend on it. Keeping compatibility is good but we also
>> want to be able to clean the codebase.
>>
>> I don't have an immediate decision so I'll send this and see what you think...
>
> What about the tests required the deletion? If it was using
> get_revision_graph then a better approach is to just change the fallback
> in remote.py to use self._get_revision_graph(); which calls the server
> verb directly.
I wanted to delete methods that have been deprecated for a long time.
Therefore, I also need to delete or fix tests that call those methods.
However, I'm now questioning whether those methods actually should be
deleted or not - if they're still needed to maintain compatibility
with older servers or to test server compatibility with older clients
we may need to keep them around long after we expect any bzrlib api
users to have moved on.
I see the approach you've suggested there is correct: we can remove
the api-public method, and still keep on supporting old clients and
testing that support.
> As it stands this patch removes important test coverage - testing that
> we can talk to older servers.
>
> So please -
> - keep the fallback tests as they were, but remove the expected
> deprecations check - we don't need to trigger that.
> - make the client fallback to self._get_revision_graph rather than
> self.get_revision_graph.
Thanks.
To make it fallback we need to cope with the fact that get_parent_map
takes a list of keys, whereas get_parent_graph can apparently either
take just one revision or get the whole graph. That should be fine.
I see what seems to be a bug in RemoteRepository._get_parent_map_rpc
which is that it's careful to do this if the server's known to be old,
but not on the first call when it's discovered to be old:
1192 # There is an api discrepency between get_parent_map and
1193 # get_revision_graph. Specifically, a "key:()" pair in
1194 # get_revision_graph just means a node has no parents. For
1195 # "get_parent_map" it means the node is a ghost. So fix up the
1196 # graph to correct this.
1197 # https://bugs.launchpad.net/bzr/+bug/214894
1198 # There is one other "bug" which is that ghosts in
1199 # get_revision_graph() are not returned at all. But
we won't worry
1200 # about that for now.
1201 for node_id, parent_ids in rg.iteritems():
1202 if parent_ids == ():
1203 rg[node_id] = (NULL_REVISION,)
1204 rg[NULL_REVISION] = ()
1205 return rg
....
1251 except errors.UnknownSmartMethod:
1252 # Server does not support this method, so get the whole graph.
1253 # Worse, we have to force a disconnection, because
the server now
1254 # doesn't realise it has a body on the wire to consume, so the
1255 # only way to recover is to abandon the connection.
1256 warning(
1257 'Server is too old for fast get_parent_map,
reconnecting. '
1258 '(Upgrade the server to Bazaar 1.2 to avoid this)')
1259 medium.disconnect()
1260 # To avoid having to disconnect repeatedly, we keep
track of the
1261 # fact the server doesn't understand remote methods
added in 1.2.
1262 medium._remember_remote_is_before((1, 2))
1263 return self.get_revision_graph(None)
I think it's probably best handled by just making this method recurse
once? So that's what I've done in this incremental patch. Without
the change to recurse the test does in fact fail which I think shows
another occurrence of something like
https://bugs.launchpad.net/bzr/+bug/214894
=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py 2009-03-17 02:17:42 +0000
+++ bzrlib/remote.py 2009-03-23 07:18:40 +0000
@@ -1183,11 +1183,12 @@
# We already found out that the server can't understand
# Repository.get_parent_map requests, so just fetch the whole
# graph.
- # XXX: Note that this will issue a deprecation warning. This is ok
- # :- its because we're working with a deprecated server anyway, and
- # the user will almost certainly have seen a warning about the
- # server version already.
- rg = self.get_revision_graph()
+ #
+ # Note that this reads the whole graph, when only some keys are
+ # wanted. On this old server there's no way (?) to get them all
+ # in one go, and the user probably will have seen a warning about
+ # the server being old anyhow.
+ rg = self._get_revision_graph(None)
# There is an api discrepency between get_parent_map and
# get_revision_graph. Specifically, a "key:()" pair in
# get_revision_graph just means a node has no parents. For
@@ -1259,7 +1260,8 @@
# To avoid having to disconnect repeatedly, we keep track of the
# fact the server doesn't understand remote methods added in 1.2.
medium._remember_remote_is_before((1, 2))
- return self.get_revision_graph(None)
+ # Recurse just once and we should use the fallback code.
+ return self._get_parent_map_rpc(keys)
response_tuple, response_handler = response
if response_tuple[0] not in ['ok']:
response_handler.cancel_read_body()
=== modified file 'bzrlib/tests/test_remote.py'
--- bzrlib/tests/test_remote.py 2009-03-17 02:17:42 +0000
+++ bzrlib/tests/test_remote.py 2009-03-23 07:17:51 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006, 2007, 2008 Canonical Ltd
+# Copyright (C) 2006, 2007, 2008, 2009 Canonical Ltd
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -1561,16 +1561,12 @@
def test_get_parent_map_reconnects_if_unknown_method(self):
transport_path = 'quack'
+ rev_id = 'revision-id'
repo, client = self.setup_fake_client_and_repository(transport_path)
- client.add_unknown_method_response('Repository,get_parent_map')
- client.add_success_response_with_body('', 'ok')
+ client.add_unknown_method_response('Repository.get_parent_map')
+ client.add_success_response_with_body(rev_id, 'ok')
self.assertFalse(client._medium._is_remote_before((1, 2)))
- rev_id = 'revision-id'
- expected_deprecations = [
- 'bzrlib.remote.RemoteRepository.get_revision_graph was deprecated '
- 'in version 1.4.']
- parents = self.callDeprecated(
- expected_deprecations, repo.get_parent_map, [rev_id])
+ parents = repo.get_parent_map([rev_id])
self.assertEqual(
[('call_with_body_bytes_expecting_body',
'Repository.get_parent_map', ('quack/', rev_id), '\n\n0'),
@@ -1580,6 +1576,7 @@
client._calls)
# The medium is now marked as being connected to an older server
self.assertTrue(client._medium._is_remote_before((1, 2)))
+ self.assertEqual({rev_id: ('null:',)}, parents)
def test_get_parent_map_fallback_parentless_node(self):
"""get_parent_map falls back to get_revision_graph on old servers. The
@@ -1597,11 +1594,7 @@
repo, client = self.setup_fake_client_and_repository(transport_path)
client.add_success_response_with_body(rev_id, 'ok')
client._medium._remember_remote_is_before((1, 2))
- expected_deprecations = [
- 'bzrlib.remote.RemoteRepository.get_revision_graph was deprecated '
- 'in version 1.4.']
- parents = self.callDeprecated(
- expected_deprecations, repo.get_parent_map, [rev_id])
+ parents = repo.get_parent_map([rev_id])
self.assertEqual(
[('call_expecting_body', 'Repository.get_revision_graph',
('quack/', ''))],
--
Martin <http://launchpad.net/~mbp/>
More information about the bazaar
mailing list