db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew McIntyre <mcintyr...@gmail.com>
Subject DERBY-516 patch review, pt. 1
Date Sat, 22 Oct 2005 07:37:57 GMT
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.

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.

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.

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.

4 - Junit noise(?) stripped in Sed.java. Is it noise? Wouldn't the  
"OK..." output be useful to narrow down testcase failure?

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.

6 - Weird diffs in build.xml (top-level) and java/client/org/apache/ 
derby/client/net/Reply.java

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.

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

Mime
View raw message