Ignored exceptions in DerbyJUnitTest

[Daniel John Debrunner <dj...@apache.org>]

DerbyJUnitTest has many cases where the SQLException thrown by Derby
(through JDBC) is ignored. To me, this seems very bad practice for test
code.

For example:

	//
	// Swallow uninteresting exceptions when disposing of jdbc objects.
	//
	protected	static	void	close( ResultSet rs )
	{
		try {
			if ( rs != null ) { rs.close(); }
		}
		catch (SQLException e) {}
	}	

If ResultSet.close is throwing an exception when being closed then to my
mind that's a bug, but we would never see it through a Junit test.

And for dropping schema objects:

	protected	static	void	dropSchemaObject( Connection conn, String genus,
String objectName )
	{
		PreparedStatement	ps = null;
		
		try {
			ps = prepare( conn, "drop " + genus + " " + objectName );

			ps.execute();
		}
		catch (SQLException e) {}

		close( ps );
	}

If this code is intended to handle the object already being dropped then
an explict SQLState check would be preferred to this blanket ignoring of
 exceptions. Again, there is the potential for valid bugs to be hidden
by this ignoring of exceptions.

I think it would be good to significantly clean up this utility class
before it gets used by too many tests. Very few of the methods have
useful Javadoc indicating their intended purpose, I'm especially
confused by the calls that convert returned values to Object, there seem
to be two approaches, convert using JDBC Type id and convert using
expected object type, not sure when I would use one or the other. And
related to that, SMALLINT is typically converted to a Short whereas JDBC
 uses a Integer. Not sure if the difference is intentional or not.

Dan.

Re: Ignored exceptions in DerbyJUnitTest

[Andreas Korneliussen <An...@Sun.COM>]

>> If ResultSet.close is throwing an exception when being closed then to my
>> mind that's a bug, but we would never see it through a Junit test.
>>
> Unless the resultset is already closed, or you have closed the 
> connection for the resultset.
> 
Correction: I guess ResultSet.close() should not throw an exception if 
it is already closed.

My point was anyway that we should be careful about throwing exceptions 
to the junit-framework in the tearDown() since it will overwrite the 
"real" error.

Andreas

Re: Ignored exceptions in DerbyJUnitTest

[Daniel John Debrunner <dj...@apache.org>]

Andreas Korneliussen wrote:

> Daniel John Debrunner wrote:
> 
>> DerbyJUnitTest has many cases where the SQLException thrown by Derby
>> (through JDBC) is ignored. To me, this seems very bad practice for test
>> code.
>>
> Isn't that bad practice for any code ?

True, but it's *very* bad practice for test code. :-)


> Anyway, I think there are cases when you are interested in not throwing
> the SQLException to the JUnit framework.
> 
> If your test has already failed with a meaningful assertion which
> describes the error, the junit- test runner will call the tearDown()
> method on your TestCase object, which may throw an exception.
> If the tearDown() throws i.e "ResultSet is closed", that error will
> overwrite the original test failure in the JUnit test report.

Thanks for that, I didn't know that. I agree there may be cases where
you want to hide an exception, maybe a specific exception, but it should
not be the normal case for test code. Maybe the parent class
DerbyJUnitTest should have a final tearDown method that calls an method
and catches all SQLExceptions are reports them. Or such close methods
should be called tearDownClose() to indicate they are only to be used in
tearDown.

rough example

public final void tearDown()
{
     try {
        derbyTearDown();
     } catch (SQLException sqle)
     {
        reportExceptionSomeHow(sqle);
     }
}

// tests can implement this method to provide test
// specific teardown.
protected void derbyTearDown() throws SQLException
{
}

Dan.

Re: Ignored exceptions in DerbyJUnitTest

[Andreas Korneliussen <An...@Sun.COM>]

Daniel John Debrunner wrote:
> DerbyJUnitTest has many cases where the SQLException thrown by Derby
> (through JDBC) is ignored. To me, this seems very bad practice for test
> code.
> 
Isn't that bad practice for any code ?

Anyway, I think there are cases when you are interested in not throwing 
the SQLException to the JUnit framework.

If your test has already failed with a meaningful assertion which 
describes the error, the junit- test runner will call the tearDown() 
method on your TestCase object, which may throw an exception.
If the tearDown() throws i.e "ResultSet is closed", that error will 
overwrite the original test failure in the JUnit test report.

An alternative to swallowing the exception, is then of course to log it, 
or to gather them somehow, and report them at the end of running the 
testsuite.

> For example:
> 
> 	//
> 	// Swallow uninteresting exceptions when disposing of jdbc objects.
> 	//
> 	protected	static	void	close( ResultSet rs )
> 	{
> 		try {
> 			if ( rs != null ) { rs.close(); }
> 		}
> 		catch (SQLException e) {}
> 	}	
> 
> If ResultSet.close is throwing an exception when being closed then to my
> mind that's a bug, but we would never see it through a Junit test.
> 
Unless the resultset is already closed, or you have closed the 
connection for the resultset.

Andreas