db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kristian Waagan <Kristian.Waa...@Sun.COM>
Subject Re: [jira] Commented: (DERBY-919) improve pattern for setting up junit tests
Date Wed, 15 Mar 2006 21:26:20 GMT
See answers inlined.

David W. Van Couvering wrote:

>
>
> 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.

I see your point, but the knowledge you want to obtain is actually not 
*required* to write the tests. Whether you get a connection to database 
"wombat", "dog", "x" or something else will normally not affect your 
test code when testing the JDBC API. The same goes with username and 
password (as long as they are valid of course). A test should be 
self-contained and must itself take responsibility for ensuring its 
requirements are met - for instance populating the database or disabling 
autocommit.  Tests that need to know the name of the database should set 
this explicitly in the getConnection call, the same goes with username 
and password.

For instance when testing encryption of the database, different security 
mechanisms or backup/restore of a database, the no argument 
getConnection method would not be good enough. In my mind, if we 
suddenly decide to change the default database name from "wombat" to 
"holycow", no tests should fail. If a test fails because of this, it has 
put to much trust into the hands of getConnection() and should use 
getConnection(dbName) or a similar call where it specifies what it needs 
to know.

I still agree with you that the defaults should be documented though :)

>
>>
>>> - 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...

This could have been changed directly in BasicDerbyJUnitTest as well, 
but I agree the TestConfiguration class is a far better solution.

>
>>
>>> - 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
>
Thanks for all the comments.
I have decided to discard BasicDerbyJUnit test entirely, it has served 
its purpose.
I hope to produce a very simple BaseJDBCTestCase and a minimal 
BaseTestCase shortly. I would appreciate if they could be commented 
quickly and commited as soon as they are acceptable.



--
Kristian

Mime
View raw message