Derby
  1. Derby
  2. DERBY-5382

Convert existing harness recovery tests to JUnit tests

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.9.1.0
    • Component/s: Test
    • Labels:
      None
    • Urgency:
      Normal
    • Issue & fix info:
      Patch Available

      Description

      Existing harness recovery tests need to be converted to JUnit tests. A framework as designed in Derby-4249 can be used for this purpose.

      Tests to be converted:
      a) oc_rec1
      b) oc_rec2
      c) oc_rec3
      d) oc_rec4

      These recovery tests run in coordination. The test oc_rec1 creates a table, inserts and then deletes rows from it and commit it which is followed by a series of insertion of rows in the existing table in oc_rec2, oc_rec3 and oc_rec4. The tests oc_rec2 and oc_3 also create table and insert, delete, compress rows in it and leave the table thus produced in committed or uncommitted state which is tested by the next corresponding test (oc_rec3 for oc_rec2, oc_rec4 for oc_rec3) for recovery.

      1. d5382.patch
        31 kB
        Siddharth Srivastava
      2. DERBY-5382.diff
        3 kB
        Myrna van Lunteren
      3. DERBY-5382_2.diff
        5 kB
        Myrna van Lunteren

        Activity

        Hide
        Siddharth Srivastava added a comment -

        Attached is the patch for this issue. Please review it.

        Show
        Siddharth Srivastava added a comment - Attached is the patch for this issue. Please review it.
        Hide
        Siddharth Srivastava added a comment -

        Hi

        Just wanted to share some observation while writing this test (which went automagically)

        I receoved the following errors:

        1) A lock could not be obtained within the time requested
        I see that it was resolved in Derby-4273. But the consistently nagged me throughout the day. As a hack I used Thread.interrupted() while committing changes.
        Problem ceased to exist after this, but later when I rebuilt the same code without Thread.interrupted(), all went fine.

        2) SQLNonTransientConnectionException: No current connection.. again after dealing with it for hours (tried System.gc() and everything else), it also went automagically.
        If I remember correctly, there was a Jira issue related to this, but it also mentioned somewhere that this error is not reproducible frequently.

        3) This test initially failed when I used
        String tableName = "RecTest", but when I used String tableName = "RECTEST1", the error again went away.
        I guess it is something like the discussion in Derby-437.
        May be a reference in the doc(may be a footer explaining non delimited and delimited identifiers), would be good.

        Can anyone familiar with these errors, help me in understanding them better(or reproducing them), so that I can get some insight that it was due to some system issues(I have been facing strange file permission issues within my local derby repo) or some fault in the patch.

        Thanks

        Show
        Siddharth Srivastava added a comment - Hi Just wanted to share some observation while writing this test (which went automagically) I receoved the following errors: 1) A lock could not be obtained within the time requested I see that it was resolved in Derby-4273. But the consistently nagged me throughout the day. As a hack I used Thread.interrupted() while committing changes. Problem ceased to exist after this, but later when I rebuilt the same code without Thread.interrupted(), all went fine. 2) SQLNonTransientConnectionException: No current connection.. again after dealing with it for hours (tried System.gc() and everything else), it also went automagically. If I remember correctly, there was a Jira issue related to this, but it also mentioned somewhere that this error is not reproducible frequently. 3) This test initially failed when I used String tableName = "RecTest", but when I used String tableName = "RECTEST1", the error again went away. I guess it is something like the discussion in Derby-437. May be a reference in the doc(may be a footer explaining non delimited and delimited identifiers), would be good. Can anyone familiar with these errors, help me in understanding them better(or reproducing them), so that I can get some insight that it was due to some system issues(I have been facing strange file permission issues within my local derby repo) or some fault in the patch. Thanks
        Hide
        Kathey Marsden added a comment -

        Marking patch available. It looks like Siddharth submitted a patch some time ago that needs to get on the radar.

        Show
        Kathey Marsden added a comment - Marking patch available. It looks like Siddharth submitted a patch some time ago that needs to get on the radar.
        Hide
        Myrna van Lunteren added a comment -

        I tried the patch, and I'll have a closer look too, but from first glance I have the following comments:

        • The old test files were removed, but they're still referenced in the functionTests.suites.storerecovery.runall file.
        • the masters for the original tests were not removed
        • When I ran the test, I got an error - perhaps the conversion has gone out of sync with the test. I'll investigate, unless Siddarth is still actively working on this?
          This is the error I saw:
          ------------------
          1) testOCRecovery_1(org.apache.derbyTesting.functionTests.tests.store.OCRecoveryTest)
          junit.framework.AssertionFailedError: expectedExitValue:0 does not match exitValue:1
          expected output strings:
          [0]OK (1 test)
          actual output:<STDOUT> <END STDOUT>
          <STDERR>Class not found "org.apache.derbyTesting.functionTests.tests.store.OCRecoveryTest.launchOCRecovery_2"
          <END STDERR>
          expected:<0> but was:<1>
          at org.apache.derbyTesting.junit.BaseTestCase.assertExecJavaCmdAsExpected(BaseTestCase.java:513)
          at org.apache.derbyTesting.junit.BaseTestCase.assertLaunchedJUnitTestMethod(BaseTestCase.java:838)
          at org.apache.derbyTesting.functionTests.tests.store.OCRecoveryTest.testOCRecovery_1(OCRecoveryTest.java:78)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:60)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:37)
          at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:113)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:19)
          at junit.extensions.TestSetup.run(TestSetup.java:23)
          at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57
          )
        Show
        Myrna van Lunteren added a comment - I tried the patch, and I'll have a closer look too, but from first glance I have the following comments: The old test files were removed, but they're still referenced in the functionTests.suites.storerecovery.runall file. the masters for the original tests were not removed When I ran the test, I got an error - perhaps the conversion has gone out of sync with the test. I'll investigate, unless Siddarth is still actively working on this? This is the error I saw: ------------------ 1) testOCRecovery_1(org.apache.derbyTesting.functionTests.tests.store.OCRecoveryTest) junit.framework.AssertionFailedError: expectedExitValue:0 does not match exitValue:1 expected output strings: [0] OK (1 test) actual output:<STDOUT> <END STDOUT> <STDERR>Class not found "org.apache.derbyTesting.functionTests.tests.store.OCRecoveryTest.launchOCRecovery_2" <END STDERR> expected:<0> but was:<1> at org.apache.derbyTesting.junit.BaseTestCase.assertExecJavaCmdAsExpected(BaseTestCase.java:513) at org.apache.derbyTesting.junit.BaseTestCase.assertLaunchedJUnitTestMethod(BaseTestCase.java:838) at org.apache.derbyTesting.functionTests.tests.store.OCRecoveryTest.testOCRecovery_1(OCRecoveryTest.java:78) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:60) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:37) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:113) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22) at junit.extensions.TestSetup$1.protect(TestSetup.java:19) at junit.extensions.TestSetup.run(TestSetup.java:23) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57 )
        Hide
        Myrna van Lunteren added a comment -

        I've looked at that failure a bit closer, and realized I was picking up an old version (3.8.1) of junit.
        The test passes with junit 3.8.2, (which allows for the -m option to run fixture/methods).

        I think the test conversion is good as it is.

        I ran the test by itself a couple of times, and in the store suite, and I did not see any of the problems Siddarth mentioned, so I am assuming they're related to environment issues, or issues that were current at the time the original patch was created.

        I did remove the reference from storetests.runall, deleted the master, and made a few minor changes:

        • using setAutoCommit() and createStatement() instead of creating a connection first.
          This was already done in the createAndLoadTable() method, but not in the launchOCRecovery_* methods, and I didn't see a reason for not doing this elsewhere.
        • remove the System.out.println("in test 1") which seemed to me a left-over from a debugging action.
        • broke up a few lines that went > 80 characters.
          (the coding practice for derby is comments not to go longer, but it doesn't hurt for code
          to follow the same rule).

        I committed the patch (with my modifications) with revision 1292027.

        Thank you Siddarth!

        Show
        Myrna van Lunteren added a comment - I've looked at that failure a bit closer, and realized I was picking up an old version (3.8.1) of junit. The test passes with junit 3.8.2, (which allows for the -m option to run fixture/methods). I think the test conversion is good as it is. I ran the test by itself a couple of times, and in the store suite, and I did not see any of the problems Siddarth mentioned, so I am assuming they're related to environment issues, or issues that were current at the time the original patch was created. I did remove the reference from storetests.runall, deleted the master, and made a few minor changes: using setAutoCommit() and createStatement() instead of creating a connection first. This was already done in the createAndLoadTable() method, but not in the launchOCRecovery_* methods, and I didn't see a reason for not doing this elsewhere. remove the System.out.println("in test 1") which seemed to me a left-over from a debugging action. broke up a few lines that went > 80 characters. (the coding practice for derby is comments not to go longer, but it doesn't hurt for code to follow the same rule). I committed the patch (with my modifications) with revision 1292027. Thank you Siddarth!
        Hide
        Mike Matrigali added a comment -

        I am having a hard time following the logic in the test, not many comments. This test needs a non clean shutdown to test what it is trying to exercise. It did this simply by exiting the process without calling clean shutdown.

        Does the following result in a clean shutdown or a "dirty" one:
        callCompress(tableName, true, true, true, true);
        TestConfiguration.getCurrent().shutdownDatabase();
        st.close();

        Show
        Mike Matrigali added a comment - I am having a hard time following the logic in the test, not many comments. This test needs a non clean shutdown to test what it is trying to exercise. It did this simply by exiting the process without calling clean shutdown. Does the following result in a clean shutdown or a "dirty" one: callCompress(tableName, true, true, true, true); TestConfiguration.getCurrent().shutdownDatabase(); st.close();
        Hide
        Myrna van Lunteren added a comment -

        The test isn't doing the same as the original - thanks for noticing, Mike.

        Show
        Myrna van Lunteren added a comment - The test isn't doing the same as the original - thanks for noticing, Mike.
        Hide
        Myrna van Lunteren added a comment -

        I'm uploading a patch in which the shutdowns have been removed, and the first step is also put into a launched method.

        Show
        Myrna van Lunteren added a comment - I'm uploading a patch in which the shutdowns have been removed, and the first step is also put into a launched method.
        Hide
        Mike Matrigali added a comment -

        comments on reviewing latest patch. This change is definitely an improvement, but still does not match what the original test
        was doing. The best case would be if this test created a new database in the first forked call and did the work below, then the
        exact work would be tested. But not sure if current harness can do that. I would suggest first making the changes below and
        checking in, then work on executing in a separate db. Maybe others can help with the separate db issue. If you don't want to deal
        with comment issues, let me know and I can add some after you check in - i don't want to conflict at this point.

        In the original test the following actions are performed in oc_rec1, and then a hard ("non clean shutdown") is performed.
        beginTest(conn, test_name);
        createAndLoadTable(conn, true, table_name, 5000, 0);
        executeQuery(conn, "delete from " + table_name, true);
        callCompress(conn, "APP", table_name, true, true, true, true);
        endTest(conn, test_name);

        The important part of this is that the create, delete, and compress log records will all be processed by restart recovery when oc_rec2
        is executed. And also of interest is all the work to create the database also is in the log of unclean shutdown, this also presents
        interesting code paths for restart recovery.

        In the current patch the createAndLoadTable(tableName, true, 5000, 0) is done and then a clean shutdown.

        If you can't figure out a way to have a new database created in the forked process, then please log an issue for adding harness support
        for that, as it would be good to be able to test restart recovery on a newly created db.

        As a good incremental work around for your test I would move the createAndLoadTable(tableName, true, 5000, 0); call from
        testOCRecovery_1() to launchOCRecovery_1(). This still will not result in the same codepaths tested, but it will be closer. Maybe add
        in a extra create table of some dummy table, to make the session not read only. At least
        the clean shutdown with no work will lay down a checkpoint and make it less likely that dependent on previous tests that a checkpoint
        might happen during one of the phases and then we would skip the restart recovery work that we are trying to test.

        launchOCRecovery_2() is missing a few comments that are in oc_rec2()

        comment in launchOCRecovery_3() is wrong (it was wrong in orignal also)
        // setup to test redo recovery on:
        // add more rows, delete rows, compress, add more, no commit

        It should be:
        // setup to test redo recovery on:
        // add more rows, delete rows, commit, compress, no commit

        launchOCRecovery_4 is missing comments from oc_rec4

        Show
        Mike Matrigali added a comment - comments on reviewing latest patch. This change is definitely an improvement, but still does not match what the original test was doing. The best case would be if this test created a new database in the first forked call and did the work below, then the exact work would be tested. But not sure if current harness can do that. I would suggest first making the changes below and checking in, then work on executing in a separate db. Maybe others can help with the separate db issue. If you don't want to deal with comment issues, let me know and I can add some after you check in - i don't want to conflict at this point. In the original test the following actions are performed in oc_rec1, and then a hard ("non clean shutdown") is performed. beginTest(conn, test_name); createAndLoadTable(conn, true, table_name, 5000, 0); executeQuery(conn, "delete from " + table_name, true); callCompress(conn, "APP", table_name, true, true, true, true); endTest(conn, test_name); The important part of this is that the create, delete, and compress log records will all be processed by restart recovery when oc_rec2 is executed. And also of interest is all the work to create the database also is in the log of unclean shutdown, this also presents interesting code paths for restart recovery. In the current patch the createAndLoadTable(tableName, true, 5000, 0) is done and then a clean shutdown. If you can't figure out a way to have a new database created in the forked process, then please log an issue for adding harness support for that, as it would be good to be able to test restart recovery on a newly created db. As a good incremental work around for your test I would move the createAndLoadTable(tableName, true, 5000, 0); call from testOCRecovery_1() to launchOCRecovery_1(). This still will not result in the same codepaths tested, but it will be closer. Maybe add in a extra create table of some dummy table, to make the session not read only. At least the clean shutdown with no work will lay down a checkpoint and make it less likely that dependent on previous tests that a checkpoint might happen during one of the phases and then we would skip the restart recovery work that we are trying to test. launchOCRecovery_2() is missing a few comments that are in oc_rec2() comment in launchOCRecovery_3() is wrong (it was wrong in orignal also) // setup to test redo recovery on: // add more rows, delete rows, compress, add more, no commit It should be: // setup to test redo recovery on: // add more rows, delete rows, commit, compress, no commit launchOCRecovery_4 is missing comments from oc_rec4
        Hide
        Myrna van Lunteren added a comment -

        I adjusted the test as per Mike's comments - so now all steps are in forkable methods except the creation of the database.
        I committed these changes with revision 1293028 with the following comment:
        follow up patch, which

        • removes shutdowns which were preventing any recovery to happen
        • makes the first step a separate forkable method
        • adds some comments

        Note, that the comments in oc_rec2 didn't exactly reflect the steps, it said:
        'create table, delete rows, compress, add rows, commit' but there were no rows getting added after the compress. So I removed the 'add rows' from the comment in the new test.

        Show
        Myrna van Lunteren added a comment - I adjusted the test as per Mike's comments - so now all steps are in forkable methods except the creation of the database. I committed these changes with revision 1293028 with the following comment: follow up patch, which removes shutdowns which were preventing any recovery to happen makes the first step a separate forkable method adds some comments Note, that the comments in oc_rec2 didn't exactly reflect the steps, it said: 'create table, delete rows, compress, add rows, commit' but there were no rows getting added after the compress. So I removed the 'add rows' from the comment in the new test.
        Hide
        Myrna van Lunteren added a comment -

        Attaching a new patch which tries to address creating the database within the first forked test, to more closely resemble the original test case.

        Because we're forking a new process, the only way I could think of doing this is by making a change to the default TestConfiguration to check for a system property specifying the database.

        Please review.

        Show
        Myrna van Lunteren added a comment - Attaching a new patch which tries to address creating the database within the first forked test, to more closely resemble the original test case. Because we're forking a new process, the only way I could think of doing this is by making a change to the default TestConfiguration to check for a system property specifying the database. Please review.
        Hide
        Kristian Waagan added a comment -

        I'm not sure it's any better, but an alternative is to use a change database decorator in the forked process. I'm doing this in the "modernized" compatibility test, but it requires a little more infrastructure and you still have to pass the database name as a system property.

        If you commit the patch, I'll see if it works well in my scenario too (I'm doing some more configuration so I'm not sure if it counts as a default TestConfiguration yet).
        To me your approach seems better for simple test cases like those being run here, since you don't have a suite method. It appears to me the -m option is incompatible with a lot of tests, but then I don't know how it's implemented (i.e., does it still run the suite method, where many tests get their setup from, and filter out the unwanted test cases afterwards?)

        Show
        Kristian Waagan added a comment - I'm not sure it's any better, but an alternative is to use a change database decorator in the forked process. I'm doing this in the "modernized" compatibility test, but it requires a little more infrastructure and you still have to pass the database name as a system property. If you commit the patch, I'll see if it works well in my scenario too (I'm doing some more configuration so I'm not sure if it counts as a default TestConfiguration yet). To me your approach seems better for simple test cases like those being run here, since you don't have a suite method. It appears to me the -m option is incompatible with a lot of tests, but then I don't know how it's implemented (i.e., does it still run the suite method, where many tests get their setup from, and filter out the unwanted test cases afterwards?)
        Hide
        Myrna van Lunteren added a comment -

        Thank you Kristian for reviewing the patch.

        I committed the change with revision 1294805
        This was the commit comment:
        DERBY-5382; Convert existing harness recovery tests to JUnit tests
        Adjusting test to create the database in the first launched method.
        Modifying TestConfiguration to this end to look for a property
        'derby.tests.defaultDatabaseName'
        which can get passed on in a new BaseTestCase method
        ((assertLaunchedJUnitTestMethod(String, String)).

        I wasn't familiar with the -m option either, that was part of Siddharth's original patch. It was new with junit 3.8.2.
        I found it hard to find information about this functionality, so I ran some experiments, and it seems to me the -m option does not run the suite method, only the constructor.

        Show
        Myrna van Lunteren added a comment - Thank you Kristian for reviewing the patch. I committed the change with revision 1294805 This was the commit comment: DERBY-5382 ; Convert existing harness recovery tests to JUnit tests Adjusting test to create the database in the first launched method. Modifying TestConfiguration to this end to look for a property 'derby.tests.defaultDatabaseName' which can get passed on in a new BaseTestCase method ((assertLaunchedJUnitTestMethod(String, String)). I wasn't familiar with the -m option either, that was part of Siddharth's original patch. It was new with junit 3.8.2. I found it hard to find information about this functionality, so I ran some experiments, and it seems to me the -m option does not run the suite method, only the constructor.
        Hide
        Myrna van Lunteren added a comment -

        I think this concludes the work on this issue.

        Show
        Myrna van Lunteren added a comment - I think this concludes the work on this issue.
        Hide
        Kristian Waagan added a comment -

        Just for the record, I don't think the -m option is compatible with JUnit 4. No big deal, it may be years before we start using JUnit 4.

        Show
        Kristian Waagan added a comment - Just for the record, I don't think the -m option is compatible with JUnit 4. No big deal, it may be years before we start using JUnit 4.
        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.

          People

          • Assignee:
            Siddharth Srivastava
            Reporter:
            Siddharth Srivastava
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development