Bryan,
Thanks as usual for the incredibly detailed explanations--I don't know how long
it took you to write that email, but it was extremely helpful!
Regarding the test cases for DERBY-125 and DERBY-170, I'll need to spend some
time looking at those before I can come up with any suggestions. In the
meantime, here's some quick feedback based from my first time through the email...
1) Should this be one patch or several?
As others on the list have also replied, if it makes sense to split the patch up
into pieces, then that's definitely the preferred way to go. It sounds doable
w.r.t to DERBY-125 and DERBY-170, so I'd say go ahead and break those two parts
out, as Knut suggested.
2) Can we make the tests better? Yes, and I'll be working on this,
but I think I need more help from you, particularly regarding the
off-by-one problem in finalizeDSSLength, which I discuss at length
below. This is the problem child in the testing area, and I need
some good ideas for how to make the tests better.
I'll have to reply to this one later; need to investigate more...
3) Is the DERBY-170 test specific to the JCC client? Yes, this test is
specific to the JCC client, and, more specifically, it is specific to
the "deferPrepares" behavior of the JCC client.
[ snip ]
Do you know of a way to cause the skipRemainder() processing to be run
on a arbitrarily large object, other than the case mentioned in the bug
script?
Thanks for explaining this one. As for an alternative, I will again have to
think about this some more before I can make any suggestions.
4) What's going on with this off-by-one bug, and how can we write a test
which clearly demonstrates the bug? [ ... ] After you read this, hopefully
you will not only understand why the test that I added doesn't show the
bug, but also will be able to suggest how I could write a test that *does*
show the bug.
Yes, I can see now why the test that you added doesn't show the bug--this is a
great explanation of the problem, and I appreciate you taking the time to write
it up. I'll try to go through this sometime in the next day or two and will let
you know if anything comes to mind (I have a lot going on right now, so I can't
get to this immediately--but I'll try to do it soon).
<snip: excellent description of the off-by-one problem>
This makes me wonder whether it would be reasonable to modify
ensureLength() so that it pre-filled the buffer with non-zero data
(for example, 0xFC), so that the bug would be more likely to be
obvious, since it would end up placing 0xFC into the last byte of
the message, rather than 00 in most cases.
My gut reaction is that we probably don't want to do that--it seems like a bit
too much for the sake of testing a single Jira issue.
I don't think we'd always want ensureLength() to do this, for performance
reasons, but perhaps there is some way to conditionally compile such code
into the test version of DDMWriter?
There may be, but I think we should avoid doing different compiles for testing
vs. development environments. Any tests we run should occur against the
compiled codeline as it will be released, or else we end up testing code that is
not released, and not testing code that is released, which seems like a bad idea
to me.
However, I do believe that the bug is real, for two reasons:
I definitely agree that it's a bug--I didn't mean to call that into question. I
was just perplexed as to why it didn't manifest itself. You've now explained
that part to me, so the trick is to write an appropriate test case.
I'm afraid that by now I've gone on much too long.
Details are great--especialy when they're as well thought-out and clearly
written as you tend to give.
- after considering my explanation of the bug, do you have an idea of a
better way to write a test for it? If not, should I just remove the
DERBY-125 test from prepStmt.java?
Not yet; I'll have to think about this some more. Note that if you decide to
break your patch up into the separate issues, you could go ahead and submit
patches for DERBY-491 and DERBY-492 right now, and then submit the other patches
(DERBY-125 and DERBY-170) when the test cases have been finalized. And as for
DERBY-170, since there's at least one case where the test fails without the
patch--namely, against JCC--I think it'd be okay to commit that change, as well;
when a better test that shows the problem against the Derby client is available,
you could submit that test case as a follow-up patch.
- Can you try the experiment of applying all of my patches *except* for
the off-by-one bug (just apply everything, then remove the "+1" from
finalizeDSSLength), and then run
java -Dframework=DerbyNetClient
org.apache.derby.functionTests.harness.RunTest lang/procedure.java
My belief is that you will get an error in the client from the
parseFastVCMorVCS routine in NetStatementReply.java saying that
"only one of the VCM, VCS length can be greater than 0". That, if you
can get it, is proof of the off-by-one bug causing damage.
I will try this out and see what happens. In the meantime, thanks again for
being so thorough with all of this. I'll post again when I have further
feedback to give...
Army
