Hi Andrew,
Thanks for reviewing this patch. Some comments follow:
Andrew McIntyre wrote:
Hi Rick,
High level comments concerning your patch for DERBY-516:
First, the requirements for the full set of compatibilty tests to run
are quite extensive. Perhaps a little more extensive than a sort-of-
casual derby user/developer might expect when trying to run a set of
tests. It is not sufficient to download the source tree and the
dependencies listed in building.txt. It requires that you have JDK
1.3, 1.4, 1.5 for your platform, and Derby jars from the 10.0 and
10.1 branches available in specific locations. I think additional
information should be added to the testing README.htm in java/testing
to include the full requirements for the compatibility test as well
as a link to the README.htm down in java/testing/.../compatibility.
At any rate, I think an effort should be made - for now - to make the
compilation and execution of these tests optional, or at the very
least, fail gracefully with informative error messages. I still
haven't been able to get just jdbcapi/CompatibilityTest to run, and
the current build/test setup doesn't give me much information to help
me figure out why.
The full compatibility test suite (all combinations) is an optional
test. Only one combination has been incorporated into derbyall. That
combination runs with the default vm against the client and server in
the trunk.
I confess I don't see the value in making compilation of this test class
optional given that we have already voted to require JUnit as part of
our build machinery. See http://wiki.apache.org/db-derby/VoteResults. I
believe you were the lone dissenter on this issue and you were kind
enough not to veto the proposal--you merely voted 0.
That said:
1 - For junit.jar - the instructions in BUILDING.txt are not clear as
to what you are supposed to do with junit.jar. "Unzip it somewhere"
is not nearly specific enough. The instructions should say what file
should be put into what location. This location should be added to
extrapath.properties and that location should be referenced in any
build files that require it, as is currently the case with osgi.jar,
for example. That location should probably be tools/java since that's
where we've been putting other dependencies.
I think that, according to a later posting, you found the relevant build
instructions.
In general, compilepath.properties is reserved for JDK-related
classes, jars containing java.*, javax.*, or direct dependencies. The
new property for junit.jar should probably be called ${junit}, added
to extrapath.properties, and the build.xml in java/testing/.../
functionTests/compatibility/build.xml should include ${junit} in the
appropriate classpath entries for the applicable <javac> tasks. Same
for java/testing/.../functionTests/util/build.xml and java/
testing/.../jdbcapi/build.xml
But I really think that new build targets that are conditional on $
{junit} being <available> should be made in the above build files, in
case someone wanted to compile the non-Junit tests but didn't have
Junit - for now.
I admit to being a bit muddled by this advice, given that we have voted
to make JUnit a mandatory part of our build machinery, just like ant. I
would appreciate a clarification.
2 - empty diff in java/build/org/apache/derbyBuild/build.xml
3 - the reference to junit-3.8.1.jar in java/testing/Readme.htm
should be made more generic, and reference the convention $jardir
which is already used in that file. '/local/derby/tools/java/
junit-3.8.1.jar' not only presupposes that the user will have put
junit.jar in tools/java (which is ok, we should probably instruct the
user to put junit.jar into tools/java and reference it in
extrapath.properties from there), but into a particular path in their
filesystem -- and that particular path does not apply to all OSs.
Thanks, I didn't catch this one. I'll correct this reference in README.htm.
4 - Junit noise(?) stripped in Sed.java. Is it noise? Wouldn't the
"OK..." output be useful to narrow down testcase failure?
The OK line is emitted only if the tests run cleanly. If a test fails,
it spews a diagnostic and a stack trace into the output file. This will
cause a failure-flagging diff under derbyall. All of the useful
information lives in the diagnostic and trace. I hope I'm not missing
the point of your comment.
5 - if you are going to add jdbcapi/CompatibilityTest to
derbynetmats.runall, then you should have a way for the current
harness to gracefully fail / skip the test if all the requirements of
CompatibilityTest are not met. As it is, it couldn't get jdbcapi/
CompatibilityTest to run, but it's likely that I missed something in
my setup. At any rate, I can help with sorting out any issues with
regards to getting the test to run as a part of the current test
harness.
In a later message you found that this test requires the presence of the
db2 client on the classpath. I will fix CompatibilityTest so that it
doesn't depend on db2jcc.
6 - Weird diffs in build.xml (top-level) and java/client/org/apache/
derby/client/net/Reply.java
Thanks. I don't understand these either. I'll scrub them.
7 - The default values in testScript.xml for the locations of the
JVMs are not applicable to Mac OS X (an itchy platform for me). I'll
follow up on this later.
If you can tell me what to use here, I'll make the changes.
Since I haven't been able to get the tests to run yet, that's all I
have for now, but I'll keep at it. :-)
andrew
