I like your addition of a assertColumnTypes method to JDBC.java. And I really like your use of assertions. Your usage of JDBC.assertFullResultSet, assertCompileError, and other methods is a good reminder to me to check where I should be using more of what others have already defined.

Here are a couple things I think could be changed:

setUp() needs a stmt.close()

Two test methods create a SQL function, but don't drop it. I didn't encounter problems when I ran your suite (both client and embedded), but I wonder if that might create a problem in some context.

Take a pass through the code and verify that the javadoc comments match the code; I know how easy it is for them to get out of sync while developing.
For example, the javadoc comment for setUp() doesn't match what it does. You might consider simplifying the description to something like "Create and populate test tables.". That would reduce maintenance later if more tables get added, their names change, or more rows get inserted.

Jean T. Anderson
added a comment - 16/Mar/07 22:07 I took a look at your patch and have several comments.
I like your addition of a assertColumnTypes method to JDBC.java. And I really like your use of assertions. Your usage of JDBC.assertFullResultSet, assertCompileError, and other methods is a good reminder to me to check where I should be using more of what others have already defined.
Here are a couple things I think could be changed:
setUp() needs a stmt.close()
Two test methods create a SQL function, but don't drop it. I didn't encounter problems when I ran your suite (both client and embedded), but I wonder if that might create a problem in some context.
Take a pass through the code and verify that the javadoc comments match the code; I know how easy it is for them to get out of sync while developing.
For example, the javadoc comment for setUp() doesn't match what it does. You might consider simplifying the description to something like "Create and populate test tables.". That would reduce maintenance later if more tables get added, their names change, or more rows get inserted.

Manjula Kutty
added a comment - 16/Mar/07 22:49 Thanks for the comments, Jean. I'm attaching the new patch which will take care of the closing issue and drop functions. Please review and if looks fine please commit

