Derby
  1. Derby
  2. DERBY-2467

Convert lang/updateCursor.java to JUnit

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.3.1.4
    • Fix Version/s: 10.3.1.4
    • Component/s: Test
    • Labels:
      None
    • Urgency:
      Low

      Description

      Convert lang/updateCursor.java to JUnit

      1. DERBY-2467_diff_03_27.txt
        13 kB
        Ugo Matrangolo
      2. DERBY-2467_diff_050407_2.txt
        13 kB
        Ugo Matrangolo
      3. DERBY-2467_diff_050407.txt
        13 kB
        Ugo Matrangolo
      4. DERBY-2467_diff_300307.txt
        13 kB
        Ugo Matrangolo
      5. DERBY-2467_stat_03_27.txt
        0.2 kB
        Ugo Matrangolo
      6. DERBY-2467_stat_050407.txt
        0.3 kB
        Ugo Matrangolo
      7. DERBY-2467_stat_300307.txt
        0.3 kB
        Ugo Matrangolo

        Issue Links

          Activity

          Hide
          Ugo Matrangolo added a comment -

          First draft of the patch that converts updateCursor.java to JUnit.

          I think that the issue is almost resolved. The only thing that remains to do is to read in some way the property file linked with the old test. Unfortunately, I haven't found no example of how to accomplish this task.

          Can someone point me on how to allow my test to read the correct property file ???

          The code is simply a 1-1 translation of the old harness code to JUnit plus some string/code refactory.

          The test was executed on WinXP and Ubuntu with no problems.

          Show
          Ugo Matrangolo added a comment - First draft of the patch that converts updateCursor.java to JUnit. I think that the issue is almost resolved. The only thing that remains to do is to read in some way the property file linked with the old test. Unfortunately, I haven't found no example of how to accomplish this task. Can someone point me on how to allow my test to read the correct property file ??? The code is simply a 1-1 translation of the old harness code to JUnit plus some string/code refactory. The test was executed on WinXP and Ubuntu with no problems.
          Hide
          Ugo Matrangolo added a comment -

          please review

          Show
          Ugo Matrangolo added a comment - please review
          Hide
          Andrew McIntyre added a comment -

          Ugo> I think that the issue is almost resolved. The only thing that remains to do is to read in some way the property file linked with the old test. Unfortunately, I haven't found no example of how to accomplish this task.

          Tests that are converted to JUnit should specify their necessary system properties programmatically, and not rely on external files. For system properties, there is a SystemPropertyTestSetup class, which provides a JUnit decorator for specifying system properties.

          Good examples of using this decorator are in tests/derbynet/ClientSideSystemPropertiesTest.suite() and tests/jdbcapi/AuthenticationTest.setBaseProps().

          A couple of other comments:

          • Instead of setting up the database in a setUp() method and then having your test be responsible for cleaning the database at teardown(), take a look at using the CleanDatabaseTestSetup decorator. A good example is in tests/lang/GroupByExpression test, but there are many other examples. Using this test decorator requires anonymous overloading of the CleanDatabaseTestSetup's decorateSQL method, but doing so will allow you to remove a lot of code related to setting up and cleaning out the database, which helps makes the test code in the test stand out from the 'background noise' of the setup operations. Refer to other tests which make use of CleanDatabaseTestSetup to get a feel for how it is used.
          • Using the above may require rethinking how the data in the table is filled. At any rate, the process of assembling the insert statement that inserts the 'largeString' into the test tables can probably be simplified. As it is now, there is a lot of object creation and method calling going on behind the scenes of the statement that assembles the String (and thus StringBuffer/StringBuilder) for inserting the 'largestring' and various values of i into t1. Using a PreparedStatement here and setting its parameters for each iteration of the for loop would have noticable benefits, including reuse of the compiled PreparedStatement and avoiding unnecessary creation of String objects in that large string concatenation that occurs for each iteration of the for loop.
          • Because BaseJDBCTestCase extends from junit.framework.TestCase, it is usually not necessary to explicitly add the Assert classname to assert methods when using them in tests. e.g.:

          Assert.assertNull(...);

          can usually be called as:

          assertNull(...);

          Thanks for taking the time to convert this test! I'll take a closer look at it tomorrow and let you know if I have any further comments.

          Show
          Andrew McIntyre added a comment - Ugo> I think that the issue is almost resolved. The only thing that remains to do is to read in some way the property file linked with the old test. Unfortunately, I haven't found no example of how to accomplish this task. Tests that are converted to JUnit should specify their necessary system properties programmatically, and not rely on external files. For system properties, there is a SystemPropertyTestSetup class, which provides a JUnit decorator for specifying system properties. Good examples of using this decorator are in tests/derbynet/ClientSideSystemPropertiesTest.suite() and tests/jdbcapi/AuthenticationTest.setBaseProps(). A couple of other comments: Instead of setting up the database in a setUp() method and then having your test be responsible for cleaning the database at teardown(), take a look at using the CleanDatabaseTestSetup decorator. A good example is in tests/lang/GroupByExpression test, but there are many other examples. Using this test decorator requires anonymous overloading of the CleanDatabaseTestSetup's decorateSQL method, but doing so will allow you to remove a lot of code related to setting up and cleaning out the database, which helps makes the test code in the test stand out from the 'background noise' of the setup operations. Refer to other tests which make use of CleanDatabaseTestSetup to get a feel for how it is used. Using the above may require rethinking how the data in the table is filled. At any rate, the process of assembling the insert statement that inserts the 'largeString' into the test tables can probably be simplified. As it is now, there is a lot of object creation and method calling going on behind the scenes of the statement that assembles the String (and thus StringBuffer/StringBuilder) for inserting the 'largestring' and various values of i into t1. Using a PreparedStatement here and setting its parameters for each iteration of the for loop would have noticable benefits, including reuse of the compiled PreparedStatement and avoiding unnecessary creation of String objects in that large string concatenation that occurs for each iteration of the for loop. Because BaseJDBCTestCase extends from junit.framework.TestCase, it is usually not necessary to explicitly add the Assert classname to assert methods when using them in tests. e.g.: Assert.assertNull(...); can usually be called as: assertNull(...); Thanks for taking the time to convert this test! I'll take a closer look at it tomorrow and let you know if I have any further comments.
          Hide
          Ugo Matrangolo added a comment -

          Andrew,

          thank you very much for your comments and suggestions.

          I took advantage of your suggestions to use the proper decorators for setting the test schema on the db and for setting up correct system properties.

          If you have the opportunity to review my code, please, tell me if you see other possible improvements.

          Again, the test was executed with no problems on Ubuntu and WinXP environments.

          Show
          Ugo Matrangolo added a comment - Andrew, thank you very much for your comments and suggestions. I took advantage of your suggestions to use the proper decorators for setting the test schema on the db and for setting up correct system properties. If you have the opportunity to review my code, please, tell me if you see other possible improvements. Again, the test was executed with no problems on Ubuntu and WinXP environments.
          Hide
          Andrew McIntyre added a comment -

          Hi Ugo,

          I took a closer look at the test, a few comments:

          • I think a check for the order of the rows for column C3 should be added to testVirtualMemoryHeap(). The old test 'checked' this by comparing the output of the order of the rows with a master file, but the test intends to check that rows are spilled from the hash table to VM to disk and that orders changes based on whether the rows are in the hashtable for the scan, virtual memory, or on disk. Please see the comment from the previous test that was output to System.out. I think this comment should be retained in the new test as well so the intent of this test method is more clear.
          • Stylistically, I would have kept the SQL statements in the test fixtures instead of as instance variables. It's easier when reading through the test to understand what is being tested if the text of the SQL that is being executed is in the test fixture.
          • verifyCount could be replaced with calls to assertUpdateCount() from BaseJDBCTestCase. Similarly, verifyBoolean could be replaced with JUnit's assertTrue() / assertFalse().

          Unchecking patch available. The last two are style issues, I'll be glad to commit a new patch with only the testVirtualMemoryHeap() issue fixed.

          Show
          Andrew McIntyre added a comment - Hi Ugo, I took a closer look at the test, a few comments: I think a check for the order of the rows for column C3 should be added to testVirtualMemoryHeap(). The old test 'checked' this by comparing the output of the order of the rows with a master file, but the test intends to check that rows are spilled from the hash table to VM to disk and that orders changes based on whether the rows are in the hashtable for the scan, virtual memory, or on disk. Please see the comment from the previous test that was output to System.out. I think this comment should be retained in the new test as well so the intent of this test method is more clear. Stylistically, I would have kept the SQL statements in the test fixtures instead of as instance variables. It's easier when reading through the test to understand what is being tested if the text of the SQL that is being executed is in the test fixture. verifyCount could be replaced with calls to assertUpdateCount() from BaseJDBCTestCase. Similarly, verifyBoolean could be replaced with JUnit's assertTrue() / assertFalse(). Unchecking patch available. The last two are style issues, I'll be glad to commit a new patch with only the testVirtualMemoryHeap() issue fixed.
          Hide
          Ugo Matrangolo added a comment -

          Hi Andrew,

          I took advantage of your comments and added the check on the column.

          Thank you,
          Ugo.

          Show
          Ugo Matrangolo added a comment - Hi Andrew, I took advantage of your comments and added the check on the column. Thank you, Ugo.
          Hide
          Ugo Matrangolo added a comment -

          Fixed a missing fail string.

          Show
          Ugo Matrangolo added a comment - Fixed a missing fail string.
          Hide
          Andrew McIntyre added a comment -

          Committed DERBY-2467_diff_050407_2.txt to trunk with revision 526070. Made one minor change, removed the check of the text of the error message at the end, this is likely to differ between locales. It is sufficient to check that the SQLState is correct.

          Thank you for converting this test, Ugo!

          Show
          Andrew McIntyre added a comment - Committed DERBY-2467 _diff_050407_2.txt to trunk with revision 526070. Made one minor change, removed the check of the text of the error message at the end, this is likely to differ between locales. It is sufficient to check that the SQLState is correct. Thank you for converting this test, Ugo!
          Hide
          Øystein Grøvlen added a comment -

          This test has been failing in the tinderbox test since it was checked in. The failure the first time after it was checked in was:

          2) testVirtualMemoryHeap(org.apache.derbyTesting.functionTests.tests.lang.UpdateCursorTest)junit.framework.AssertionFailedError: Virtual memory heap test failed! Got unexpected value. expected:<202> but was:<103>
          at org.apache.derbyTesting.functionTests.tests.lang.UpdateCursorTest.testVirtualMemoryHeap(UpdateCursorTest.java:178)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:88)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)

          Show
          Øystein Grøvlen added a comment - This test has been failing in the tinderbox test since it was checked in. The failure the first time after it was checked in was: 2) testVirtualMemoryHeap(org.apache.derbyTesting.functionTests.tests.lang.UpdateCursorTest)junit.framework.AssertionFailedError: Virtual memory heap test failed! Got unexpected value. expected:<202> but was:<103> at org.apache.derbyTesting.functionTests.tests.lang.UpdateCursorTest.testVirtualMemoryHeap(UpdateCursorTest.java:178) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:88) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25)
          Hide
          Ugo Matrangolo added a comment -

          I updated all the trunk code and restarted the test. It goes fine.

          I think that the failure is caused by the fact that the test is executed inside a suite where other tests can influence its environment.
          The failing test depends heavily on the derby.language.maxMemoryPerTable property. Changing this to a different value than 1
          fails the test.

          I'm investigating how this value can be influenced by other tests.

          Show
          Ugo Matrangolo added a comment - I updated all the trunk code and restarted the test. It goes fine. I think that the failure is caused by the fact that the test is executed inside a suite where other tests can influence its environment. The failing test depends heavily on the derby.language.maxMemoryPerTable property. Changing this to a different value than 1 fails the test. I'm investigating how this value can be influenced by other tests.
          Hide
          Ugo Matrangolo added a comment -

          Test fails when executed in a suite

          Show
          Ugo Matrangolo added a comment - Test fails when executed in a suite
          Hide
          Andrew McIntyre added a comment -

          I've commented out the failing testcase in the test until it can be fixed. A new JIRA issue has been filed for the testcase failure, please continue the discussion in DERBY-2543. Thanks,

          Show
          Andrew McIntyre added a comment - I've commented out the failing testcase in the test until it can be fixed. A new JIRA issue has been filed for the testcase failure, please continue the discussion in DERBY-2543 . Thanks,

            People

            • Assignee:
              Ugo Matrangolo
              Reporter:
              Ugo Matrangolo
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development