Dag H. Wanvik
added a comment - 29/Jun/11 11:36 Uploading a patch which converts updatelocks.sql into JUnit: UpdateLocksTest.
It also contains a fix to JDBC.assertUnorderedResultSet for a bug when trimming is not used.
Running regressions.

A comment on my approach to the conversion: I used the existing canons and converted them semi-automatically into corresponding Java result set tuples for the assertions, to ensure the test semantics are retained.

Dag H. Wanvik
added a comment - 29/Jun/11 11:47 A comment on my approach to the conversion: I used the existing canons and converted them semi-automatically into corresponding Java result set tuples for the assertions, to ensure the test semantics are retained.

The new test looks clean and systematic, so I'd say commit it. I must admit I haven't read every single line of the patch, but just to prove that I have looked at it, here are my nit-picks:

In decorateSQL(), auto-commit is turned off and commit() is called explicitly at the end of the method. This is not necessary, since CleanDatabaseTestSetup already turns off auto-commit before calling decorateSQL() and commits immediately afterwards.

Most of the code in decorateSQL() is commented out. Could it be removed?

One of the added lines in JDBC.java has trailing blanks.

In tearDown(), I'd suggest using the helper method BaseJDBCTestCase.dropTable().

Closing getLocksQuery in tearDown() shouldn't be necessary since it was created with BaseJDBCTestCase.prepareStatement() and will be closed in super.tearDown(). Setting the reference to null would be good, though, so that the statement and the connection can be garbage collected after the test case has completed.

Knut Anders Hatlen
added a comment - 29/Jun/11 14:42 Whoa! A 2.68 MB patch!
The new test looks clean and systematic, so I'd say commit it. I must admit I haven't read every single line of the patch, but just to prove that I have looked at it, here are my nit-picks:
In decorateSQL(), auto-commit is turned off and commit() is called explicitly at the end of the method. This is not necessary, since CleanDatabaseTestSetup already turns off auto-commit before calling decorateSQL() and commits immediately afterwards.
Most of the code in decorateSQL() is commented out. Could it be removed?
One of the added lines in JDBC.java has trailing blanks.
In tearDown(), I'd suggest using the helper method BaseJDBCTestCase.dropTable().
Closing getLocksQuery in tearDown() shouldn't be necessary since it was created with BaseJDBCTestCase.prepareStatement() and will be closed in super.tearDown(). Setting the reference to null would be good, though, so that the statement and the connection can be garbage collected after the test case has completed.

Thanks for the comments, Knut. As for the commented out decorateSQL code, I was torn on whether to remove it or retain it since it was part of the original test (although unused), but kept it till review time so someone else could chime in on it... Now that you spotted it, I'm +1 to removing it unless someone closer to the original authorship of this test feels it should stay.

Dag H. Wanvik
added a comment - 30/Jun/11 12:33 Thanks for the comments, Knut. As for the commented out decorateSQL code, I was torn on whether to remove it or retain it since it was part of the original test (although unused), but kept it till review time so someone else could chime in on it... Now that you spotted it, I'm +1 to removing it unless someone closer to the original authorship of this test feels it should stay.

Uploading version "b" incorporating Knut's suggestions including removing the cruft. I also got rid of some too long lines. Re-running regressions. This time I upload a gzipped version of the patch ("b") and I will also remove the first 2.5Mb version ("a") to save some space. If somebody want to see the first version, I will retain it for some time.

Dag H. Wanvik
added a comment - 01/Jul/11 13:04 Uploading version "b" incorporating Knut's suggestions including removing the cruft. I also got rid of some too long lines. Re-running regressions. This time I upload a gzipped version of the patch ("b") and I will also remove the first 2.5Mb version ("a") to save some space. If somebody want to see the first version, I will retain it for some time.

Committed a trivial cleanup as revision 1143768: removed two unused imports and removed an unused variable.
The method 'show' is currently unused. Is this something that is useful for debugging, or can it be removed? If the former, maybe a comment saying so would be useful?

Kristian Waagan
added a comment - 07/Jul/11 12:37 Committed a trivial cleanup as revision 1143768: removed two unused imports and removed an unused variable.
The method 'show' is currently unused. Is this something that is useful for debugging, or can it be removed? If the former, maybe a comment saying so would be useful?

Yes, the show method was useful when making the test and I see it being potentially useful when debugging the test if it ever starts to fail. I'll add a Javadoc comment and comment out the whole for now for good measure.

Dag H. Wanvik
added a comment - 07/Jul/11 17:35 Yes, the show method was useful when making the test and I see it being potentially useful when debugging the test if it ever starts to fail. I'll add a Javadoc comment and comment out the whole for now for good measure.

backported the fix to 10.8. 10.8 was seeing some intermittent failures in updatelocks.sql test, so backporting this will eliminate those and I have not
seen any intermittent errors assocated with the new UpdateLocks converted test.

Mike Matrigali
added a comment - 10/Feb/12 20:00 backported the fix to 10.8. 10.8 was seeing some intermittent failures in updatelocks.sql test, so backporting this will eliminate those and I have not
seen any intermittent errors assocated with the new UpdateLocks converted test.