Daniel John Debrunner
added a comment - 19/Mar/07 15:54 Patch doesn't compile for me,
[javac] C:_work\svn_clean4\trunk\java\testing\org\apache\derbyTesting\funct
ionTests\tests\lang\UnaryArithmeticParameterTest.java:262: assertColumnTypes(jav
a.sql.ResultSet,int[]) in org.apache.derbyTesting.junit.JDBC cannot be applied t
o (java.sql.PreparedStatement,int[])
[javac] JDBC.assertColumnTypes(ps,expectedTypes);
[javac] ^
Seems like some other changes are missing.
The comments don't indicate why this test requires JDBC 3? I couldn't see anything obvious that would stop this running on JSR169.
FYI - a single patch file is much more convienent, than a separate one for the change to the suite file.
For code like this:
+ Object[][] expectedRows = new Object[][] new String("-2"),new String("2")},{new String("-2"),new String("2") ;
+ JDBC.assertFullResultSet(ps.executeQuery(), expectedRows, true);
it can be cleaned up a couple of ways:
new String("-2") is kind of pointless, "-2" is a String object, just it can be used directly.
a utility assertFullResultSet method exists that takes strings, so the code could be:
+ String[][] expectedRows = new String[][] "-2", "2"},{-2","2" ;
+ JDBC.assertFullResultSet(ps.executeQuery(), expectedRows);
It looks like the suite method runs the fixtures twice in embedded, once adding them directly and once through defaultSuite

>>Seems like some other changes are missing.
Sorry that in the last patch I forgot to include the changes I made in the junit/JDBC.java

>>The comments don't indicate why this test requires JDBC 3? I couldn't see anything obvious that would stop this running on JSR169.
Added comments to to reason why we need JDBC3. Also if I do specify only about JDBC3, I thought the test will run in JSR 169 (I may be wrong here...Please clear me if I'm wrong).

>>FYI - a single patch file is much more convienent, than a separate one for the change to the suite file.
Sure I will be attaching the recent patch in a single set.

>>it can be cleaned up a couple of ways:
>> - new String("-2") is kind of pointless, "-2" is a String object, just it can be used directly.
>> - a utility assertFullResultSet method exists that takes strings, so the code could be:

>>It looks like the suite method runs the fixtures twice in embedded, once adding them directly and once through defaultSuite
I thought I give
return TestConfiguration.defaultSuite(UnaryArithmeticParameterTest.class);
The test will run both embedded and n/w server mode. (Please correct me if I'm wrong)

Manjula Kutty
added a comment - 19/Mar/07 19:34 >>Patch doesn't compile for me,
>> [javac] C:_work\svn_clean4\trunk\java\testing\org\apache\derbyTesting\funct
>>nTests\tests\lang\UnaryArithmeticParameterTest.java:262: assertColumnTypes(jav
>>a.sql.ResultSet,int[]) in org.apache.derbyTesting.junit.JDBC cannot be applied t
>>o (java.sql.PreparedStatement,int[])
>> [javac] JDBC.assertColumnTypes(ps,expectedTypes);
>> [javac] ^
>>Seems like some other changes are missing.
Sorry that in the last patch I forgot to include the changes I made in the junit/JDBC.java
>>The comments don't indicate why this test requires JDBC 3? I couldn't see anything obvious that would stop this running on JSR169.
Added comments to to reason why we need JDBC3. Also if I do specify only about JDBC3, I thought the test will run in JSR 169 (I may be wrong here...Please clear me if I'm wrong).
>>FYI - a single patch file is much more convienent, than a separate one for the change to the suite file.
Sure I will be attaching the recent patch in a single set.
>>For code like this:
>>+ Object[][] expectedRows = new Object[][] new String("-2"),new String("2")},{new String("-2"),new String("2") ;
>>+ JDBC.assertFullResultSet(ps.executeQuery(), expectedRows, true);
>>it can be cleaned up a couple of ways:
>> - new String("-2") is kind of pointless, "-2" is a String object, just it can be used directly.
>> - a utility assertFullResultSet method exists that takes strings, so the code could be:
>> + String[][] expectedRows = new String[][] "-2", "2"},{-2","2" ;
>>+ JDBC.assertFullResultSet(ps.executeQuery(), expectedRows);
Will be fxing this in the next patch
>>It looks like the suite method runs the fixtures twice in embedded, once adding them directly and once through defaultSuite
I thought I give
return TestConfiguration.defaultSuite(UnaryArithmeticParameterTest.class);
The test will run both embedded and n/w server mode. (Please correct me if I'm wrong)

Patch generally looks good but it's good to have clear & logical naming. The new utility method you've added JDBC.assertColumnTypes is not checking column types, it's checking parameter types. That's the danger of copying other code, as I see it see has comments in its javadoc related to DatabaseMetaData which is not relevant here.

And then I would be more explicit on the JDBC3 requirement, just stating the test requires ParameterMetaData I think would be clearer than "it relies on jdk14 jdbc metadata calls"

Daniel John Debrunner
added a comment - 20/Mar/07 00:02 Patch generally looks good but it's good to have clear & logical naming. The new utility method you've added JDBC.assertColumnTypes is not checking column types, it's checking parameter types. That's the danger of copying other code, as I see it see has comments in its javadoc related to DatabaseMetaData which is not relevant here.
And then I would be more explicit on the JDBC3 requirement, just stating the test requires ParameterMetaData I think would be clearer than "it relies on jdk14 jdbc metadata calls"

Jean T. Anderson
added a comment - 20/Mar/07 16:11 So I'm a little confused by the "Patch generally looks good but it's good to have clear & logical naming."
Is this patch ready to commit? If so, I'll go ahead and do so,
Or do you want the naming changed first? Would it be sufficient for me to change the JDBC.assertColumnTypes to JDBC.assertParameterTypes when it's committed?

Jean T. Anderson
added a comment - 20/Mar/07 17:26 Actually, I can't apply the latest patch ( DERBY-2458 _diff_03_19.txt), I get this error:
patching file java/testing/org/apache/derbyTesting/functionTests/tests/lang/_Suite.java
patch: **** malformed patch at line 377: Index: java/testing/org/apache/derbyTesting/junit/JDBC.java
I'm not spotting what the problem is in the patch file.
Manjula, I suggest that you sync up to the latest svn revision, take care of the remaining comments, and upload a final patch.

Manjula Kutty
added a comment - 20/Mar/07 19:17 Jean and Dan thanks for reviewing the patch. Your comments are really very helpful.
Attaching the new patch (hopefully the final one). I took care of most of Dan's comments.
Please review it and if everything looks good please commit

Almost there ... unfortunately DERBY-2458_diff_03_20.txt gets a Hunk failure at 77 – the problem is with _Suite.java. Please be sure to do an 'svn up'. For a tricky patch, especially if your svn working copy has a lot going on and your patch applies to just some of those changes, it might make sense to do a fresh checkout and apply your patch there to make sure it will work.

Jean T. Anderson
added a comment - 20/Mar/07 20:43 Almost there ... unfortunately DERBY-2458 _diff_03_20.txt gets a Hunk failure at 77 – the problem is with _Suite.java. Please be sure to do an 'svn up'. For a tricky patch, especially if your svn working copy has a lot going on and your patch applies to just some of those changes, it might make sense to do a fresh checkout and apply your patch there to make sure it will work.

Manjula Kutty
added a comment - 21/Mar/07 18:50 Thanks for the commit Jean. Here is the additional patch that will take care of the deletion on old test and update to the derbylang.runall file. Please review and if looks good please commit