Thanks for this very careful review, Oystein. Responses below.
�ystein Gr�vlen wrote:
"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?
The code for getRealConnection() checks the isClosed boolean and throws
an exception, so I assumed that meant I'd get a SQLException rather than
null. Note this matches the logic used throughout the rest of
BrokeredConnection. getRealConnection() acts as a wrapper so that the
rest of the code doesn't have to check for null everywhere.
- 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.
I didn't think it mattered so much in test code, because any exception
is considered a failure, but I can change this.
- 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?
Good point, thanks, I'll close them there as well.
- 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.)
I wanted to test both that they were not equal, and I wanted to make
sure the output format was correct. If the sed doesn't work, that means
the format is incorrect, and then I can look at the output to see what
went wrong. Just checking to make sure they weren't equal without
ensuring the format was right made me nervous. I suppose I was lazy in
that I could have written a routine that validated the format rather
than using the master output file mechanism to do the heavy lifting for me.
BTW, I created TestUtil.checkUniqueToString, I'm not reusing an existing
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.
I originally had the class name being printed out, but there were
objections from Dan and others that this was not necessary.
- 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?
I was actually checking for the most common bug, where you're reusing
the same id over and over again.
I think if you want to be picky, what you really want is to make sure
that any given string doesn't match any other string that has occurred
thus far. I should keep a hashtable of all connection strings obtained
and each time I test I make sure it's not in the existing hash table and
then add it. I can create this test.
- You do not have a test that tests that brokered connections get
the same id as the underlying physical connection.
OK. I have to think about that one a bit...
- Some of the lines are longer than 79 characters.
OK, I can fix that.
