[PATCH] fix bound branch SFTP test speed, and optimise somewhat.
Martin Pool
mbp at sourcefrog.net
Wed Apr 19 06:38:16 BST 2006
On 26/03/2006, at 7:50 AM, Robert Collins wrote:
> I've done a mini optimisation pass over the bound branch section of
> the
> test suite. There are some clearly right things:
>
> * reduce lock-unlock sequences in WorkingTree disk construction.
> * fix the 5 second timeout that was triggering in the bound branch
> SFTP
> tests.
>
> There is some more controversial stuff though, which may need
> discussion. I've changed the blackbox tests for bound branches a
> little
> - to use the bzrlib api *when setting up the test*, and the ui only to
> execute the test itself. I think this is a nicer pattern on balance
> than
> using the command line tool only, for several reasons:
> - Its impossible to test failure modes adequately with just the
> command
> line API because its designed *not* to introduce bad state.
> - Its (IMO) often clearer to use the library api when evaluating
> whether the outcome [not the output, rather the resulting disk
> state] of
> a test is correct.
> - We have more flexability when we need it, and having the library
> api
> routinely used means that folk patching or adding bugs will have good
> examples and not have to struggle over the question of whether to
> use it
> or not.
Right, and also it lets us consider whether the API is useful and
tasteful for typical uses outside of bzr itself. I agree with that
approach.
+1 for the patch, with some comments.
> === modified file 'a/bzrlib/builtins.py'
> --- a/bzrlib/builtins.py
> +++ b/bzrlib/builtins.py
> @@ -2041,7 +2041,7 @@
> remote_branch = bzrlib.branch.Branch.open(other_branch)
> remote_branch.lock_read()
> try:
> - local_branch.lock_write()
> + local_branch.lock_read()
> try:
> local_extra, remote_extra = find_unmerged
(local_branch, remote_branch)
> if (log_format == None):
> @@ -2073,11 +2073,16 @@
> print "Branches are up to date."
> else:
> status_code = 1
> - if parent is None and other_branch is not None:
> - local_branch.set_parent(other_branch)
> - return status_code
> finally:
> local_branch.unlock()
> + if parent is None and other_branch is not None:
> + local_branch.lock_write()
> + try:
> + if local_branch.get_parent() is not None:
> + local_branch.set_parent(other_branch)
> + finally:
> + local_branch.unlock()
> + return status_code
> finally:
> remote_branch.unlock()
>
>
> === modified file 'a/bzrlib/tests/blackbox/test_bound_branches.py'
> --- a/bzrlib/tests/blackbox/test_bound_branches.py
> +++ b/bzrlib/tests/blackbox/test_bound_branches.py
> @@ -66,28 +66,22 @@
> bzr = self.run_bzr
> self.build_tree(['base/', 'base/a', 'base/b'])
>
> - self.init_meta_branch('base')
> - os.chdir('base')
> - bzr('add')
> - bzr('commit', '-m', 'init')
> -
> - os.chdir('..')
> -
> - bzr('checkout', 'base', 'child')
> -
> - self.failUnlessExists('child')
> + branch = self.init_meta_branch('base')
> + tree = branch.bzrdir.open_workingtree()
> + tree.lock_write()
> + tree.add(['a', 'b'])
> + tree.commit('init')
> + tree.unlock()
> +
> + self.run_bzr('checkout', 'base', 'child')
>
> self.check_revno(1, 'child')
> d = BzrDir.open('child')
> self.assertNotEqual(None, d.open_branch
().get_master_branch())
>
> - def check_revno(self, val, loc=None):
> - if loc is not None:
> - cwd = os.getcwd()
> - os.chdir(loc)
> - self.assertEquals(str(val), self.run_bzr('revno')[0].strip
())
> - if loc is not None:
> - os.chdir(cwd)
> + def check_revno(self, val, loc='.'):
> + self.assertEqual(
> + val, len(BzrDir.open(loc).open_branch
().revision_history()))
>
> def test_simple_binding(self):
> self.build_tree(['base/', 'base/a', 'base/b'])
> @@ -113,7 +107,8 @@
> old_format = BzrDirFormat.get_default_format()
> BzrDirFormat.set_default_format(BzrDirMetaFormat1())
> try:
> - self.run_bzr('init', path)
> + return BzrDir.create_branch_convenience(
> + path, BzrDirMetaFormat1())
> finally:
> BzrDirFormat.set_default_format(old_format)
>
> @@ -285,8 +280,11 @@
> self.check_revno(5)
>
> def test_bind_child_ahead(self):
> - bzr = self.run_bzr
> - self.create_branches()
> + # test binding when the master branches history is a
prefix of the
> + # childs
> + bzr = self.run_bzr
> + self.create_branches()
> + return
>
> os.chdir('child')
> bzr('unbind')
>
> === modified file 'a/bzrlib/tests/blackbox/test_missing.py'
> --- a/bzrlib/tests/blackbox/test_missing.py
> +++ b/bzrlib/tests/blackbox/test_missing.py
> @@ -6,7 +6,9 @@
> from bzrlib.branch import Branch
> from bzrlib.tests import TestCaseInTempDir
>
> +
> class TestMissing(TestCaseInTempDir):
> +
> def test_missing(self):
> missing = "You are missing 1 revision(s):"
>
> @@ -24,12 +26,29 @@
> open('a', 'ab').write('more\n')
> self.capture('commit -m more')
>
> - # compare a against b
> + # run missing in a against b
> os.chdir('../a')
> - lines = self.capture('missing ../b', retcode=1).splitlines()
> + # this should not require missing to take out a write
lock on a
> + # or b. So we take a write lock on both to test that at
the same
> + # time. This may let the test pass while the default
branch is an
> + # os-locking branch, but it will trigger failures with
lockdir based
> + # branches.
> + branch_a = Branch.open('.')
> + branch_a.lock_write()
> + branch_b = Branch.open('../b')
> + branch_b.lock_write()
> + out,err = self.run_bzr('missing', '../b', retcode=1)
> + lines = out.splitlines()
> # we're missing the extra revision here
> self.assertEqual(missing, lines[0])
> + # and we expect 8 lines of output which we trust at the
moment to be
> + # good.
> self.assertEqual(8, len(lines))
> + # we do not expect any error output.
> + self.assertEqual('', err)
> + # unlock the branches for the rest of the test
> + branch_a.unlock()
> + branch_b.unlock()
>
> # get extra revision from b
> self.capture('merge ../b')
>
> === modified file 'a/bzrlib/tests/branch_implementations/
test_bound_sftp.py'
> --- a/bzrlib/tests/branch_implementations/test_bound_sftp.py
> +++ b/bzrlib/tests/branch_implementations/test_bound_sftp.py
> @@ -19,7 +19,7 @@
>
> import os
>
> -
> +import bzrlib
> from bzrlib.branch import Branch
> from bzrlib.bzrdir import (BzrDir,
> BzrDirFormat,
> @@ -56,6 +56,11 @@
>
> return b_base, wt_child
>
> + def tearDown(self):
> + self.sftp_base = None
> + bzrlib.transport.sftp.clear_connection_cache()
> + super(BoundSFTPBranch, self).tearDown()
> +
> def test_simple_binding(self):
> self.build_tree(['base/', 'base/a', 'base/b', 'child/'])
> wt_base = BzrDir.create_standalone_workingtree('base')
>
> === modified file 'a/bzrlib/transport/sftp.py'
> --- a/bzrlib/transport/sftp.py
> +++ b/bzrlib/transport/sftp.py
> @@ -183,6 +183,14 @@
> # sort of expiration policy, such as disconnect if inactive for
> # X seconds. But that requires a lot more fanciness.
> _connected_hosts = weakref.WeakValueDictionary()
> +
> +def clear_connection_cache():
> + """Remove all hosts from the SFTP connection cache.
> +
> + Primarily useful for test cases wanting to force garbage
collection.
> + """
> + while _connected_hosts:
> + _connected_hosts.popitem()
Is thee any reason why this is not just _connected_hosts.clear()?
(If so, add a comment, so it's not changed unknowingly.)
>
>
> def load_host_keys():
>
> === modified file 'a/bzrlib/workingtree.py'
> --- a/bzrlib/workingtree.py
> +++ b/bzrlib/workingtree.py
> @@ -1489,6 +1489,7 @@
> transport = a_bzrdir.get_workingtree_transport(self)
> control_files = self._open_control_files(a_bzrdir)
> control_files.create_lock()
> + control_files.lock_write()
> control_files.put_utf8('format', self.get_format_string())
> branch = a_bzrdir.open_branch()
> if revision_id is None:
> @@ -1501,11 +1502,16 @@
> _format=self,
> _bzrdir=a_bzrdir,
> _control_files=control_files)
> - wt._write_inventory(inv)
> - wt.set_root_id(inv.root.file_id)
> - wt.set_last_revision(revision_id)
> - wt.set_pending_merges([])
> - build_tree(wt.basis_tree(), wt)
> + wt.lock_write()
> + try:
> + wt._write_inventory(inv)
> + wt.set_root_id(inv.root.file_id)
> + wt.set_last_revision(revision_id)
> + wt.set_pending_merges([])
> + build_tree(wt.basis_tree(), wt)
> + finally:
> + wt.unlock()
> + control_files.unlock()
> return wt
>
> def __init__(self):
>
There seems to be a window of statements run with control_files
locked but guarded by a try/finally block. Isn't that a problem?
--
Martin
More information about the bazaar
mailing list