[jira] Commented: (DERBY-2283) convert lang/currentof.java test to junit test

["Daniel John Debrunner (JIRA)" <ji...@apache.org>]

    [ 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.