Code review comment for lp:~lifeless/bzr/hpss_ratches

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