Code review comment for lp:~jameinel/bzr/2.0-490228-gc-segfault

Let me get this straight:

I approved your patch.

I think you did very well on that bug.

I'm reacting to the "no test sorry" part and trying to debug what
I consider a weakness in the way we practice TDD *in general*.

I don't *know* the answer, but I can recognize a concern here.

I think this patch is the right trade-off, but it is a trade-off
or you wouldn't have said "I don't have a good way to test this".

>>>>> "jam" == John A Meinel <email address hidden> writes:

    jam> Vincent Ladeuil wrote:
    >> Review: Approve

    >> I would be curious to know how long you and other people
    >> involved have spent on this bug and compare that to the
    >> time necessary to get a test infrastructure good enough to
    >> write a test reproducing this bug...
    >>

    jam> Once I had a traceback, and did a line-by-line analysis
    jam> of what was going on, maybe 20 min?

Yeah, *once* you had the traceback.

The time I was referring to was the one *including* the time you
spent to get there *AND* the time spent by *others* to get there.

    jam> Rewriting the code to be testable is at least a days
    jam> work for me, depending on how much I want to cheat.

Exactly, but I'm perfectly fine with cheating when testing !

The goal is to reduce the time spent to *reproduce* the bug. I
may sound like a test freak but what I really care about is to
minimize the time spent to fix bugs and it's crystal clear to me
that the most important part of that time is spent in
*reproducing* any given bug.

    jam> The main problems are:

    jam> 1) Data injection. I'd have to figure out how to
    jam> generate a structure that is ready to expose the
    jam> bug. Going further, how do you do it without generating
    jam> a brittle test?

Sometimes a brittle test is better than no test. I agree that
this is very hard to decide and that's why I try to focus on
building robust low-level code by doing TDD to the book: never
write code without a failing test. (I don't mean that *you* don't
by the way :)

And I insist on the "try", the pain threshold often makes me
resign when working on code that doesn't have a good enough
associated test infrastructure.

    jam> 2) Measuring the error.

<snip/>

    jam> However, accessing the 21st byte will not be a
    jam> segfault. You have to access the 8193rd byte to get the
    jam> OS to say "hey, you don't have access to that page, go
    jam> home."

Pain, pain :(

<snip/>

    jam> 3) Alternatively, we refactor the whole thing so that
    jam> memory access goes through a callback function.

Or use some independent object that could be tested independently
maybe ?

    jam> And the gc code is definitely one of the performance
    jam> sensitive areas of our codebase...

Make it work, make it right, make it fast.

Untested code is broken code.

I think we all agree on these principles *even* when we don't
find a good way to respect them.

I'm fine with violating them when circumstances or judgment tell
us to do so. What I'd like is that we document these violations
more carefully so that we at least know where we did.

<snip/>

    jam> If you have any hints as to a good way to test this, I'm all ears.

Without investing as much time (at least) as you did ? Certainly
not, you're the expert here and I respect that.

Again, I'm reacting to the "no test because..." as I'm sure
you're not happy with that and I want to support you during this
painful time ;-)

So, I don't have a specific answer.

The generic answer is to start small with the case at hand faking
anything that comes in the way, concentrating on the layering to
end with a simplified object where one simple test can reproduce
the bug.

If that means replacing the compression part with some trivial
implementation that don't compress but with a compatible API,
good. If that means replacing it with a trivial implementation
that fake the compression by providing pre-calculated streams,
good too. Anything, really.

At the minimum, put some comments telling: 'FIXME: How can I test
x or y ?'.

I don't care that much about perfect tests, I care more about
*identifying* where we don't test enough so that we can better
track where we don't get the benefits of TDD because we didn't
invest enough...

May be we should just file a bug saying, damn, the gc code is not
well tested see bug #490228.

May be we lack some rules to write more testable pyrex code, may
be we need to investigate using C function pointers that we can
override for tests (indirections like that are cheap in C, far
cheaper than in python).

Here I go, asking questions and not giving answers... Well at
least that may better explain my point :)

And in the end, I think my point is: sometimes we fail to write
good enough tests and we shouldn't put that under the carpet and
be done with it. If we lack good tests we will get bugs and we
will spend time reproducing them before being able to fix them,
so let's be honest and document that instead of enforcing the
meme that it's ok to not write tests.

    >>
    >> Do you believe that this bug is present upstream or that
    >> it was introduced during the adaptation to pyrex ?

    jam> It is a bug from our adaptation of the code. They don't
    jam> ever insert new records into the hash map. It has to do
    jam> with how groupcompress blocks work.

That's good news, kind of ;-)

       Vincent