db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David W. Van Couvering" <David.Vancouver...@Sun.COM>
Subject Re: [jira] Commented: (DERBY-919) improve pattern for setting up junit tests
Date Wed, 15 Mar 2006 18:42:10 GMT


Kristian Waagan wrote:
> 
> So far most of the comments I have received have been regarding 
> implementation, which was not my primary goal. Do we all agree what we 
> need, but we want to do it in different ways? Or are there still someone 
> out there that have more fundamental issues to comment on?

I agree with Andreas: have a base generic class and a base class for JDBC

> 
> 
>    /**
>     * Get a connection to some database.
>     * If the database does not exist, it will be created.
>     * As long as the state of this class is not changed, this method will
>     * return a connection to the same database. Most suites will require 
> that
>     * this condition is true.
>     */
> 
> My "as long as state of this class is not changed" was supposed to 
> indicate that a connection to the same database was returned. "some" was 
> chosen because a test should not generally care what the database it 
> uses is named - as long as it is the same database for a given time 
> (typically while running a single test method or a suite). This is a 
> matter of wording.

OK.  It's just not clear to me what "some database" is, and it took some 
work for me to track it down.

>> - You should document what the defaults are, such as "wombat" for the 
>> database name and "APP" for username and password.
>>   
> 
> 
> I did not do this, because most tests should not care.

Why do you say that?  I'd like to know what's happening by default 
before I use the default behavior of an API.

> 
>> - why can't setup() or teardown() automaticlly call resetState() 
>> rather than asking the user to do it?  This seems dangerous and highly 
>> error-prone.
>>   
> 
> 
> When I wrote the class, I was looking at the JDBC4 test suite. It is 
> configured to always run under DerbyNetClient, but it "internally" runs 
> tests for embedded as well. BasicDerbyJUnitTest was written with the 
> following paradigm in mind: do setup (basically set framework, maybe 
> change default name and force use of datasource), then use 
> getConnection() for all tests in suite. The setup would be done in a 
> TestSetup decorator. With this approach, the test code can be exactly 
> the same for multiple frameworks and/or setups, where as the test 
> decorator would do different things for each suite. Calling resetState 
> in TestCase.setUp() would delete the state setup by 
> TestDecorator.setUp(). If this way to run tests are not needed, things 
> can be rewritten (in many ways...).
>

OK, thanks for clarifying.  It would be good to provide this discussion 
in the Javadoc so users know why they have to jump through this 
particular hoop.  It also explains better the intent for this class.

By the way, wouldn't separating configuration out from the base class 
make this management of state go away?  You can pass in a non-default 
configuration object and a non-default JDBCClient if you want to use 
non-default behavior.

>>
>> - I don't understand why you set a property for the database name 
>> before calling obtainConnection() rather than just passing in the 
>> database name to obtainConnection().  It would be good to at least 
>> explain the motivation behind this, as at first read it seems a bit 
>> odd.  Scanning at the code, it looks like you use db(P_DBNAME) in 
>> createBaseDatabaseUrl() where you could just as easily pass the 
>> database name in as a parameter
>>   
> 
> 
> All properties can be hardcoded and represented by constants instead of 
> using a Properties object. I just wanted to signal that results are 
> based on the state of the class, and that P_* are not set by environment 
> properties, but that S_* are (passed on from the test harness). Andreas' 
> TestConfiguration would separate this information from the class, which 
> is good. I do think it is a bit too rigid as it is to give the required 
> flexibility (not possible to change anything), but this is of course 
> easily changed. It also give me the impression that every configurable 
> aspect is passed on from the test harness, which is not yet true.

Again, I think this whole thing goes away if you use the 
TestConfiguratio model.  Right?

> 
>> - You have a lot of useful debug statements, but your javadoc doesn't 
>> really explain how to turn debug on and off, nor is it clear from the 
>> class (e.g. there is not "debug(boolean)" method).
>>   
> 
> 
> Would mentioning this in the public class documentation do the trick?

Yes, that would help.
> 
>> - It would be nice to have a helper method that allows subclasses to 
>> quickly obtain other properties set by the harness (e.g. so they don't 
>> have to do a privileged block every time they want to get a system 
>> property)
>>   
> 
> 
> Are you thinking of a getSystemProperty(key) with a privileged block, or 
> a method accessing a "cached" system properties object?

The former if system properties are not read-only, otherwise the latter 
is fine with me.  Since accessing system properties directly requires 
jumping through security hoops, I thought it would be good to isolate it.

Also, it would be very good to abstract out configuration implementation 
details so that if we change the way tests are configured all the tests 
don't have to change.  Another motivation for the TestConfiguration class...

> 
>> - I like Andreas' typesafe JDBCClient class, it seems a better approach.
>>   
> 
> 
> Yes. It should be extended to have information about DataSource classes 
> also, or else it would not be sufficent in a JSR 169 (like) environment. 
> Some complications here if we want to allow for more than one type of 
> DataSource for a given framework (XA, Pooled, Simple, "normal").

OK


Mime
View raw message