>>>>> "DVC" == David Van Couvering <[EMAIL PROTECTED]> writes:
DVC> Hi, Kathey. Nobody has tested this patch. I think it's ready to go,
DVC> but the last patch I did with a full derbyall that then resulted in a
DVC> bunch of failures for Dan indicates we really need somebody else to
DVC> review and test it. So far no volunteers.
I have looked at the patch and it basically looks good. I have a few
comments/questions:
- BrokeredConnection.toString(): Are you guranteed that
getRealConnection never returns null?
- checkDataSource.checkNesConn(): Why have you changed it to
"throws Exception" instead of SQLException? This disables the
whole idea of advertising checked exceptions. (Same class as
"import java.*"). I also see that your new methods advertises
that they "throws Exceptions". I think you should be more
specific.
- While in the checkDataSource test you close the extra
connections you create, the dataSourcePermissions test does not
do so? Why is this less necessary in this test?
- Why do you want to write something in
TestUtil.checkUniqueToString? It just creates a lot of hazzle
to have to sed this from the tests. If I were to write a test
that needed to check that strings were not equal, I think I
would make my on method instead of reusing this method in order
not to have to worry about "sedding". (I also think
...NotEqual.. is a more descriptive name than ...Unique... for
this method.)
- Is it OK for the toString method to not write the class name? I
have not investigated how this is normally done in Derby, but I
think many would expect toString() to print the type of an
object.
- The tests now test that the Ids of two connections are not
equal. However, for all possible bugs giving a non-unique id, I
would think that only a very few would give equal ids to two
connections opened in sequence. Would it not be better to check
against all currently open connections?
- You do not have a test that tests that brokered connections get
the same id as the underlying physical connection.
- Some of the lines are longer than 79 characters.
--
�ystein
