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
