Merge lp:~lifeless/bzr/hpss_ratches into lp:~bzr/bzr/trunk-old

[Robert Collins]

[Robert Collins (lifeless)]

My small yak shave for the day - do the TODO Andrew and I have had for
hpss ratchets. We had a warning we wanted adjacent to the ratchet
figure, and this patch preserves that but moves all the ratchet figures
to one place. Its not the only way to tackle it, but this seems fairly
small and clear.

-Rob

--

[Martin Pool (mbp)]

> My small yak shave for the day - do the TODO Andrew and I have had for
> hpss ratchets. We had a warning we wanted adjacent to the ratchet
> figure, and this patch preserves that but moves all the ratchet figures
> to one place. Its not the only way to tackle it, but this seems fairly
> small and clear.

It's nice to remove the duplication but this seems like a really convoluted way to do it.

The Ratchet doesn't add any value beyond just holding a callback.

Passing something that's almost but not quite the name of the test into a dictionary is just an invitation for them to get out of sync.

It seems like you want to separate out the definition of the length by moving it to a place people will not easily change it, but that's just a bit like security by obscurity.

I'd rather just see the assertion changed to something like self.check_network_effort(...) and then put a comment on that method about not changing it.

review: Disapprove

[Robert Collins (lifeless)]

On Mon, 2009-08-10 at 03:12 +0000, Martin Pool wrote:
>
>
> It's nice to remove the duplication but this seems like a really
> convoluted way to do it.
>
> The Ratchet doesn't add any value beyond just holding a callback.

Thats true. I could have used partial function application too, but I
felt that would be more obscure.

> Passing something that's almost but not quite the name of the test
> into a dictionary is just an invitation for them to get out of sync.

I don't see that happening, because people have to make explicit
changes.

> It seems like you want to separate out the definition of the length by
> moving it to a place people will not easily change it, but that's just
> a bit like security by obscurity.

Can you be clearer about the parallels here? I'm trying to make sure
that when people change the figure:
 - its obvious to reviewers that its a ratchet figure
 - whoever is changing it has read the text about changing the figures

> I'd rather just see the assertion changed to something like
> self.check_network_effort(...) and then put a comment on that method
> about not changing it.

This doesn't meet the goals at all, or we would have just done that at
the start.
-Rob

[Martin Pool (mbp)]

2009/8/10 Robert Collins <email address hidden>:

>> Passing something that's almost but not quite the name of the test
>> into a dictionary is just an invitation for them to get out of sync.
>
> I don't see that happening, because people have to make explicit
> changes.

My point is: you're introducing two quoted strings in different parts
of the codebase that need to be in sync with each other. Why? The
point is that the ratchet corresponds to the test and there is already
an object (the bound method) corresponding to that context.

>> It seems like you want to separate out the definition of the length by
>> moving it to a place people will not easily change it, but that's just
>> a bit like security by obscurity.
>
> Can you be clearer about the parallels here?

