db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rick Hillegas <Richard.Hille...@Sun.COM>
Subject Re: DERBY-516 patch review, pt. 1
Date Mon, 24 Oct 2005 20:16:49 GMT
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 

> 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

View raw message