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.
