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

Proposed by Robert Collins on 2009-08-10
Status: Rejected
Rejected by: Robert Collins on 2009-08-31
Proposed branch: lp:~lifeless/bzr/hpss_ratches
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 276 lines (has conflicts)
Text conflict in bzrlib/tests/test_selftest.py
To merge this branch: bzr merge lp:~lifeless/bzr/hpss_ratches
Reviewer Review Type Date Requested Status
Martin Pool 2009-08-10 Disapprove on 2009-08-10
Review via email: mp+9905@code.launchpad.net
To post a comment you must log in.
Robert Collins (lifeless) wrote :

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) wrote :

> 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) wrote :

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) 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.

>> 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) wrote :

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) wrote :

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-08-28 22:24:40 +0000
3+++ NEWS 2009-08-29 03:36:19 +0000
4@@ -532,6 +532,10 @@
5 Testing
6 *******
7
8+* HPSS ratchets are now centralised in the ``setup_smart_server_with_call_log``
9+ helper method, removing the need to duplicate the warning about changing
10+ them throughout the code base. (Robert Collins)
11+
12 * Merge directive cherrypick tests must use the same root id.
13 (Martin Pool, #409684)
14
15
16=== modified file 'bzrlib/tests/__init__.py'
17--- bzrlib/tests/__init__.py 2009-08-28 21:05:31 +0000
18+++ bzrlib/tests/__init__.py 2009-08-29 03:36:19 +0000
19@@ -2298,7 +2298,12 @@
20 self.reduceLockdirTimeout()
21
22 def setup_smart_server_with_call_log(self):
23- """Sets up a smart server as the transport server with a call log."""
24+ """Sets up a smart server as the transport server with a call log.
25+
26+ This is typically used for hpss regression tests, and in addition to
27+ setting up the smart server a collection of hpss ratchets are created
28+ at self.hpss_ratchets.
29+ """
30 self.transport_server = server.SmartTCPServer_for_testing
31 self.hpss_calls = []
32 import traceback
33@@ -2310,6 +2315,23 @@
34 CapturedCall(params, prefix_length))
35 client._SmartClient.hooks.install_named_hook(
36 'call', capture_hpss_call, None)
37+ # These figures represent the amount of work to perform each use case.
38+ # It is entirely ok to reduce a figure if a test fails due to rpc_count
39+ # being too low. If the rpc_count increases, more network roundtrips
40+ # have become necessary for this use case. Please do not adjust these
41+ # figures upwards without agreement from bzr's network support
42+ # maintainers.
43+ self.hpss_ratchets = {
44+ 'push_smart_non_stacked_streaming': Ratchet(self.assertLength, 9),
45+ 'push_smart_stacked_streaming': Ratchet(self.assertLength, 14),
46+ 'push_smart_tags_streaming': Ratchet(self.assertLength, 11),
47+ 'init_on_format': Ratchet(self.assertLength, 2),
48+ 'branch_from_trivial_same_server': Ratchet(self.assertLength, 38),
49+ 'branch_from_trivial': Ratchet(self.assertLength, 10),
50+ 'branch_from_trivial_stacked': Ratchet(self.assertLength, 15),
51+ 'pull_smart_stacked': Ratchet(self.assertLength, 18),
52+ 'init_repo': Ratchet(self.assertLength, 16),
53+ }
54
55 def reset_smart_call_log(self):
56 self.hpss_calls = []
57@@ -4025,6 +4047,35 @@
58 return None
59
60
61+class Ratchet(object):
62+ """A Ratchet separates an assertion from code that needs to check it.
63+
64+ This is useful to allow checking ratchet style assertions - such as that
65+ no more than N round trips are made during an operation, without having
66+ the expected values spread out over the code base where the can be changed
67+ without reading documentation about them.
68+
69+ A typical use is to prepare a Ratchet in setUp, and then test it later.
70+ """
71+
72+ def __init__(self, assertion, *args):
73+ """Create a ratchet.
74+
75+ :param assertion: The assertion to invoke when the ratchet is checked.
76+ :param *args: positional arguments to pass to the assertion.
77+ """
78+ self.assertion = assertion
79+ self.args = args
80+
81+ def check(self, *args):
82+ """Check the ratchet.
83+
84+ :param args: Additional arguments to pass to the ratchet. Typically the
85+ expected value of the underlying assertion.
86+ """
87+ return self.assertion(*(self.args + args))
88+
89+
90 class _HTTPSServerFeature(Feature):
91 """Some tests want an https Server, check if one is available.
92
93
94=== modified file 'bzrlib/tests/blackbox/test_branch.py'
95--- bzrlib/tests/blackbox/test_branch.py 2009-08-20 12:30:49 +0000
96+++ bzrlib/tests/blackbox/test_branch.py 2009-08-29 03:36:20 +0000
97@@ -353,12 +353,7 @@
98 self.reset_smart_call_log()
99 out, err = self.run_bzr(['branch', self.get_url('from'),
100 self.get_url('target')])
101- # This figure represent the amount of work to perform this use case. It
102- # is entirely ok to reduce this number if a test fails due to rpc_count
103- # being too low. If rpc_count increases, more network roundtrips have
104- # become necessary for this use case. Please do not adjust this number
105- # upwards without agreement from bzr's network support maintainers.
106- self.assertLength(38, self.hpss_calls)
107+ self.hpss_ratchets['branch_from_trivial_same_server'].check(self.hpss_calls)
108
109 def test_branch_from_trivial_branch_streaming_acceptance(self):
110 self.setup_smart_server_with_call_log()
111@@ -368,12 +363,7 @@
112 self.reset_smart_call_log()
113 out, err = self.run_bzr(['branch', self.get_url('from'),
114 'local-target'])
115- # This figure represent the amount of work to perform this use case. It
116- # is entirely ok to reduce this number if a test fails due to rpc_count
117- # being too low. If rpc_count increases, more network roundtrips have
118- # become necessary for this use case. Please do not adjust this number
119- # upwards without agreement from bzr's network support maintainers.
120- self.assertLength(10, self.hpss_calls)
121+ self.hpss_ratchets['branch_from_trivial'].check(self.hpss_calls)
122
123 def test_branch_from_trivial_stacked_branch_streaming_acceptance(self):
124 self.setup_smart_server_with_call_log()
125@@ -388,12 +378,7 @@
126 self.reset_smart_call_log()
127 out, err = self.run_bzr(['branch', self.get_url('feature'),
128 'local-target'])
129- # This figure represent the amount of work to perform this use case. It
130- # is entirely ok to reduce this number if a test fails due to rpc_count
131- # being too low. If rpc_count increases, more network roundtrips have
132- # become necessary for this use case. Please do not adjust this number
133- # upwards without agreement from bzr's network support maintainers.
134- self.assertLength(15, self.hpss_calls)
135+ self.hpss_ratchets['branch_from_trivial_stacked'].check(self.hpss_calls)
136
137
138 class TestRemoteBranch(TestCaseWithSFTPServer):
139
140=== modified file 'bzrlib/tests/blackbox/test_pull.py'
141--- bzrlib/tests/blackbox/test_pull.py 2009-06-15 06:47:14 +0000
142+++ bzrlib/tests/blackbox/test_pull.py 2009-08-29 03:36:20 +0000
143@@ -382,12 +382,7 @@
144 self.reset_smart_call_log()
145 self.run_bzr(['pull', '-r', '1', self.get_url('stacked')],
146 working_dir='empty')
147- # This figure represent the amount of work to perform this use case. It
148- # is entirely ok to reduce this number if a test fails due to rpc_count
149- # being too low. If rpc_count increases, more network roundtrips have
150- # become necessary for this use case. Please do not adjust this number
151- # upwards without agreement from bzr's network support maintainers.
152- self.assertLength(18, self.hpss_calls)
153+ self.hpss_ratchets['pull_smart_stacked'].check(self.hpss_calls)
154 remote = Branch.open('stacked')
155 self.assertEndsWith(remote.get_stacked_on_url(), '/parent')
156
157
158=== modified file 'bzrlib/tests/blackbox/test_push.py'
159--- bzrlib/tests/blackbox/test_push.py 2009-08-27 22:17:35 +0000
160+++ bzrlib/tests/blackbox/test_push.py 2009-08-29 03:36:20 +0000
161@@ -214,12 +214,8 @@
162 t.commit(allow_pointless=True, message='first commit')
163 self.reset_smart_call_log()
164 self.run_bzr(['push', self.get_url('to-one')], working_dir='from')
165- # This figure represent the amount of work to perform this use case. It
166- # is entirely ok to reduce this number if a test fails due to rpc_count
167- # being too low. If rpc_count increases, more network roundtrips have
168- # become necessary for this use case. Please do not adjust this number
169- # upwards without agreement from bzr's network support maintainers.
170- self.assertLength(9, self.hpss_calls)
171+ self.hpss_ratchets['push_smart_non_stacked_streaming'].check(
172+ self.hpss_calls)
173
174 def test_push_smart_stacked_streaming_acceptance(self):
175 self.setup_smart_server_with_call_log()
176@@ -230,12 +226,8 @@
177 self.reset_smart_call_log()
178 self.run_bzr(['push', '--stacked', '--stacked-on', '../parent',
179 self.get_url('public')], working_dir='local')
180- # This figure represent the amount of work to perform this use case. It
181- # is entirely ok to reduce this number if a test fails due to rpc_count
182- # being too low. If rpc_count increases, more network roundtrips have
183- # become necessary for this use case. Please do not adjust this number
184- # upwards without agreement from bzr's network support maintainers.
185- self.assertLength(14, self.hpss_calls)
186+ self.hpss_ratchets['push_smart_stacked_streaming'].check(
187+ self.hpss_calls)
188 remote = branch.Branch.open('public')
189 self.assertEndsWith(remote.get_stacked_on_url(), '/parent')
190
191@@ -246,12 +238,7 @@
192 t.branch.tags.set_tag('new-tag', rev_id)
193 self.reset_smart_call_log()
194 self.run_bzr(['push', self.get_url('to-one')], working_dir='from')
195- # This figure represent the amount of work to perform this use case. It
196- # is entirely ok to reduce this number if a test fails due to rpc_count
197- # being too low. If rpc_count increases, more network roundtrips have
198- # become necessary for this use case. Please do not adjust this number
199- # upwards without agreement from bzr's network support maintainers.
200- self.assertLength(11, self.hpss_calls)
201+ self.hpss_ratchets['push_smart_tags_streaming'].check(self.hpss_calls)
202
203 def test_push_smart_with_default_stacking_url_path_segment(self):
204 # If the default stacked-on location is a path element then branches
205
206=== modified file 'bzrlib/tests/blackbox/test_shared_repository.py'
207--- bzrlib/tests/blackbox/test_shared_repository.py 2009-08-12 23:52:15 +0000
208+++ bzrlib/tests/blackbox/test_shared_repository.py 2009-08-29 03:36:20 +0000
209@@ -114,9 +114,4 @@
210 # be fixed.
211 self.setup_smart_server_with_call_log()
212 self.run_bzr(['init-repo', self.get_url('repo')])
213- # This figure represent the amount of work to perform this use case. It
214- # is entirely ok to reduce this number if a test fails due to rpc_count
215- # being too low. If rpc_count increases, more network roundtrips have
216- # become necessary for this use case. Please do not adjust this number
217- # upwards without agreement from bzr's network support maintainers.
218- self.assertLength(16, self.hpss_calls)
219+ self.hpss_ratchets['init_repo'].check(self.hpss_calls)
220
221=== modified file 'bzrlib/tests/test_bzrdir.py'
222--- bzrlib/tests/test_bzrdir.py 2009-07-10 07:14:02 +0000
223+++ bzrlib/tests/test_bzrdir.py 2009-08-29 03:36:19 +0000
224@@ -906,13 +906,7 @@
225 self.reset_smart_call_log()
226 instance = new_format.initialize_on_transport(transport)
227 self.assertIsInstance(instance, remote.RemoteBzrDir)
228- rpc_count = len(self.hpss_calls)
229- # This figure represent the amount of work to perform this use case. It
230- # is entirely ok to reduce this number if a test fails due to rpc_count
231- # being too low. If rpc_count increases, more network roundtrips have
232- # become necessary for this use case. Please do not adjust this number
233- # upwards without agreement from bzr's network support maintainers.
234- self.assertEqual(2, rpc_count)
235+ self.hpss_ratchets['init_on_format'].check(self.hpss_calls)
236
237
238 class TestFormat5(TestCaseWithTransport):
239
240=== modified file 'bzrlib/tests/test_selftest.py'
241--- bzrlib/tests/test_selftest.py 2009-08-26 23:25:28 +0000
242+++ bzrlib/tests/test_selftest.py 2009-08-29 03:36:20 +0000
243@@ -2965,3 +2965,33 @@
244 self.verbosity)
245 tests.run_suite(suite, runner_class=MyRunner, stream=StringIO())
246 self.assertLength(1, calls)
247+<<<<<<< TREE
248+=======
249+
250+ def test_done(self):
251+ """run_suite should call result.done()"""
252+ self.calls = 0
253+ def one_more_call(): self.calls += 1
254+ def test_function():
255+ pass
256+ test = unittest.FunctionTestCase(test_function)
257+ class InstrumentedTestResult(tests.ExtendedTestResult):
258+ def done(self): one_more_call()
259+ class MyRunner(tests.TextTestRunner):
260+ def run(self, test):
261+ return InstrumentedTestResult(self.stream, self.descriptions,
262+ self.verbosity)
263+ tests.run_suite(test, runner_class=MyRunner, stream=StringIO())
264+ self.assertEquals(1, self.calls)
265+
266+
267+class TestRatchet(tests.TestCase):
268+
269+ def test_ok(self):
270+ ratchet = tests.Ratchet(self.assertLength, 3)
271+ ratchet.check([1, 2, 3])
272+
273+ def test_not_ok(self):
274+ ratchet = tests.Ratchet(self.assertLength, 3)
275+ self.assertRaises(AssertionError, ratchet.check, [1, 2])
276+>>>>>>> MERGE-SOURCE