Derby
  1. Derby
  2. DERBY-2283

convert lang/currentof.java test to junit test

    Details

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

      Description

      Using this Jira entry to attach the converted test in future

      1. _Suite.java
        3 kB
        Manjula Kutty
      2. CurrentOfTest.java
        10 kB
        Manjula Kutty
      3. DERBY-2283_diff_03_16.txt
        5 kB
        Manjula Kutty
      4. DERBY-2283_stat_03_16.txt
        0.2 kB
        Manjula Kutty
      5. jira_2283_.diff.txt
        7 kB
        Manjula Kutty
      6. jira_2283_02_08_diff.txt
        30 kB
        Manjula Kutty
      7. jira_2283_02_08_stat.txt
        0.4 kB
        Manjula Kutty
      8. jira_2283_stat.txt
        0.2 kB
        Manjula Kutty
      9. svn_diff_02_02.txt
        10 kB
        Manjula Kutty
      10. svn_stat_02_02.txt
        0.4 kB
        Manjula Kutty

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Closed Closed
        27d 1h 40m 1 Manjula Kutty 28/Feb/07 22:23
        Closed Closed Reopened Reopened
        2d 2h 11m 1 Daniel John Debrunner 03/Mar/07 00:35
        Reopened Reopened Resolved Resolved
        18d 18h 16m 1 Manjula Kutty 21/Mar/07 18:52
        Resolved Resolved Closed Closed
        4h 20m 1 Manjula Kutty 21/Mar/07 23:12
        Gavin made changes -
        Workflow jira [ 12395581 ] Default workflow, editable Closed status [ 12798230 ]
        Dag H. Wanvik made changes -
        Issue Type Test [ 6 ] Improvement [ 4 ]
        Kathey Marsden made changes -
        Component/s Test [ 11413 ]
        Manjula Kutty made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Manjula Kutty made changes -
        Resolution Fixed [ 1 ]
        Status Reopened [ 4 ] Resolved [ 5 ]
        Hide
        Manjula Kutty added a comment -

        Thanks for the commit Kathey

        Show
        Manjula Kutty added a comment - Thanks for the commit Kathey
        Kathey Marsden made changes -
        Derby Info [Patch Available]
        Hide
        Kathey Marsden added a comment -

        committed revision 520662 for this issue with patch DERBY-2283_diff_03_16.txt

        Show
        Kathey Marsden added a comment - committed revision 520662 for this issue with patch DERBY-2283 _diff_03_16.txt
        Manjula Kutty made changes -
        Derby Info [Patch Available]
        Manjula Kutty made changes -
        Attachment DERBY-2283_stat_03_16.txt [ 12353537 ]
        Attachment DERBY-2283_diff_03_16.txt [ 12353538 ]
        Hide
        Manjula Kutty added a comment -

        Attaching the patch which will validate the rows to be deleted and updated. Please review and if looks fine please commit

        Show
        Manjula Kutty added a comment - Attaching the patch which will validate the rows to be deleted and updated. Please review and if looks fine please commit
        Hide
        Manjula Kutty added a comment -

        Will be attaching a new patch to address this issue

        Show
        Manjula Kutty added a comment - Will be attaching a new patch to address this issue
        Daniel John Debrunner made changes -
        Resolution Fixed [ 1 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Hide
        Daniel John Debrunner added a comment -

        Test needs to ensure that the UPDATE and DELETE statements are updating and deleting the correct rows and for update to the correct values.

        Show
        Daniel John Debrunner added a comment - Test needs to ensure that the UPDATE and DELETE statements are updating and deleting the correct rows and for update to the correct values.
        Hide
        Daniel John Debrunner added a comment -

        I was going to add a test case for a change in the cursor statement to this test but then I realized that the fixture testbug4395(0 was doing exactly that though its name did not indicate that and its javadoc comments did not describe what it is testing. I cleaned it up with revision 513967.

        Looking in detail at the test I see that even though it is testing UPDATE and DELETE WHERE CURRENT OF it only checks that the update counts are correct after an execution, not that the right values were updated or the right row deleted. I think this is because the old test would have tested the output be comparing it to the master file, something to watch out for when converting tests.

        Also the testUpdate() has a whole set of tests that are testing if a read only statement results in a null cursor name. These tests are not specific to UPDATE so ideally would be in their own fixture.

        Show
        Daniel John Debrunner added a comment - I was going to add a test case for a change in the cursor statement to this test but then I realized that the fixture testbug4395(0 was doing exactly that though its name did not indicate that and its javadoc comments did not describe what it is testing. I cleaned it up with revision 513967. Looking in detail at the test I see that even though it is testing UPDATE and DELETE WHERE CURRENT OF it only checks that the update counts are correct after an execution, not that the right values were updated or the right row deleted. I think this is because the old test would have tested the output be comparing it to the master file, something to watch out for when converting tests. Also the testUpdate() has a whole set of tests that are testing if a read only statement results in a null cursor name. These tests are not specific to UPDATE so ideally would be in their own fixture.
        Manjula Kutty made changes -
        Status Open [ 1 ] Closed [ 6 ]
        Resolution Fixed [ 1 ]
        Hide
        Manjula Kutty added a comment -

        Closing this issue

        Show
        Manjula Kutty added a comment - Closing this issue
        Hide
        Andrew McIntyre added a comment -

        Committed jira_2283_02_08_diff.txt with revision 505131.

        Show
        Andrew McIntyre added a comment - Committed jira_2283_02_08_diff.txt with revision 505131.
        Manjula Kutty made changes -
        Attachment jira_2283_02_08_diff.txt [ 12350710 ]
        Attachment jira_2283_02_08_stat.txt [ 12350711 ]
        Hide
        Manjula Kutty added a comment -

        Adding patches for the removal of old test files. The following files are deleted

        java/testing/org/apache/derbyTesting/functionTests/tests/lang/currentof.java
        java/testing/org/apache/derbyTesting/functionTests/tests/lang/currentof_derby.properties
        java/testing/org/apache/derbyTesting/functionTests/master/currentof.out

        Also removed the currentof.java from derbylang.runall file

        Please review it and if every thing looks good, can any one please commit this patch?

        Show
        Manjula Kutty added a comment - Adding patches for the removal of old test files. The following files are deleted java/testing/org/apache/derbyTesting/functionTests/tests/lang/currentof.java java/testing/org/apache/derbyTesting/functionTests/tests/lang/currentof_derby.properties java/testing/org/apache/derbyTesting/functionTests/master/currentof.out Also removed the currentof.java from derbylang.runall file Please review it and if every thing looks good, can any one please commit this patch?
        Daniel John Debrunner made changes -
        Derby Info [Patch Available]
        Hide
        Daniel John Debrunner added a comment -

        Patch committed - made two minor changes:
        1) Moved the class comment to before the class definition so it is a javadoc comment for the class
        2) renamed the suite to match the test name
        Thanks Manjula.

        Show
        Daniel John Debrunner added a comment - Patch committed - made two minor changes: 1) Moved the class comment to before the class definition so it is a javadoc comment for the class 2) renamed the suite to match the test name Thanks Manjula.
        Manjula Kutty made changes -
        Attachment jira_2283_stat.txt [ 12350604 ]
        Attachment jira_2283_.diff.txt [ 12350603 ]
        Hide
        Manjula Kutty added a comment -

        I did svn delete to currentof.java and currentof_derby.properties. Ran the lang suite in the old harness and found some failures. None are related to currentof.java removal. So attaching the modified test patch . I added some more assert methods for client/server mode, but have some problem, so making the test to run only in embedded mode. Please review

        Show
        Manjula Kutty added a comment - I did svn delete to currentof.java and currentof_derby.properties. Ran the lang suite in the old harness and found some failures. None are related to currentof.java removal. So attaching the modified test patch . I added some more assert methods for client/server mode, but have some problem, so making the test to run only in embedded mode. Please review
        Hide
        Daniel John Debrunner added a comment -

        Patch Committed revision 502769 - Thanks Manjula.

        The test is lacking javadoc comments on the class and fixture methods, if you gained any knowledge during the conversion it would be great if you could find time to save that knowledge as comments. Even an overview in the class javadoc of what the test is covering would be good.

        I think the remaining try/catch blocks could all use the JDBC.assertStatementError() utility method, thus cleaning up the test even more.

        In earlier comments I think you had said that this is the type of language test that should run in client server as well, but it's only setup to run embedded. This would make it work in both:

        public static Test suite()

        { return TestConfiguration.defaultSuite(CurrentOfTest.class); }
        Show
        Daniel John Debrunner added a comment - Patch Committed revision 502769 - Thanks Manjula. The test is lacking javadoc comments on the class and fixture methods, if you gained any knowledge during the conversion it would be great if you could find time to save that knowledge as comments. Even an overview in the class javadoc of what the test is covering would be good. I think the remaining try/catch blocks could all use the JDBC.assertStatementError() utility method, thus cleaning up the test even more. In earlier comments I think you had said that this is the type of language test that should run in client server as well, but it's only setup to run embedded. This would make it work in both: public static Test suite() { return TestConfiguration.defaultSuite(CurrentOfTest.class); }
        Hide
        Mamta A. Satoor added a comment -

        Manjula, I just have one comment. Should the setUp() method be calling it's super's implementation (ie super.setUp) like the tearDown() method?

        Show
        Mamta A. Satoor added a comment - Manjula, I just have one comment. Should the setUp() method be calling it's super's implementation (ie super.setUp) like the tearDown() method?
        Hide
        Mamta A. Satoor added a comment -

        Manjula, just an FYI and it is not a biggie but if you use svn stat -q, then you won't get following kind of output.
        ? test
        ? .project
        ? system
        ? .externalToolBuilders
        ? .classpath
        ? svn_stat_02_02
        ? logs
        ? svn_diff_02_02.txt

        Show
        Mamta A. Satoor added a comment - Manjula, just an FYI and it is not a biggie but if you use svn stat -q, then you won't get following kind of output. ? test ? .project ? system ? .externalToolBuilders ? .classpath ? svn_stat_02_02 ? logs ? svn_diff_02_02.txt
        Manjula Kutty made changes -
        Attachment svn_stat_02_02.txt [ 12350254 ]
        Attachment svn_diff_02_02.txt [ 12350253 ]
        Hide
        Manjula Kutty added a comment -

        I changed the test to include Dan's comments. I'm attaching the diff and stat files. Please review it and let me know if you have any comments/questions/suggestions

        Show
        Manjula Kutty added a comment - I changed the test to include Dan's comments. I'm attaching the diff and stat files. Please review it and let me know if you have any comments/questions/suggestions
        Hide
        Daniel John Debrunner added a comment -

        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?

        Show
        Daniel John Debrunner added a comment - 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?
        Manjula Kutty made changes -
        Summary convert lang/currentof.java test junit convert lang/currentof.java test to junit test
        Manjula Kutty made changes -
        Derby Info [Patch Available]
        Manjula Kutty made changes -
        Field Original Value New Value
        Attachment CurrentOfTest.java [ 12350179 ]
        Attachment _Suite.java [ 12350180 ]
        Hide
        Manjula Kutty added a comment -

        Attaching the CurrentOfTest.java for review. I ran the lang Suite and one test failed. It was UpdatableResultSetTest which timed out during getting a n/w server connection. This is my first attempt in conversion of tests to junit.

        Show
        Manjula Kutty added a comment - Attaching the CurrentOfTest.java for review. I ran the lang Suite and one test failed. It was UpdatableResultSetTest which timed out during getting a n/w server connection. This is my first attempt in conversion of tests to junit.
        Manjula Kutty created issue -

          People

          • Assignee:
            Manjula Kutty
            Reporter:
            Manjula Kutty
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development