Derby
  1. Derby
  2. DERBY-4509

Convert autoincrement.sql to JUnit

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.6.1.0
    • Component/s: Test
    • Labels:
      None
    • Environment:
      Apache Derby

      Description

      The task is to convert existing autoincrement.sql to JUnit

      1. AutoIncrementDO-NOT_COMMIT_diff.txt
        3 kB
        Jayaram Subramanian
      2. AutoIncrementDO_NOT_COMMIT_diff.txt
        30 kB
        Jayaram Subramanian
      3. AutoIncrementDO_NOT_COMMIT_diff.txt
        58 kB
        Jayaram Subramanian
      4. jira4509.txt
        101 kB
        Jayaram Subramanian
      5. jira4509stat.txt
        0.7 kB
        Jayaram Subramanian
      6. jira4509feb4.txt
        193 kB
        Jayaram Subramanian
      7. jira4509feb4stat.txt
        0.5 kB
        Jayaram Subramanian
      8. AutoIncrementrevised.txt
        192 kB
        Jayaram Subramanian
      9. statrevised.txt
        0.5 kB
        Jayaram Subramanian
      10. autoincrement_March162010.txt
        200 kB
        Jayaram Subramanian
      11. stat_March162010.txt
        0.5 kB
        Jayaram Subramanian
      12. stat_March24.txt
        0.5 kB
        Jayaram Subramanian
      13. autoincrement_March24.txt
        199 kB
        Jayaram Subramanian

        Activity

        Hide
        Knut Anders Hatlen added a comment -

        [bulk update] Close all resolved issues that haven't been updated for more than one year.

        Show
        Knut Anders Hatlen added a comment - [bulk update] Close all resolved issues that haven't been updated for more than one year.
        Hide
        Myrna van Lunteren added a comment -

        Looks fine from the nightly tests from IBM (not sure what's up with the ones from Sun/Oracle, but likely unrelated to this issue), so resolving. The reporter can close this, I think.

        Show
        Myrna van Lunteren added a comment - Looks fine from the nightly tests from IBM (not sure what's up with the ones from Sun/Oracle, but likely unrelated to this issue), so resolving. The reporter can close this, I think.
        Hide
        Myrna van Lunteren added a comment -

        I ran the tests with ibm 1.6 on windows, and derbyall and suites.All both passed for me.
        Committed patch autoincrement_March24.txt to trunk with revision 930311.
        I suggest we leave this open for a day or so until the tinderbox tests have passed.
        Thanks for the contribution and congratulations on getting your first patch committed Jayaram!

        Show
        Myrna van Lunteren added a comment - I ran the tests with ibm 1.6 on windows, and derbyall and suites.All both passed for me. Committed patch autoincrement_March24.txt to trunk with revision 930311. I suggest we leave this open for a day or so until the tinderbox tests have passed. Thanks for the contribution and congratulations on getting your first patch committed Jayaram!
        Hide
        Jayaram Subramanian added a comment -

        Submitting the patch as per the latest review comments from Kathy

        Show
        Jayaram Subramanian added a comment - Submitting the patch as per the latest review comments from Kathy
        Hide
        Kathey Marsden added a comment -

        I looked at the autoincrement_March162010.txt patch and it looks good except that there seem to be some extra commented out statements that should be removed, e.g as I think they must be something superfluous from your work converting the test e.g after //-with unique index you have.

        //s.execute("insert into uniquet4 values(3)");
        //s.execute("insert into uniquet4 values(4)");
        //s.execute("insert into uniquet4 values(5)");
        //assertStatementError("23505",s,"insert into uniquet4(i,uniquet4_autogen) values(1,0)");
        //assertStatementError("23505",s,"insert into uniquet4(i,uniquet4_autogen) values(1,0)");

        Please remove those and post a new patch and I will commit it. I will be out next week, so hopefully you can do this today or tomorrow.

        After commit, It might be nice too as extra credit, to make an additional patch in which you add javadoc comments to the fixtures. In Eclipse, you can just click on the method name and then right click -> Source -> Generate element comment and then add a brief comment of what the fixture is testing, just to make things tidy, but first just remove the superfluous commented code and then follow up with this if you want to.

        Show
        Kathey Marsden added a comment - I looked at the autoincrement_March162010.txt patch and it looks good except that there seem to be some extra commented out statements that should be removed, e.g as I think they must be something superfluous from your work converting the test e.g after //-with unique index you have. //s.execute("insert into uniquet4 values(3)"); //s.execute("insert into uniquet4 values(4)"); //s.execute("insert into uniquet4 values(5)"); //assertStatementError("23505",s,"insert into uniquet4(i,uniquet4_autogen) values(1,0)"); //assertStatementError("23505",s,"insert into uniquet4(i,uniquet4_autogen) values(1,0)"); Please remove those and post a new patch and I will commit it. I will be out next week, so hopefully you can do this today or tomorrow. After commit, It might be nice too as extra credit, to make an additional patch in which you add javadoc comments to the fixtures. In Eclipse, you can just click on the method name and then right click -> Source -> Generate element comment and then add a brief comment of what the fixture is testing, just to make things tidy, but first just remove the superfluous commented code and then follow up with this if you want to.
        Hide
        Jayaram Subramanian added a comment -

        Update AutoIncrementTest as per interim review comments from Kathy..

        Show
        Jayaram Subramanian added a comment - Update AutoIncrementTest as per interim review comments from Kathy..
        Hide
        Kathey Marsden added a comment -

        I spoke with Jayaram on the phone and IRC about the Autoincrementrevised.txt patch.
        The main thing was that he needs to review the new test and compare it to autoincrement.out and make sure everything is included. I put suggested he put it in the same general order and copy the comments exactly so we don't miss anything and it is easy to check. Besides that, I had the following details:

        • suite.addTest(OrderByAndOffsetFetchInSubqueries.suite()); seems to be missing now from lang/_Suite.java
        • autoincrement.sql is still in derbylang.runall and needs to be removed.
        • test name in AutoIncrementTest in licence header should be changed from SimpleTest
        • use local variables for rs and expectedRows in each fixture instead of fields. Fields tend to leak memory in Junit tests.
        • make sure lock_table queries are being executed as they were in autoincrement.sql. The view creation is there but I didn't see the queries to check for locking behavior.
        Show
        Kathey Marsden added a comment - I spoke with Jayaram on the phone and IRC about the Autoincrementrevised.txt patch. The main thing was that he needs to review the new test and compare it to autoincrement.out and make sure everything is included. I put suggested he put it in the same general order and copy the comments exactly so we don't miss anything and it is easy to check. Besides that, I had the following details: suite.addTest(OrderByAndOffsetFetchInSubqueries.suite()); seems to be missing now from lang/_Suite.java autoincrement.sql is still in derbylang.runall and needs to be removed. test name in AutoIncrementTest in licence header should be changed from SimpleTest use local variables for rs and expectedRows in each fixture instead of fields. Fields tend to leak memory in Junit tests. make sure lock_table queries are being executed as they were in autoincrement.sql. The view creation is there but I didn't see the queries to check for locking behavior.
        Hide
        Jayaram Subramanian added a comment -

        Attaching the autoincrement revised file based on the review comments from Kathy.

        Show
        Jayaram Subramanian added a comment - Attaching the autoincrement revised file based on the review comments from Kathy.
        Hide
        Kathey Marsden added a comment -

        Thank you Jayaram for the patch.
        My first general comment would be for the creation and cleanup of schema objects, you use CleanDatabaseTestSetup and decorateSQL to create the objects such as tables and views up front and then let them get cleaned up automatically at the end of the test. You can then remove the drop statements from the test. If any table names are reused in the test you will have to choose distinct names so there is no collision. One big advantage of doing it this way is that if the test fails for some reason we are assured that things get cleaned up properly and also declutters the test. Take a look at GroupByTest for an example of how to do this.

        My second would be that it would be good to break the test up into multiple fixtures instead of just one big one, for example maybe testIdentity testIdentityWithStartValue, testIdentityWithIncrement, etc. Just go down through the autoincrement.out file and group it into logical chunks and make sure each fixture has javadoc comments.

        I did not check the out file against the new test, but noticed that the lock checking seems to be missing. You need to add the creation of the lock_table view and check the locks where they are being checked in the original test. The master/autoincrement.out file has the expected output.

        Also a small thing, instead of doing:
        Connection conn = getConnection();
        Statement s = conn.createStatement();
        to get a statement, just do
        Statement s - getStatement();

        Here again the statement will get cleaned up automatically, so you don't have to put the cleanup code in the test.

        There are other built in helper methods too, like
        conn.commit(); -> commit()
        conn.setAutoCommit(true); -> setAutoCommit(true);

        Take a look at BaseJDBCTestCase to see what is available.

        Thanks

        Kathey

        Show
        Kathey Marsden added a comment - Thank you Jayaram for the patch. My first general comment would be for the creation and cleanup of schema objects, you use CleanDatabaseTestSetup and decorateSQL to create the objects such as tables and views up front and then let them get cleaned up automatically at the end of the test. You can then remove the drop statements from the test. If any table names are reused in the test you will have to choose distinct names so there is no collision. One big advantage of doing it this way is that if the test fails for some reason we are assured that things get cleaned up properly and also declutters the test. Take a look at GroupByTest for an example of how to do this. My second would be that it would be good to break the test up into multiple fixtures instead of just one big one, for example maybe testIdentity testIdentityWithStartValue, testIdentityWithIncrement, etc. Just go down through the autoincrement.out file and group it into logical chunks and make sure each fixture has javadoc comments. I did not check the out file against the new test, but noticed that the lock checking seems to be missing. You need to add the creation of the lock_table view and check the locks where they are being checked in the original test. The master/autoincrement.out file has the expected output. Also a small thing, instead of doing: Connection conn = getConnection(); Statement s = conn.createStatement(); to get a statement, just do Statement s - getStatement(); Here again the statement will get cleaned up automatically, so you don't have to put the cleanup code in the test. There are other built in helper methods too, like conn.commit(); -> commit() conn.setAutoCommit(true); -> setAutoCommit(true); Take a look at BaseJDBCTestCase to see what is available. Thanks Kathey
        Hide
        Jayaram Subramanian added a comment -

        Patch updated after discussion and review comments from Kathy

        Show
        Jayaram Subramanian added a comment - Patch updated after discussion and review comments from Kathy
        Hide
        Jayaram Subramanian added a comment -

        All changes related to conversion of autoincrementTest.sql file to autoincrementTest.java junit file have been included in this patch.

        Show
        Jayaram Subramanian added a comment - All changes related to conversion of autoincrementTest.sql file to autoincrementTest.java junit file have been included in this patch.
        Hide
        Jayaram Subramanian added a comment -

        Complete converted code of autoincrement.sql

        Show
        Jayaram Subramanian added a comment - Complete converted code of autoincrement.sql
        Hide
        Jayaram Subramanian added a comment -

        Interim patch for AutoIncrement Test conversion

        Show
        Jayaram Subramanian added a comment - Interim patch for AutoIncrement Test conversion
        Hide
        Jayaram Subramanian added a comment -

        Partial patch for JUnit conversion of autoincrement.sql

        Show
        Jayaram Subramanian added a comment - Partial patch for JUnit conversion of autoincrement.sql

          People

          • Assignee:
            Jayaram Subramanian
            Reporter:
            Jayaram Subramanian
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development