Knut Anders Hatlen wrote:
Bryan Pendleton updated DERBY-491:
----------------------------------
Attachment: svn_jan_24_2006.diff
[ snip ]
I read through your patch, ran derbynetclientmats and derbynetmats
successfully, and verified that the test case failed without your
patch (actually, it didn't fail, it just hung until I got bored and
killed the test).
I too looked at the changes and they seem correct and well-documented. I also
ran the new tests with and without the patch applied: with the patch, they
passed, without, the tests hung, so that's good.
My one nit-pick, though, is that it looks _both_ test cases result in a hang if
the patch hasn't been applied. That in itself isn't a bad thing, but based on
the comments in the test and the error description for DERBY-491, I was
expecting to see a protocol exception for the first test case (DERBY-491), not a
hang.
My guess is that the fix for DERBY-125 and/or DERBY-170 have changed the
symptoms of DERBY-491 from "protocol exception" to "hang"--and if that's the
case, I have no complaints with the tests per se. I do, however, think it's
slightly non-intuitive to have comments in the test saying that the regression
would be a protocol exception, when in fact, given the current state of the
codeline, the regression would actually be a hang.
Of course, I'll admit that I'm perhaps being too picky. You've identified the
problem, written a fix, documented it very well, and have written test cases
that fail without your fix and pass with it--so as far as all of that goes, I
vote a definite +1 for commit.
That said, if you wouldn't mind updating the comments in the test and/or the
Jira entry to match the current 491 regression behavior (or perhaps to mention
that the regression for DERBY-491 could be _either_ a protocol exception or a
hang, depending on the situation), I think that'd be useful. Not obligatory,
but useful...
Thanks for your patience with my picking,
Army
