Derby
  1. Derby
  2. DERBY-2293

convert batchUpdate.java to junit

    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

      Convert the test jdbcapi.batchUpdate.java to junit framework

      1. DERBY-2293_20070205.diff
        150 kB
        Myrna van Lunteren
      2. DERBY-2293_20070205.stat
        0.6 kB
        Myrna van Lunteren
      3. DERBY-2293_20070206_2.diff
        155 kB
        Myrna van Lunteren
      4. DERBY-2293_20070206.diff
        154 kB
        Myrna van Lunteren
      5. DERBY-2293_20070206.stat
        0.9 kB
        Myrna van Lunteren
      6. DERBY-2293_20070208.diff
        61 kB
        Myrna van Lunteren
      7. DERBY-2293_20070209.diff
        62 kB
        Myrna van Lunteren

        Activity

        Gavin made changes -
        Workflow jira [ 12395805 ] Default workflow, editable Closed status [ 12800684 ]
        Hide
        Daniel John Debrunner added a comment -

        In trying to add the JDBCHarnessJavaTets I was hitting some failures in jdbcapi._Suite with the network server not coming up in sufficient time. In the past these have been due to tests not closing their statement objects. Cleaned up BatchUpdateTest by closing its statements. Committed revision 506082
        (ps. I hit the failures about 50% of the time running jdbcapi._Suite on windows, I still see these failures with the changes but it removes BatchUpdateTest as a possible culprit and is a good thing for the test).

        Show
        Daniel John Debrunner added a comment - In trying to add the JDBCHarnessJavaTets I was hitting some failures in jdbcapi._Suite with the network server not coming up in sufficient time. In the past these have been due to tests not closing their statement objects. Cleaned up BatchUpdateTest by closing its statements. Committed revision 506082 (ps. I hit the failures about 50% of the time running jdbcapi._Suite on windows, I still see these failures with the changes but it removes BatchUpdateTest as a possible culprit and is a good thing for the test).
        Myrna van Lunteren made changes -
        Status Open [ 1 ] Closed [ 6 ]
        Fix Version/s 10.3.0.0 [ 12310800 ]
        Resolution Fixed [ 1 ]
        Hide
        Myrna van Lunteren added a comment -

        committed all I intend to do on this with revision 505603. See: http://svn.apache.org/viewvc?view=rev&revision=505603.

        Show
        Myrna van Lunteren added a comment - committed all I intend to do on this with revision 505603. See: http://svn.apache.org/viewvc?view=rev&revision=505603 .
        Myrna van Lunteren made changes -
        Attachment DERBY-2293_20070209.diff [ 12350811 ]
        Hide
        Myrna van Lunteren added a comment -

        Attaching a patch that differs from the one from yesterday in that:

        • the decorator now creates tables using statements & there is a separate little fixture to test create tables in batch (called testMinimalDDLInBatch)
        • the decorator doesn't create its own statement but uses the one passed in
        • the setUp() now relies on the normal connection and sets AutoCommit to false
        • all superfluous getConnection().setAutocommit(false) statements have been removed from individual fixtures
        • all superfluous s.execute("delete from t1"), which is done in the setUp(), have been removed from individual fixtures.

        I tested just the jdbcapi._Suite again and all still works.
        I'll commit this soon.

        Show
        Myrna van Lunteren added a comment - Attaching a patch that differs from the one from yesterday in that: the decorator now creates tables using statements & there is a separate little fixture to test create tables in batch (called testMinimalDDLInBatch) the decorator doesn't create its own statement but uses the one passed in the setUp() now relies on the normal connection and sets AutoCommit to false all superfluous getConnection().setAutocommit(false) statements have been removed from individual fixtures all superfluous s.execute("delete from t1"), which is done in the setUp(), have been removed from individual fixtures. I tested just the jdbcapi._Suite again and all still works. I'll commit this soon.
        Hide
        Daniel John Debrunner added a comment -

        Comments on the decorateSQL() method.

        The method is designed to allow the test writer to just implement the SQL DDL and any other setup such as inserting rows. Thus it already puts the connection into auto commit false mode, provides a Statement object and commits the transaction for you. Thus an implementation of a decorateSQL method need not do those things. This is documented in the javadoc for CleanDatabaseTestSetup and CleanDatabaseTestSetup.decorateSQL. If the current documentation is unclear or could use more information, please improve it.

        Performing assertions in a setup method or a decorator setup method is not really the JUnit style. setUp methods are for setting up to run a test, the test method is for performing the asserts. If a decorator setUp fails then I think none of its fixtures will be run. If a test for DDL in a batch is needed then have an explicit testDDLInBatch method.

        An FYI on connection handling in BaseJDBCTestCase. The getConnection() method returns a single connection for the lifetime of an instance of the class (assuming the connection is not closed until super.tearDown() is called). Thus the connection used in the setUp() method is the same as the connection in the fixture method and the same as the one in the tearDown method. Thus, in this case, since the setUp method puts the connection in autocommit false mode, there is no need for each test fixture to also do it. Though it does not harm.
        The BaseJDBCTestCase also provides the abilty for a test class to put all of its connections in a consistent initial state, by having the test class implement the method initializeConnection. See the javadoc in BaseJDBCTestCase. Thus one could perform the setAutoCommit(false) in that method and then be assured that every connection used by a test was in auto commit false mode.

        Show
        Daniel John Debrunner added a comment - Comments on the decorateSQL() method. The method is designed to allow the test writer to just implement the SQL DDL and any other setup such as inserting rows. Thus it already puts the connection into auto commit false mode, provides a Statement object and commits the transaction for you. Thus an implementation of a decorateSQL method need not do those things. This is documented in the javadoc for CleanDatabaseTestSetup and CleanDatabaseTestSetup.decorateSQL. If the current documentation is unclear or could use more information, please improve it. Performing assertions in a setup method or a decorator setup method is not really the JUnit style. setUp methods are for setting up to run a test, the test method is for performing the asserts. If a decorator setUp fails then I think none of its fixtures will be run. If a test for DDL in a batch is needed then have an explicit testDDLInBatch method. An FYI on connection handling in BaseJDBCTestCase. The getConnection() method returns a single connection for the lifetime of an instance of the class (assuming the connection is not closed until super.tearDown() is called). Thus the connection used in the setUp() method is the same as the connection in the fixture method and the same as the one in the tearDown method. Thus, in this case, since the setUp method puts the connection in autocommit false mode, there is no need for each test fixture to also do it. Though it does not harm. The BaseJDBCTestCase also provides the abilty for a test class to put all of its connections in a consistent initial state, by having the test class implement the method initializeConnection. See the javadoc in BaseJDBCTestCase. Thus one could perform the setAutoCommit(false) in that method and then be assured that every connection used by a test was in auto commit false mode.
        Myrna van Lunteren made changes -
        Attachment DERBY-2293_20070208.diff [ 12350727 ]
        Hide
        Myrna van Lunteren added a comment -

        New patch that does the following:

        • does away with all passed on conns and stmts, renames all run* methods to test*, and identify them at the top of the test as fixtures.
        • reworked the suite(), and setUp() methods so that the tables are created once, and all records get deleted from t1 in the setUp.
        • removed the tearDown() (as a consequence not needed anymore)
        • removed the superfluous assertBatchExecuteError for preparedStatements.
          only difference was in the message, so adjusted that to be more generic.
        • tightened the try/catch blocks
        • catching BatchUpdateExceptions rather than SQLExceptions where applicable
        • suddenly t1 could get dropped in the two transaction methods/fixtures, so re-enabled them for network server.

        And removed the extra call in jdbcapi/_Suite.java. (Once is enough, I had put it in for testing purposes and had forgotten to remove it).

        Review is welcome.

        Show
        Myrna van Lunteren added a comment - New patch that does the following: does away with all passed on conns and stmts, renames all run* methods to test*, and identify them at the top of the test as fixtures. reworked the suite(), and setUp() methods so that the tables are created once, and all records get deleted from t1 in the setUp. removed the tearDown() (as a consequence not needed anymore) removed the superfluous assertBatchExecuteError for preparedStatements. only difference was in the message, so adjusted that to be more generic. tightened the try/catch blocks catching BatchUpdateExceptions rather than SQLExceptions where applicable suddenly t1 could get dropped in the two transaction methods/fixtures, so re-enabled them for network server. And removed the extra call in jdbcapi/_Suite.java. (Once is enough, I had put it in for testing purposes and had forgotten to remove it). Review is welcome.
        Myrna van Lunteren made changes -
        Derby Info [Patch Available]
        Hide
        Myrna van Lunteren added a comment -

        committed patch DERBY-2293_2007020_2.diff with revision 504641, see http://svn.apache.org/viewvc?view=rev&revision=504641.
        I will do a next pass to address Dan's review comments.

        Show
        Myrna van Lunteren added a comment - committed patch DERBY-2293 _2007020_2.diff with revision 504641, see http://svn.apache.org/viewvc?view=rev&revision=504641 . I will do a next pass to address Dan's review comments.
        Hide
        Daniel John Debrunner added a comment -

        Looks good in general, some potential cleanup, I think these could all be applied once this patch is committed.

        TestConfiguration.defaultSuite already adds CleanDatabaseTestSetup so there's no need to add another one.

        For the methods that take a connection, such as runStatementNonBatchStuffInBatch and only ever use the default connection it might be better to not have them pass a connection. They can use the utility method commit() or getConnection() if they need access to the connection. The code then becomes like the general style of other tests and it becomes obvious to the reader that the standard default connection is being used. Then of course the run methods start to look a lot like individual test fixtures, which they really are, so it might make sense to convert them them all the way to testXXX methods.

        There is no need to have a assertBatchExecuteError() method that takes a PreparedStatement. Since a PreparedStatement is a Statement and the executeBatch() method is defined on Statement, the code can use the assertBatchExecuteError that takes a Statement.

        In cases where you need to have a try/catch block like runTransactionErrorBatch() I think it is clearer and better if the try catch block only encloses the statement you expect to fail. So I think in this case it would be just the statement

        updateCount = stmt2.executeBatch();

        With the code as it is, if an earlier statement in the block incorrectly fails with a timeout then there is a chance the test will pass even though it should fail.

        As an alternative to:
        } catch (SQLException sqle)

        { /* Ensure the exception is time out while getting lock */ assertSQLState("40XL1",sqle); assertTrue("we should get a BatchUpdateException", (sqle instanceof BatchUpdateException)); one could just catch BatchUpdateException }

        catch (BatchUpdateException sqle) {
        /* Ensure the exception is time out while getting lock */
        assertSQLState("40XL1",sqle);

        Any non-batch exception will continue to fail the test.

        Show
        Daniel John Debrunner added a comment - Looks good in general, some potential cleanup, I think these could all be applied once this patch is committed. TestConfiguration.defaultSuite already adds CleanDatabaseTestSetup so there's no need to add another one. For the methods that take a connection, such as runStatementNonBatchStuffInBatch and only ever use the default connection it might be better to not have them pass a connection. They can use the utility method commit() or getConnection() if they need access to the connection. The code then becomes like the general style of other tests and it becomes obvious to the reader that the standard default connection is being used. Then of course the run methods start to look a lot like individual test fixtures, which they really are, so it might make sense to convert them them all the way to testXXX methods. There is no need to have a assertBatchExecuteError() method that takes a PreparedStatement. Since a PreparedStatement is a Statement and the executeBatch() method is defined on Statement, the code can use the assertBatchExecuteError that takes a Statement. In cases where you need to have a try/catch block like runTransactionErrorBatch() I think it is clearer and better if the try catch block only encloses the statement you expect to fail. So I think in this case it would be just the statement updateCount = stmt2.executeBatch(); With the code as it is, if an earlier statement in the block incorrectly fails with a timeout then there is a chance the test will pass even though it should fail. As an alternative to: } catch (SQLException sqle) { /* Ensure the exception is time out while getting lock */ assertSQLState("40XL1",sqle); assertTrue("we should get a BatchUpdateException", (sqle instanceof BatchUpdateException)); one could just catch BatchUpdateException } catch (BatchUpdateException sqle) { /* Ensure the exception is time out while getting lock */ assertSQLState("40XL1",sqle); Any non-batch exception will continue to fail the test.
        Myrna van Lunteren made changes -
        Derby Info [Patch Available]
        Myrna van Lunteren made changes -
        Attachment DERBY-2293_20070206_2.diff [ 12350528 ]
        Hide
        Myrna van Lunteren added a comment -

        _20070206_2.diff is the absolute latest cut. I think I added a slightly older one before.

        Show
        Myrna van Lunteren added a comment - _20070206_2.diff is the absolute latest cut. I think I added a slightly older one before.
        Myrna van Lunteren made changes -
        Attachment DERBY-2293_20070206.stat [ 12350526 ]
        Attachment DERBY-2293_20070206.diff [ 12350527 ]
        Hide
        Myrna van Lunteren added a comment -

        I'm attaching an updated patch.
        I've tried to address the most of the comments (Thx for the review!):

        • limited the length of text (and code too, for good measure)
        • filtered out repeating code to an assertBatchExecuteError method
        • renamed verifyBatchUpdateCounts to assertBatchUpdateCounts
        • reworded comment containing resulti
        • added license
        • set lock time out
        • revamped sections to use " new int[] {1,1,1}

          " constructs to make expected values easier to recognize

        • corrected use of same connection in transaction tests
        • made test work with network server as well as embedded
          This last one was not trivial because network server continues after an error,
          whereas embedded stops processing. So I've added if (usingEmbedded()
          and usingDerbyNetClient() sections, but seeing how little batch testing was
          occurring with networkserver/client, I thought it was worth it.

        Also updated my tree so I could remove the original test from Dan's JavaHarnessAdapter.

        I think that's it.

        Test passes by itself using junit.textui.TestRunner. I will run suites.All.

        Reviews are welcome.

        Show
        Myrna van Lunteren added a comment - I'm attaching an updated patch. I've tried to address the most of the comments (Thx for the review!): limited the length of text (and code too, for good measure) filtered out repeating code to an assertBatchExecuteError method renamed verifyBatchUpdateCounts to assertBatchUpdateCounts reworded comment containing resulti added license set lock time out revamped sections to use " new int[] {1,1,1} " constructs to make expected values easier to recognize corrected use of same connection in transaction tests made test work with network server as well as embedded This last one was not trivial because network server continues after an error, whereas embedded stops processing. So I've added if (usingEmbedded() and usingDerbyNetClient() sections, but seeing how little batch testing was occurring with networkserver/client, I thought it was worth it. Also updated my tree so I could remove the original test from Dan's JavaHarnessAdapter. I think that's it. Test passes by itself using junit.textui.TestRunner. I will run suites.All. Reviews are welcome.
        Hide
        Daniel John Debrunner added a comment -

        Krustian> Maybe you could even rename it to something like 'assertBatchUpdateCounts' to clearly signal the intention.

        +1

        'assert' is the correct terminology and will follow the standard JUnit pattern, thus be understandable by most folks.

        Show
        Daniel John Debrunner added a comment - Krustian> Maybe you could even rename it to something like 'assertBatchUpdateCounts' to clearly signal the intention. +1 'assert' is the correct terminology and will follow the standard JUnit pattern, thus be understandable by most folks.
        Hide
        Kristian Waagan added a comment -

        Wow, this test was big! Good work on converting it

        Regarding your worry about the try/catch and no exception, maybe you can use fail("Some informative message")?
        This way you don't have to set a flag and check it later.

        Personally, I think using 'verifyBatchUpdateCounts' looks better. Maybe you could even rename it to something like 'assertBatchUpdateCounts' to clearly signal the intention. If needed, it could easily be moved into a super/utility class.

        Comments/questions to BatchUpdateTest.java:
        a) Missing header/licence.
        b) testStatementBatchUpdateNegative: the two calls to getConnection will return the same connection. Is this intended?

        I have a few nits as well:
        1) There are a few tabs in the diff.
        2) Typo in the class JavaDoc: fo -> of
        3) Lines longer than 80 characters.
        4) resulti -> result in verifyBatchUpdateCounts
        5) Out of curiosity (first catch in runStatementWithResultSetBatch):
        if (updateCount != null)

        { assertEquals( "Select is the first statement in the batch, so there should not be any update count", 0, updateCount.length); }

        Can we assert null/not null here instead, or does this vary with the exception?

        If this test had been written from scratch, I would have split it up into more independent fixtures/test methods. Since it has been converted, and because of its size, I think the current approach is okay.
        The point of splitting it up, is to better isolate the thing that is failing (and to see that other things are not failing).

        The class is big, and I had to stop reviewing somewhere in the middle. There is more to be reviewed for the willing

        With 'DERBY-2293_20070205.diff ' I get this result:
        Tests run: 10, Failures: 4, Errors: 1

        Show
        Kristian Waagan added a comment - Wow, this test was big! Good work on converting it Regarding your worry about the try/catch and no exception, maybe you can use fail("Some informative message")? This way you don't have to set a flag and check it later. Personally, I think using 'verifyBatchUpdateCounts' looks better. Maybe you could even rename it to something like 'assertBatchUpdateCounts' to clearly signal the intention. If needed, it could easily be moved into a super/utility class. Comments/questions to BatchUpdateTest.java: a) Missing header/licence. b) testStatementBatchUpdateNegative: the two calls to getConnection will return the same connection. Is this intended? I have a few nits as well: 1) There are a few tabs in the diff. 2) Typo in the class JavaDoc: fo -> of 3) Lines longer than 80 characters. 4) resulti -> result in verifyBatchUpdateCounts 5) Out of curiosity (first catch in runStatementWithResultSetBatch): if (updateCount != null) { assertEquals( "Select is the first statement in the batch, so there should not be any update count", 0, updateCount.length); } Can we assert null/not null here instead, or does this vary with the exception? If this test had been written from scratch, I would have split it up into more independent fixtures/test methods. Since it has been converted, and because of its size, I think the current approach is okay. The point of splitting it up, is to better isolate the thing that is failing (and to see that other things are not failing). The class is big, and I had to stop reviewing somewhere in the middle. There is more to be reviewed for the willing With ' DERBY-2293 _20070205.diff ' I get this result: Tests run: 10, Failures: 4, Errors: 1
        Hide
        Myrna van Lunteren added a comment -

        It seemed to me the current assertStatementError implementations expect a (prepared/callable/)statement to execute() (or executeUpdate()). But executeBatch() cannot be executed that way.

        I guess I could make a assertBatchExecuteError, but I thought it would not be very widely applicable. Now I think it makes sense to create one, if only in this test. I'll modify the test. If it turns out the method is more widely useable, it can be moved into one of the junit super/utility/setup classes later.

        Show
        Myrna van Lunteren added a comment - It seemed to me the current assertStatementError implementations expect a (prepared/callable/)statement to execute() (or executeUpdate()). But executeBatch() cannot be executed that way. I guess I could make a assertBatchExecuteError, but I thought it would not be very widely applicable. Now I think it makes sense to create one, if only in this test. I'll modify the test. If it turns out the method is more widely useable, it can be moved into one of the junit super/utility/setup classes later.
        Hide
        Daniel John Debrunner added a comment -

        Mryna> I could not use the BaseJDBCTestCase.assertStatementError(..) method for the Batch statements, so I maintained the try-catch blocks.

        What's the issue with not being able to use assertStatementError?
        Maybe there's a new utility assert method you could add so that other tests could benefit?

        Show
        Daniel John Debrunner added a comment - Mryna> I could not use the BaseJDBCTestCase.assertStatementError(..) method for the Batch statements, so I maintained the try-catch blocks. What's the issue with not being able to use assertStatementError? Maybe there's a new utility assert method you could add so that other tests could benefit?
        Myrna van Lunteren made changes -
        Derby Info [Patch Available]
        Hide
        Myrna van Lunteren added a comment -

        oops, I must have not built after my last change. The test in the patch fails...Fixing...

        Show
        Myrna van Lunteren added a comment - oops, I must have not built after my last change. The test in the patch fails...Fixing...
        Myrna van Lunteren made changes -
        Field Original Value New Value
        Attachment DERBY-2293_20070205.stat [ 12350363 ]
        Attachment DERBY-2293_20070205.diff [ 12350364 ]
        Hide
        Myrna van Lunteren added a comment -

        Attaching a patch - first attempt to convert the test jdbcapi/batchUpdate.java.

        The new test jdbcapi/BatchUpdateTest.java is not quite ready for commit...

        I'd like reviewers especially to give input on:

        • There is a method, verifyBatchUpdateCounts, which takes 2 arrays, and compares both the length and the values resulting from the *.executeBatch() statements.
          However, in most of the test I used a two-step assertEquals approach that is closer to what was done in the original test, it was easier to put in that way. But which approach is easier to the reader?
        • I could not use the BaseJDBCTestCase.assertStatementError(..) method for the Batch statements, so I maintained the try-catch blocks. However, I worry that if there is no Exception, there is no assert. The original test put statements like this: passed=false; below the statements expecting to fail, and that value gets evaluated later. Is that the approach I should take?
        Show
        Myrna van Lunteren added a comment - Attaching a patch - first attempt to convert the test jdbcapi/batchUpdate.java. The new test jdbcapi/BatchUpdateTest.java is not quite ready for commit... I'd like reviewers especially to give input on: There is a method, verifyBatchUpdateCounts, which takes 2 arrays, and compares both the length and the values resulting from the *.executeBatch() statements. However, in most of the test I used a two-step assertEquals approach that is closer to what was done in the original test, it was easier to put in that way. But which approach is easier to the reader? I could not use the BaseJDBCTestCase.assertStatementError(..) method for the Batch statements, so I maintained the try-catch blocks. However, I worry that if there is no Exception, there is no assert. The original test put statements like this: passed=false; below the statements expecting to fail, and that value gets evaluated later. Is that the approach I should take?
        Myrna van Lunteren created issue -

          People

          • Assignee:
            Myrna van Lunteren
            Reporter:
            Myrna van Lunteren
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development