db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Van Couvering <David.Vancouver...@Sun.COM>
Subject Re: [jira] Updated: (DERBY-243) connection toString should uniquely identify the connection
Date Mon, 06 Jun 2005 17:09:58 GMT
Thanks for this very careful review, Oystein.  Responses below.

Øystein Grøvlen wrote:

>>>>>>"DVC" == David Van Couvering <David.Vancouvering@Sun.COM> 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.

> 

Mime
View raw message