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 13:13:23 GMT
Answering these by mail, not Jira comment, as it is not the best way to 
answer a lot of specific questions. Maybe I'll condense the discussion 
and add a Jira comment later.
Just to be clear, I do not primarily work on this issue. I just wanted 
to bring out comments to get things started, and it does seem people 
have some.

David Van Couvering (JIRA) wrote:
>     [ http://issues.apache.org/jira/browse/DERBY-919?page=comments#action_12370400 ]

>
> David Van Couvering commented on DERBY-919:
> -------------------------------------------
>
> I think it is great to have base unit test like this, although I agree with Andreas that
this should be renamed.  This class is almost solely about obtaining connections using different
frameworks, and is very JDBC-specific.  There are plenty of unit tests that have no need for
this functionality.
>   

Yes, indeed. But then we are *almost* back to simply extending TestCase. 
What tests not related to frameworks (and thus JDBC) need some kind of 
common functionality? Including JDBC in the name seems like a solution. 
Do we agree that non-JDBC tests should extend TestCase directly?

> I am not sure how this work integrates with/coincides with the work Andreas did to create
a junit test type which allows you to run 'raw' JUnit tests under the harness.  Can you explain?
>   

Andreas' work for running a "raw" JUnit test under the harness is not 
affected. This is all about getting a connection and some other basic 
functionality. It was written because the existing DerbyJUnitTest need 
additional methods calls before getConnection() returns a valid 
connection, and because TestUtil does not have a getConnection() (but 
several other getConnection(arguments) methods). We have several choices:
* Use TestUtil, maybe do some additional work on it.
* Adapt/change DerbyJUnitTest (dependencies restrict what we can change 
of existing API/behavior)
* Write a new common class from scratch

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?

> Here are my general comments...
>
> Javadoc:
> - getConnection() - it would be great to say it connects to the default database, instead
of "some database", which is a little disconcerting as it sounds somewhat non-deterministic.
>   

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

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

> - when I try to click on a method name, I get a "404" error, although I can scroll directly
down to view the method details.  I suspect this is probably just a JIRA bug.
>   

This is because I just copied a single Javadoc file and attached it. 
Jira is not to blame here. I did not find it necessary to upload all 
results generated when running javadoc.

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

Also, it seems the harness uses all of "hostName", 
"derbyTesting.serverhost" and "derbyTesting.clienthost". Can anyone shed 
some light on this?

> - There are a lot of defaults being setup in a hardcoded fashion in resetState().  It
would be better to have a section of static finals at the top with all the default values
so that someone looking at this code can tell right away what they are.  Actually, looking
at Andreas' TestConfiguration, that is a nice way of doing it . Having it as a separate class
also seems to be useful and more coherent.
>   

One note here, is that it would not be possible to change the framework 
with the current TestConfiguration. This would cause trouble for 
exceptional cases (as the current JDBC4 testsuite) and if we want to run 
useprocess=false and switch framework. Is this switching of framework 
something we don't need?

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

    /**
     * Set property used to determine debug mode.
     * Set property to <code>true</code> to enable debug mode.
     * Debug mode can only be enabled/disabled on JVM startup.
     */
    private static final String  DEBUG_PROPERTY     = "derby.tests.debug";
    private static final boolean DEBUG              = new Boolean(
                    PropertyUtil.getSystemProperty(DEBUG_PROPERTY,
                                                   "false")).booleanValue();


> - Your javadoc should be a little more explicit about how the default framework gets
set
>   

Noted. For the record, it reads system property "framework".

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

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



--
Kristian

> Thanks,
>
> David
>
>   
>   
>> improve pattern for setting up junit tests
>> ------------------------------------------
>>
>>          Key: DERBY-919
>>          URL: http://issues.apache.org/jira/browse/DERBY-919
>>      Project: Derby
>>         Type: Sub-task
>>   Components: Test
>>  Environment: All
>>     Reporter: Andreas Korneliussen
>>  Attachments: BasicDerbyJUnitTest.html, BasicDerbyJUnitTest.java, BasicDerbyJUnitTestTest.java,
JDBCClient.java, TestConfiguration.java
>>
>> The current junit tests cannot be run directly from the java.ui.textrunner by i.e
using:
>> java junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.junitTests.lang.BooleanTest
>> .E
>> Time: 0.008
>> There was 1 error:
>> 1) testBoolean(org.apache.derbyTesting.functionTests.tests.junitTests.lang.BooleanTest)java.lang.NullPointerException
>>         at org.apache.derbyTesting.functionTests.util.DerbyJUnitTest.faultInDriver(DerbyJUnitTest.java:317)
>>         at org.apache.derbyTesting.functionTests.util.DerbyJUnitTest.getConnection(DerbyJUnitTest.java:345)
>>         at org.apache.derbyTesting.functionTests.util.DerbyJUnitTest.getConnection(DerbyJUnitTest.java:335)
>>         at org.apache.derbyTesting.functionTests.tests.junitTests.lang.BooleanTest.testBoolean(BooleanTest.java:136)
>>         at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
>>         at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
>>         at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
>> FAILURES!!!
>> Tests run: 1,  Failures: 0,  Errors: 1
>> The reason is that the tests needs to have some fixture being set up before the test
can run, and that this is currently supported by calling a bunch of static methods in the
correct order to initialize some static members of DerbyJUnitTest.
>> The proposed alternative is that the added fixture is set up in the suite() method,
which is used by JUnit to get the Test object to be run.
>>     
>
>   


Mime
View raw message