Derby
  1. Derby
  2. DERBY-5305

Convert store/updatelocks.sql to JUnit

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.8.3.0, 10.9.1.0
    • Component/s: Test
    • Labels:
      None
    1. derby-5305-b.stat
      0.9 kB
      Dag H. Wanvik
    2. derby-5305-b.diff.gz
      113 kB
      Dag H. Wanvik

      Issue Links

        Activity

        Hide
        Dag H. Wanvik added a comment -

        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.

        Show
        Dag H. Wanvik added a comment - 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.
        Hide
        Dag H. Wanvik added a comment -

        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.

        Show
        Dag H. Wanvik added a comment - 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.
        Hide
        Knut Anders Hatlen added a comment -

        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.
        Show
        Knut Anders Hatlen added a comment - 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.
        Hide
        Dag H. Wanvik added a comment -

        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.

        Show
        Dag H. Wanvik added a comment - 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.
        Hide
        Dag H. Wanvik added a comment -

        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.

        Show
        Dag H. Wanvik added a comment - 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.
        Hide
        Dag H. Wanvik added a comment -

        Regressions ran OK, committed as svn 1142626.

        Show
        Dag H. Wanvik added a comment - Regressions ran OK, committed as svn 1142626.
        Hide
        Kristian Waagan added a comment -

        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?

        Show
        Kristian Waagan added a comment - 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?
        Hide
        Dag H. Wanvik added a comment -

        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.

        Show
        Dag H. Wanvik added a comment - 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.
        Hide
        Dag H. Wanvik added a comment -

        Committed a patch to this end as svn 1143939.

        Show
        Dag H. Wanvik added a comment - Committed a patch to this end as svn 1143939.
        Hide
        Mike Matrigali added a comment -

        reopening to backport to 10.8.

        Show
        Mike Matrigali added a comment - reopening to backport to 10.8.
        Hide
        Mike Matrigali added a comment -

        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.

        Show
        Mike Matrigali added a comment - 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.

          People

          • Assignee:
            Dag H. Wanvik
            Reporter:
            Dag H. Wanvik
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development