The parallel to security by obscurity (and it's not a very close one)
is: why did you put the check on the number of round trips about four
logical steps away from the place it's checked? Presumably so people
don't unwittingly change it, but I'm not sure it's a good tradeoff.

> I'm trying to make sure
> that when people change the figure:
>  - its obvious to reviewers that its a ratchet figure
>  - whoever is changing it has read the text about changing the figures

To me the simplest thing that could possibly work is a specific assertion, like

>> I'd rather just see the assertion changed to something like
>> self.check_network_effort(...) and then put a comment on that method
>> about not changing it.
>
> This doesn't meet the goals at all, or we would have just done that at
> the start.

To me that would clearly meet the first. I'm not sure if it would
ensure that the person had really thought about the consequences of
changing it, but it at least gives them a reasonable warning. If
someone's just going to randomly change things until the tests pass no
amount of comments are going to stop them.

So I'd like to work out what specific requirement a clear assertion
method would not meet.

Maybe the failure message could give some guidance for both the up and
down cases.

--
Martin <http://launchpad.net/~mbp/>

[Robert Collins (lifeless)]

On Mon, 2009-08-10 at 04:45 +0000, Martin Pool wrote:
> 2009/8/10 Robert Collins <email address hidden>:
>
> >> Passing something that's almost but not quite the name of the test
> >> into a dictionary is just an invitation for them to get out of sync.
> >
> > I don't see that happening, because people have to make explicit
> > changes.
>
> My point is: you're introducing two quoted strings in different parts
> of the codebase that need to be in sync with each other. Why? The
> point is that the ratchet corresponds to the test and there is already
> an object (the bound method) corresponding to that context.

Ok. By the way, the disapprove vote may have thrown me off; it felt like
you were saying you didn't want to get the problem fixed, but your
review was more of a resubmit. I wonder perhaps if we shouldn't use
disapprove except when the goal of the patch is wrong?

> >> It seems like you want to separate out the definition of the length by
> >> moving it to a place people will not easily change it, but that's just
> >> a bit like security by obscurity.
> >
> > Can you be clearer about the parallels here?
>
> The parallel to security by obscurity (and it's not a very close one)
> is: why did you put the check on the number of round trips about four
> logical steps away from the place it's checked? Presumably so people
> don't unwittingly change it, but I'm not sure it's a good tradeoff.

Thats precisely it - I want to draw attention to the conceptual issues
surrounding the ratchets.

> >> I'd rather just see the assertion changed to something like
> >> self.check_network_effort(...) and then put a comment on that method
> >> about not changing it.
> >
> > This doesn't meet the goals at all, or we would have just done that at
> > the start.
>
> To me that would clearly meet the first. I'm not sure if it would
> ensure that the person had really thought about the consequences of
> changing it, but it at least gives them a reasonable warning. If
> someone's just going to randomly change things until the tests pass no
> amount of comments are going to stop them.

I don't think we're dealing with hostile contributors. I don't think a
single line phrase can really get the concept across though.

> So I'd like to work out what specific requirement a clear assertion
> method would not meet.
>
> Maybe the failure message could give some guidance for both the up and
> down cases.

I think having a custom failure message is better than embedding
assertLength in each method, but it would still concern me vs moving the
assertions somewhere specific to hpss specific code.

Another way to approach it would be to move the tests - breaking our
'tests for X are in Y' general rule, but making it possible to document
the ratchet aspect once in a single test module.

-Rob

[Martin Pool (mbp)]

2009/8/10 Robert Collins <email address hidden>:
> Ok. By the way, the disapprove vote may have thrown me off; it felt like
> you were saying you didn't want to get the problem fixed, but your
> review was more of a resubmit. I wonder perhaps if we shouldn't use
> disapprove except when the goal of the patch is wrong?

Oh, ok. Probably. There's probably some kind of Launchpad ui deficit here too.

>> The parallel to security by obscurity (and it's not a very close one)
>> is: why did you put the check on the number of round trips about four
>> logical steps away from the place it's checked?  Presumably so people
>> don't unwittingly change it, but I'm not sure it's a good tradeoff.
>
> Thats precisely it - I want to draw attention to the conceptual issues
> surrounding the ratchets.

So let's assume our developers are smart, and aim to get it at at
least the level of "the interface encourages you to get it right."
If it turns out that regressions of these ratchets are actually
getting through review, then we can use a bigger hammer.

It's interesting to contemplate what we could do to avoid you ever
being able to get it wrong, but I don't think that making it harder to
change is enough.

This is only one aspect of defense against the dark ^w^w performance
regressions, and people can still break cases that aren't tested, such
as old formats or larger trees.

--
Martin <http://launchpad.net/~mbp/>

Unmerged revisions

4596. By Robert Collins on 2009-08-10

Remove duplicate warnings about hpss ratchets by centralising all the ratchets in bzrlib.tests. (Robert Collins)

4595. By Canonical.com Patch Queue Manager <email address hidden> on 2009-08-07

(mbp) small tweak to bundle_info tests

4594. By Canonical.com Patch Queue Manager <email address hidden> on 2009-08-07

(mbp) fix 2a test failure in merge directive cherrypick test

4593. By Canonical.com Patch Queue Manager <email address hidden> on 2009-08-06

(robertc) Partial overhaul of check to do less duplicate work.
 (Robert Collins)

4592. By Canonical.com Patch Queue Manager <email address hidden> on 2009-08-05

Updates to buildout.cfg etc to have 'make installer-all' start being
 the preferred way to build win32 installer.

4591. By Canonical.com Patch Queue Manager <email address hidden> on 2009-08-05

(jam) Small 'const' fixes for diff-delta.c (bug #408441)

4590. By Canonical.com Patch Queue Manager <email address hidden> on 2009-08-04

(Jelmer) Document bzr_transports property in the plugin API.

4589. By Canonical.com Patch Queue Manager <email address hidden> on 2009-08-04

(bialix) Include the image format plugins in the win32 all-in-one
 installer.

4588. By Canonical.com Patch Queue Manager <email address hidden> on 2009-08-04

(jam) Get bundles working with --2a (bug #393349)

4587. By Canonical.com Patch Queue Manager <email address hidden> on 2009-08-04

(mbp) further LockableFiles cleanups

=== modified file 'NEWS'
--- NEWS	2009-08-28 22:24:40 +0000
+++ NEWS	2009-08-29 03:36:19 +0000
@@ -532,6 +532,10 @@
 Testing
 *******
 
+* HPSS ratchets are now centralised in the ``setup_smart_server_with_call_log``
+  helper method, removing the need to duplicate the warning about changing
+  them throughout the code base. (Robert Collins)
+
 * Merge directive cherrypick tests must use the same root id.
   (Martin Pool, #409684)
 
 
=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2009-08-28 21:05:31 +0000
+++ bzrlib/tests/__init__.py	2009-08-29 03:36:19 +0000
@@ -2298,7 +2298,12 @@
         self.reduceLockdirTimeout()
 
     def setup_smart_server_with_call_log(self):
-        """Sets up a smart server as the transport server with a call log."""
+        """Sets up a smart server as the transport server with a call log.
+        
+        This is typically used for hpss regression tests, and in addition to
+        setting up the smart server a collection of hpss ratchets are created
+        at self.hpss_ratchets.
+        """
         self.transport_server = server.SmartTCPServer_for_testing
         self.hpss_calls = []
         import traceback
@@ -2310,6 +2315,23 @@
                 CapturedCall(params, prefix_length))
         client._SmartClient.hooks.install_named_hook(
             'call', capture_hpss_call, None)
+        # These figures represent the amount of work to perform each use case.
+        # It is entirely ok to reduce a figure if a test fails due to rpc_count
+        # being too low. If the rpc_count increases, more network roundtrips
+        # have become necessary for this use case. Please do not adjust these
+        # figures upwards without agreement from bzr's network support
+        # maintainers.
+        self.hpss_ratchets = {
+            'push_smart_non_stacked_streaming': Ratchet(self.assertLength, 9),
+            'push_smart_stacked_streaming': Ratchet(self.assertLength, 14),
+            'push_smart_tags_streaming': Ratchet(self.assertLength, 11),
+            'init_on_format': Ratchet(self.assertLength, 2),
+            'branch_from_trivial_same_server': Ratchet(self.assertLength, 38),
+            'branch_from_trivial': Ratchet(self.assertLength, 10),
+            'branch_from_trivial_stacked': Ratchet(self.assertLength, 15),
+            'pull_smart_stacked': Ratchet(self.assertLength, 18),
+            'init_repo': Ratchet(self.assertLength, 16),
+            }
 
     def reset_smart_call_log(self):
         self.hpss_calls = []
@@ -4025,6 +4047,35 @@
     return None
 
 
+class Ratchet(object):
+    """A Ratchet separates an assertion from code that needs to check it.
+    
+    This is useful to allow checking ratchet style assertions - such as that
+    no more than N round trips are made during an operation, without having
+    the expected values spread out over the code base where the can be changed
+    without reading documentation about them.
+
+    A typical use is to prepare a Ratchet in setUp, and then test it later.
+    """
+
+    def __init__(self, assertion, *args):
+        """Create a ratchet.
+
+        :param assertion: The assertion to invoke when the ratchet is checked.
+        :param *args: positional arguments to pass to the assertion.
+        """
+        self.assertion = assertion
+        self.args = args
+
+    def check(self, *args):
+        """Check the ratchet.
+
+        :param args: Additional arguments to pass to the ratchet. Typically the
+            expected value of the underlying assertion.
+        """
+        return self.assertion(*(self.args + args))
+
+
 class _HTTPSServerFeature(Feature):
     """Some tests want an https Server, check if one is available.
 
 
=== modified file 'bzrlib/tests/blackbox/test_branch.py'
--- bzrlib/tests/blackbox/test_branch.py	2009-08-20 12:30:49 +0000
+++ bzrlib/tests/blackbox/test_branch.py	2009-08-29 03:36:20 +0000
@@ -353,12 +353,7 @@
         self.reset_smart_call_log()
         out, err = self.run_bzr(['branch', self.get_url('from'),
             self.get_url('target')])
-        # This figure represent the amount of work to perform this use case. It
-        # is entirely ok to reduce this number if a test fails due to rpc_count
-        # being too low. If rpc_count increases, more network roundtrips have
-        # become necessary for this use case. Please do not adjust this number
-        # upwards without agreement from bzr's network support maintainers.
-        self.assertLength(38, self.hpss_calls)
+        self.hpss_ratchets['branch_from_trivial_same_server'].check(self.hpss_calls)
 
     def test_branch_from_trivial_branch_streaming_acceptance(self):
         self.setup_smart_server_with_call_log()
@@ -368,12 +363,7 @@
         self.reset_smart_call_log()
         out, err = self.run_bzr(['branch', self.get_url('from'),
             'local-target'])
-        # This figure represent the amount of work to perform this use case. It
-        # is entirely ok to reduce this number if a test fails due to rpc_count
-        # being too low. If rpc_count increases, more network roundtrips have
-        # become necessary for this use case. Please do not adjust this number
-        # upwards without agreement from bzr's network support maintainers.
-        self.assertLength(10, self.hpss_calls)
+        self.hpss_ratchets['branch_from_trivial'].check(self.hpss_calls)
 
     def test_branch_from_trivial_stacked_branch_streaming_acceptance(self):
         self.setup_smart_server_with_call_log()
@@ -388,12 +378,7 @@
         self.reset_smart_call_log()
         out, err = self.run_bzr(['branch', self.get_url('feature'),
             'local-target'])
-        # This figure represent the amount of work to perform this use case. It
-        # is entirely ok to reduce this number if a test fails due to rpc_count
-        # being too low. If rpc_count increases, more network roundtrips have
-        # become necessary for this use case. Please do not adjust this number
-        # upwards without agreement from bzr's network support maintainers.
-        self.assertLength(15, self.hpss_calls)
+        self.hpss_ratchets['branch_from_trivial_stacked'].check(self.hpss_calls)
 
 
 class TestRemoteBranch(TestCaseWithSFTPServer):
 
=== modified file 'bzrlib/tests/blackbox/test_pull.py'
--- bzrlib/tests/blackbox/test_pull.py	2009-06-15 06:47:14 +0000
+++ bzrlib/tests/blackbox/test_pull.py	2009-08-29 03:36:20 +0000
@@ -382,12 +382,7 @@
         self.reset_smart_call_log()
         self.run_bzr(['pull', '-r', '1', self.get_url('stacked')],
             working_dir='empty')
-        # This figure represent the amount of work to perform this use case. It
-        # is entirely ok to reduce this number if a test fails due to rpc_count
-        # being too low. If rpc_count increases, more network roundtrips have
-        # become necessary for this use case. Please do not adjust this number
-        # upwards without agreement from bzr's network support maintainers.
-        self.assertLength(18, self.hpss_calls)
+        self.hpss_ratchets['pull_smart_stacked'].check(self.hpss_calls)
         remote = Branch.open('stacked')
         self.assertEndsWith(remote.get_stacked_on_url(), '/parent')
 
 
=== modified file 'bzrlib/tests/blackbox/test_push.py'
--- bzrlib/tests/blackbox/test_push.py	2009-08-27 22:17:35 +0000
+++ bzrlib/tests/blackbox/test_push.py	2009-08-29 03:36:20 +0000
@@ -214,12 +214,8 @@
         t.commit(allow_pointless=True, message='first commit')
         self.reset_smart_call_log()
         self.run_bzr(['push', self.get_url('to-one')], working_dir='from')
-        # This figure represent the amount of work to perform this use case. It
-        # is entirely ok to reduce this number if a test fails due to rpc_count
-        # being too low. If rpc_count increases, more network roundtrips have
-        # become necessary for this use case. Please do not adjust this number
-        # upwards without agreement from bzr's network support maintainers.
-        self.assertLength(9, self.hpss_calls)
+        self.hpss_ratchets['push_smart_non_stacked_streaming'].check(
+            self.hpss_calls)
 
     def test_push_smart_stacked_streaming_acceptance(self):
         self.setup_smart_server_with_call_log()
@@ -230,12 +226,8 @@
         self.reset_smart_call_log()
         self.run_bzr(['push', '--stacked', '--stacked-on', '../parent',
             self.get_url('public')], working_dir='local')
-        # This figure represent the amount of work to perform this use case. It
-        # is entirely ok to reduce this number if a test fails due to rpc_count
-        # being too low. If rpc_count increases, more network roundtrips have
-        # become necessary for this use case. Please do not adjust this number
-        # upwards without agreement from bzr's network support maintainers.
-        self.assertLength(14, self.hpss_calls)
+        self.hpss_ratchets['push_smart_stacked_streaming'].check(
+            self.hpss_calls)
         remote = branch.Branch.open('public')
         self.assertEndsWith(remote.get_stacked_on_url(), '/parent')
 
@@ -246,12 +238,7 @@
         t.branch.tags.set_tag('new-tag', rev_id)
         self.reset_smart_call_log()
         self.run_bzr(['push', self.get_url('to-one')], working_dir='from')
-        # This figure represent the amount of work to perform this use case. It
-        # is entirely ok to reduce this number if a test fails due to rpc_count
-        # being too low. If rpc_count increases, more network roundtrips have
-        # become necessary for this use case. Please do not adjust this number
-        # upwards without agreement from bzr's network support maintainers.
-        self.assertLength(11, self.hpss_calls)
+        self.hpss_ratchets['push_smart_tags_streaming'].check(self.hpss_calls)
 
     def test_push_smart_with_default_stacking_url_path_segment(self):
         # If the default stacked-on location is a path element then branches
 
=== modified file 'bzrlib/tests/blackbox/test_shared_repository.py'
--- bzrlib/tests/blackbox/test_shared_repository.py	2009-08-12 23:52:15 +0000
+++ bzrlib/tests/blackbox/test_shared_repository.py	2009-08-29 03:36:20 +0000
@@ -114,9 +114,4 @@
         # be fixed.
         self.setup_smart_server_with_call_log()
         self.run_bzr(['init-repo', self.get_url('repo')])
-        # This figure represent the amount of work to perform this use case. It
-        # is entirely ok to reduce this number if a test fails due to rpc_count
-        # being too low. If rpc_count increases, more network roundtrips have
-        # become necessary for this use case. Please do not adjust this number
-        # upwards without agreement from bzr's network support maintainers.
-        self.assertLength(16, self.hpss_calls)
+        self.hpss_ratchets['init_repo'].check(self.hpss_calls)
 
=== modified file 'bzrlib/tests/test_bzrdir.py'
--- bzrlib/tests/test_bzrdir.py	2009-07-10 07:14:02 +0000
+++ bzrlib/tests/test_bzrdir.py	2009-08-29 03:36:19 +0000
@@ -906,13 +906,7 @@
         self.reset_smart_call_log()
         instance = new_format.initialize_on_transport(transport)
         self.assertIsInstance(instance, remote.RemoteBzrDir)
-        rpc_count = len(self.hpss_calls)
-        # This figure represent the amount of work to perform this use case. It
-        # is entirely ok to reduce this number if a test fails due to rpc_count
-        # being too low. If rpc_count increases, more network roundtrips have
-        # become necessary for this use case. Please do not adjust this number
-        # upwards without agreement from bzr's network support maintainers.
-        self.assertEqual(2, rpc_count)
+        self.hpss_ratchets['init_on_format'].check(self.hpss_calls)
 
 
 class TestFormat5(TestCaseWithTransport):
 
=== modified file 'bzrlib/tests/test_selftest.py'
--- bzrlib/tests/test_selftest.py	2009-08-26 23:25:28 +0000
+++ bzrlib/tests/test_selftest.py	2009-08-29 03:36:20 +0000
@@ -2965,3 +2965,33 @@
                                                 self.verbosity)
         tests.run_suite(suite, runner_class=MyRunner, stream=StringIO())
         self.assertLength(1, calls)
+<<<<<<< TREE
+=======
+
+    def test_done(self):
+        """run_suite should call result.done()"""
+        self.calls = 0
+        def one_more_call(): self.calls += 1
+        def test_function():
+            pass
+        test = unittest.FunctionTestCase(test_function)
+        class InstrumentedTestResult(tests.ExtendedTestResult):
+            def done(self): one_more_call()
+        class MyRunner(tests.TextTestRunner):
+            def run(self, test):
+                return InstrumentedTestResult(self.stream, self.descriptions,
+                                              self.verbosity)
+        tests.run_suite(test, runner_class=MyRunner, stream=StringIO())
+        self.assertEquals(1, self.calls)
+
+
+class TestRatchet(tests.TestCase):
+
+    def test_ok(self):
+        ratchet = tests.Ratchet(self.assertLength, 3)
+        ratchet.check([1, 2, 3])
+
+    def test_not_ok(self):
+        ratchet = tests.Ratchet(self.assertLength, 3)
+        self.assertRaises(AssertionError, ratchet.check, [1, 2])
+>>>>>>> MERGE-SOURCE