db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Daniel John Debrunner (JIRA)" <j...@apache.org>
Subject [jira] Commented: (DERBY-2283) convert lang/currentof.java test to junit test
Date Fri, 02 Feb 2007 00:05:05 GMT

    [ https://issues.apache.org/jira/browse/DERBY-2283?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12469623
] 

Daniel John Debrunner commented on DERBY-2283:
----------------------------------------------

The test generally looks good, usually the timeout failures are due to another test not closing
its JDBC objects, such as statements. So it might be due to your test, I think at least one
prepared statement is not closed.

Some minor improvements that would make the test even more readable for others:

Instead of:
   assertEquals(cursor.getCursorName(), null);
use
   assertNull(cursor.getCursorName());
Also the pattern for assert methods is the expected value is the left argument , and the tested
value the right, the use with null above has it the other way around, just an FYI.

Similar for:
   assertEquals(false, cursor.next());
use
   assertFalse(cursor.next());

You can look at the JUnit javadoc for the Assert class to see the range of assert methods
available.

I think many of the try-catch blocks in the test can be replaced with the assert methods Army
added a while ago:
e.g.
		try {
			delete.executeUpdate(); // no current row / closed
			fail("Exception expected above!");
		} catch (SQLException se) {
			assertSQLState("24000", se);
		}

with
       // no current row / closed
       assertStatementError("24000", delete);

This helps to give the code a cleaner look.

That would also cleanup these try-catch blocks:
		try {
			assertUpdateCount(update, 0);
			fail("Exception expected above!");

		} catch (SQLException se) {
			assertSQLState("XCL07", se);
		}

Here I think the assertUpdateCount() confuses the reader as you don't expect that update statement
to succeed an return zero rows, but the assertUpdateCount implies that. If for some reason
the assertStatementError() didn't work here, I think clearer code would be:
		try {
			update.executeUpdate();
			fail("Exception expected above!");

		} catch (SQLException se) {
			assertSQLState("XCL07", se);
		}
but this is just an FYI, since the assertStatementError is the correct approach.

Can you provide a patch using svn diff instead of individual files? Also are you going to
remove the old test and its master files etc?



> convert lang/currentof.java test to junit test
> ----------------------------------------------
>
>                 Key: DERBY-2283
>                 URL: https://issues.apache.org/jira/browse/DERBY-2283
>             Project: Derby
>          Issue Type: Test
>            Reporter: Manjula Kutty
>         Assigned To: Manjula Kutty
>            Priority: Minor
>             Fix For: 10.3.0.0
>
>         Attachments: _Suite.java, CurrentOfTest.java
>
>
> Using this Jira entry to attach the converted test in future

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message