You are viewing a plain text version of this content. The canonical link for it is here.
Posted to derby-dev@db.apache.org by Kristian Waagan <Kr...@Sun.COM> on 2007/05/28 00:01:56 UTC
Re: svn commit: r542016 - in
/db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4: BlobTest.java
CallableStatementTest.java ClobTest.java DataSourceTest.java
PreparedStatementTest.java ResultSetTest.java TestJDBC40Exception.java
kahatlen@apache.org wrote:
> Author: kahatlen
> Date: Sun May 27 11:30:45 2007
> New Revision: 542016
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=542016
> Log:
> DERBY-2707: Inadequate clean-up in many jdbc4 tests
>
> Fixed the tearDown() methods in many of the jdbc4 tests.
>
> Modified:
> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/BlobTest.java
> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/CallableStatementTest.java
> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ClobTest.java
> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/DataSourceTest.java
> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/PreparedStatementTest.java
> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ResultSetTest.java
> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/TestJDBC40Exception.java
>
> Modified: db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/BlobTest.java
> URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/BlobTest.java?view=diff&rev=542016&r1=542015&r2=542016
> ==============================================================================
> --- db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/BlobTest.java (original)
> +++ db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/BlobTest.java Sun May 27 11:30:45 2007
> @@ -125,7 +125,7 @@
> //throwing a SQLException when they are called after calling free()
> //on a LOB.
>
> - private ExemptBlobMD [] emd = new ExemptBlobMD [] {
> + private static final ExemptBlobMD [] emd = new ExemptBlobMD [] {
> new ExemptBlobMD( "getBinaryStream", new Class[] { long.class,long.class }
> ,true,true ),
> new ExemptBlobMD( "setBinaryStream", new Class[] { long.class },false,true ),
> @@ -167,6 +167,13 @@
> //from throwing a SQLException after free has been called
> //on the Clob object.
> buildHashSet();
> + }
> +
> + protected void tearDown() throws Exception {
> + blob.free();
> + blob = null;
> + excludedMethodSet = null;
> + super.tearDown();
> }
>
> /**
>
> Modified: db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/CallableStatementTest.java
> URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/CallableStatementTest.java?view=diff&rev=542016&r1=542015&r2=542016
> ==============================================================================
> --- db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/CallableStatementTest.java (original)
> +++ db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/CallableStatementTest.java Sun May 27 11:30:45 2007
> @@ -71,9 +71,9 @@
> */
> protected void tearDown()
> throws Exception {
> - if (cStmt != null && !cStmt.isClosed()) {
> - cStmt.close();
> - }
> +
> + cStmt.close();
> + cStmt = null;
>
Just out of curiosity:
At first look the existing code looks sound - calling close only if the
statement is not null and not already closed.
Why is that inadequate?
--
Kristian
>
> super.tearDown();
> }
> @@ -83,7 +83,6 @@
> DatabaseMetaData met = getConnection().getMetaData();
> assertFalse("Named parameters are not supported, but the metadata " +
> "says they are", met.supportsNamedParameters());
> - met = null;
> }
>
> public void testGetDoubleIntOnInParameter()
>
> Modified: db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ClobTest.java
> URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ClobTest.java?view=diff&rev=542016&r1=542015&r2=542016
> ==============================================================================
> --- db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ClobTest.java (original)
> +++ db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ClobTest.java Sun May 27 11:30:45 2007
> @@ -126,7 +126,7 @@
> //throwing a SQLException when they are called after calling free()
> //on a LOB.
>
> - private ExemptClobMD [] emd = new ExemptClobMD [] {
> + private static final ExemptClobMD [] emd = new ExemptClobMD [] {
> new ExemptClobMD( "getCharacterStream", new Class[] { long.class, long.class } ,true,true),
> new ExemptClobMD( "setAsciiStream", new Class[] { long.class } ,false,true),
> new ExemptClobMD( "setCharacterStream", new Class[] { long.class } ,true,true),
> @@ -161,6 +161,13 @@
> //from throwing a SQLException after free has been called
> //on the Clob object.
> buildHashSet();
> + }
> +
> + protected void tearDown() throws Exception {
> + clob.free();
> + clob = null;
> + excludedMethodSet = null;
> + super.tearDown();
> }
>
> /**
>
> Modified: db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/DataSourceTest.java
> URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/DataSourceTest.java?view=diff&rev=542016&r1=542015&r2=542016
> ==============================================================================
> --- db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/DataSourceTest.java (original)
> +++ db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/DataSourceTest.java Sun May 27 11:30:45 2007
> @@ -62,8 +62,9 @@
> * Initialize the ds to null once the tests that need to be run have been
> * run
> */
> - public void tearDown() {
> + public void tearDown() throws Exception {
> ds = null;
> + super.tearDown();
> }
>
> public void testIsWrapperForDataSource() throws SQLException {
>
> Modified: db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/PreparedStatementTest.java
> URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/PreparedStatementTest.java?view=diff&rev=542016&r1=542015&r2=542016
> ==============================================================================
> --- db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/PreparedStatementTest.java (original)
> +++ db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/PreparedStatementTest.java Sun May 27 11:30:45 2007
> @@ -144,12 +144,21 @@
> s.close();
> ps.close();
>
> + s = null;
> + ps = null;
> +
> psFetchBlob.close();
> psFetchClob.close();
> psInsertBlob.close();
> psInsertClob.close();
> psInsertLongVarchar.close();
>
> + psFetchBlob = null;
> + psFetchClob = null;
> + psInsertBlob = null;
> + psInsertClob = null;
> + psInsertLongVarchar = null;
> +
> super.tearDown();
> }
>
>
> Modified: db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ResultSetTest.java
> URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ResultSetTest.java?view=diff&rev=542016&r1=542015&r2=542016
> ==============================================================================
> --- db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ResultSetTest.java (original)
> +++ db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ResultSetTest.java Sun May 27 11:30:45 2007
> @@ -94,12 +94,11 @@
> protected void tearDown()
> throws Exception {
>
> - if (rs != null) {
> - rs.close();
> - }
> - if (stmt != null) {
> - stmt.close();
> - }
> + rs.close();
> + stmt.close();
> +
> + rs = null;
> + stmt = null;
>
> super.tearDown();
> }
>
> Modified: db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/TestJDBC40Exception.java
> URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/TestJDBC40Exception.java?view=diff&rev=542016&r1=542015&r2=542016
> ==============================================================================
> --- db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/TestJDBC40Exception.java (original)
> +++ db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/TestJDBC40Exception.java Sun May 27 11:30:45 2007
> @@ -56,7 +56,10 @@
> }
>
> protected void tearDown() throws Exception {
> - createStatement().execute("drop table EXCEPTION_TABLE1");
> + Statement s = createStatement();
> + s.execute("drop table EXCEPTION_TABLE1");
> + s.close();
> + commit();
> super.tearDown();
> }
>
>
>
>
Re: svn commit: r542016 - in
/db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4: BlobTest.java
CallableStatementTest.java ClobTest.java DataSourceTest.java
PreparedStatementTest.java ResultSetTest.java TestJDBC40Exception.java
Posted by Knut Anders Hatlen <Kn...@Sun.COM>.
Kristian Waagan <Kr...@Sun.COM> writes:
> I was not thinking about prepareCall() returning null. I think the if
> was added because some people liked to do the cleaning up themselves,
> nullifying the reference in the test method. That's the NPE I was
> thinking about, but these things are probably best handled by code
> reviews (i.e. don't nullify references yourself).
I agree. Clean-up code belongs in tearDown(), not in the test method. If
someone nullified the statement in the test method, I guess they would
get NPE on the first run and fix it.
> The change is okay for me, was just wondering if there was something
> fishy with isClosed() so that it couldn't be trusted.
No, there's nothing fishy about isClosed(), but I don't think it's
necessary since close() is defined to be a no-op if the statement is
closed. And if close() fails on a closed object, I think it is OK that
the test fails too.
--
Knut Anders
Re: svn commit: r542016 - in
/db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4: BlobTest.java
CallableStatementTest.java ClobTest.java DataSourceTest.java
PreparedStatementTest.java ResultSetTest.java TestJDBC40Exception.java
Posted by Kristian Waagan <Kr...@Sun.COM>.
Knut Anders Hatlen wrote:
> Kristian Waagan <Kr...@Sun.COM> writes:
>
>
>>>> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/CallableStatementTest.java
>>>> Sun May 27 11:30:45 2007
>>>> @@ -71,9 +71,9 @@
>>>> */
>>>> protected void tearDown()
>>>> throws Exception {
>>>> - if (cStmt != null && !cStmt.isClosed()) {
>>>> - cStmt.close();
>>>> - }
>>>> +
>>>> + cStmt.close();
>>>> + cStmt = null;
>>>>
>>>>
>>> Just out of curiosity:
>>> At first look the existing code looks sound - calling close only if
>>> the statement is not null and not already closed.
>>> Why is that inadequate?
>>>
>> A bit quick on the send button...
>> I understand why you nullify the reference, what I was wondering about
>> was if there was a reason for changing the if-block.
>> As far as I can tell, it removes complexity, but increases the chance
>> of a NPE. Unless there is some other reason I don's see?
>>
>
> Since none of the test cases in that test closes cStmt or sets it to
> null, I didn't see any reason to check for it. As to the increased
> chance of NPE, since the variable is initialized in setUp() and
> shouldn't be changed afterwards, a NPE would (correctly) have been
> thrown much earlier if prepareCall() had returned null. It seems like
> it's the common practice in the tearDown() methods just to call close
> with no checks, but feel free to change it back.
>
>
I was not thinking about prepareCall() returning null. I think the if
was added because some people liked to do the cleaning up themselves,
nullifying the reference in the test method. That's the NPE I was
thinking about, but these things are probably best handled by code
reviews (i.e. don't nullify references yourself).
The change is okay for me, was just wondering if there was something
fishy with isClosed() so that it couldn't be trusted.
--
Kristian
Re: svn commit: r542016 - in
/db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4: BlobTest.java
CallableStatementTest.java ClobTest.java DataSourceTest.java
PreparedStatementTest.java ResultSetTest.java TestJDBC40Exception.java
Posted by Knut Anders Hatlen <Kn...@Sun.COM>.
Kristian Waagan <Kr...@Sun.COM> writes:
>>> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/CallableStatementTest.java
>>> Sun May 27 11:30:45 2007
>>> @@ -71,9 +71,9 @@
>>> */
>>> protected void tearDown()
>>> throws Exception {
>>> - if (cStmt != null && !cStmt.isClosed()) {
>>> - cStmt.close();
>>> - }
>>> +
>>> + cStmt.close();
>>> + cStmt = null;
>>>
>>
>> Just out of curiosity:
>> At first look the existing code looks sound - calling close only if
>> the statement is not null and not already closed.
>> Why is that inadequate?
>
> A bit quick on the send button...
> I understand why you nullify the reference, what I was wondering about
> was if there was a reason for changing the if-block.
> As far as I can tell, it removes complexity, but increases the chance
> of a NPE. Unless there is some other reason I don's see?
Since none of the test cases in that test closes cStmt or sets it to
null, I didn't see any reason to check for it. As to the increased
chance of NPE, since the variable is initialized in setUp() and
shouldn't be changed afterwards, a NPE would (correctly) have been
thrown much earlier if prepareCall() had returned null. It seems like
it's the common practice in the tearDown() methods just to call close
with no checks, but feel free to change it back.
--
Knut Anders
Re: svn commit: r542016 - in
/db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4: BlobTest.java
CallableStatementTest.java ClobTest.java DataSourceTest.java
PreparedStatementTest.java ResultSetTest.java TestJDBC40Exception.java
Posted by Kristian Waagan <Kr...@Sun.COM>.
Kristian Waagan wrote:
> kahatlen@apache.org wrote:
>> Author: kahatlen
>> Date: Sun May 27 11:30:45 2007
>> New Revision: 542016
>>
>> URL: http://svn.apache.org/viewvc?view=rev&rev=542016
>> Log:
>> DERBY-2707: Inadequate clean-up in many jdbc4 tests
>>
>> Fixed the tearDown() methods in many of the jdbc4 tests.
>>
>> Modified:
>>
>> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/BlobTest.java
>>
>>
>> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/CallableStatementTest.java
>>
>>
>> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ClobTest.java
>>
>>
>> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/DataSourceTest.java
>>
>>
>> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/PreparedStatementTest.java
>>
>>
>> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ResultSetTest.java
>>
>>
>> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/TestJDBC40Exception.java
>>
>>
>> Modified:
>> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/BlobTest.java
>>
>> URL:
>> http://svn.apache.org/viewvc/db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/BlobTest.java?view=diff&rev=542016&r1=542015&r2=542016
>>
>> ==============================================================================
>>
>> ---
>> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/BlobTest.java
>> (original)
>> +++
>> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/BlobTest.java
>> Sun May 27 11:30:45 2007
>> @@ -125,7 +125,7 @@
>> //throwing a SQLException when they are called after calling free()
>> //on a LOB.
>> - private ExemptBlobMD [] emd = new ExemptBlobMD [] {
>> + private static final ExemptBlobMD [] emd = new ExemptBlobMD [] {
>> new ExemptBlobMD( "getBinaryStream", new Class[] {
>> long.class,long.class }
>>
>> ,true,true ),
>> new ExemptBlobMD( "setBinaryStream", new Class[] {
>> long.class },false,true ),
>> @@ -167,6 +167,13 @@
>> //from throwing a SQLException after free has been called
>> //on the Clob object.
>> buildHashSet();
>> + }
>> +
>> + protected void tearDown() throws Exception {
>> + blob.free();
>> + blob = null;
>> + excludedMethodSet = null;
>> + super.tearDown();
>> }
>> /**
>>
>> Modified:
>> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/CallableStatementTest.java
>>
>> URL:
>> http://svn.apache.org/viewvc/db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/CallableStatementTest.java?view=diff&rev=542016&r1=542015&r2=542016
>>
>> ==============================================================================
>>
>> ---
>> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/CallableStatementTest.java
>> (original)
>> +++
>> db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/CallableStatementTest.java
>> Sun May 27 11:30:45 2007
>> @@ -71,9 +71,9 @@
>> */
>> protected void tearDown()
>> throws Exception {
>> - if (cStmt != null && !cStmt.isClosed()) {
>> - cStmt.close();
>> - }
>> +
>> + cStmt.close();
>> + cStmt = null;
>>
>
> Just out of curiosity:
> At first look the existing code looks sound - calling close only if
> the statement is not null and not already closed.
> Why is that inadequate?
A bit quick on the send button...
I understand why you nullify the reference, what I was wondering about
was if there was a reason for changing the if-block.
As far as I can tell, it removes complexity, but increases the chance of
a NPE. Unless there is some other reason I don's see?
regards,
--
Kristian
>
>