Derby
  1. Derby
  2. DERBY-5014

Tests should restore the timeout values to default after they are done running.

    Details

    • Urgency:
      Normal
    • Issue & fix info:
      Known fix, Newcomer

      Description

      There are still couple more tests that change the lock time out during the test run but don't restore it to the default at the
      end of the test. Knut already fixed this behavior for SetTransactionIsolationTest.java and ResultSetMiscTest.java as part of DERBY-4273
      Following are some additional tests that should be fixed
      1)ErrorMessageTest.java 2)StressMultiTest.java 3)Sttest.java 4)SysinfoTest.java 5)DatabaseMetaDataTest.java 6)ResultSetsFromPreparedStatementTest.java

      1. d5014_1.diff
        2 kB
        Tiago R. Espinha
      2. d5014_1.diff
        3 kB
        Siddharth Srivastava

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open In Progress In Progress
        103d 8h 51m 1 Siddharth Srivastava 23/May/11 08:48
        In Progress In Progress Resolved Resolved
        14d 13h 30m 1 Tiago R. Espinha 06/Jun/11 22:19
        Resolved Resolved Reopened Reopened
        5d 1h 57m 1 Myrna van Lunteren 22/Aug/11 22:53
        Reopened Reopened Closed Closed
        32m 34s 1 Myrna van Lunteren 22/Aug/11 23:25
        Closed Closed Reopened Reopened
        458d 11h 27m 2 Kathey Marsden 13/Sep/12 00:08
        Reopened Reopened Resolved Resolved
        12m 2s 2 Kathey Marsden 13/Sep/12 00:20
        Resolved Resolved Closed Closed
        428d 20h 46m 2 Knut Anders Hatlen 15/Nov/13 08:15
        Knut Anders Hatlen made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Knut Anders Hatlen added a comment -

        [bulk update: close all resolved issues that haven't had any activity the last year]

        Show
        Knut Anders Hatlen added a comment - [bulk update: close all resolved issues that haven't had any activity the last year]
        Gavin made changes -
        Workflow jira [ 12544945 ] Default workflow, editable Closed status [ 12801928 ]
        Kathey Marsden made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Kathey Marsden made changes -
        Labels derby_triage10_8 derby_backport_reject_10_5
        Kathey Marsden made changes -
        Resolution Fixed [ 1 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Myrna van Lunteren made changes -
        Fix Version/s 10.8.2.2 [ 12317968 ]
        Fix Version/s 10.8.2.0 [ 12317955 ]
        Myrna van Lunteren made changes -
        Fix Version/s 10.8.2.0 [ 12317955 ]
        Fix Version/s 10.8.1.6 [ 12316676 ]
        Myrna van Lunteren made changes -
        Status Reopened [ 4 ] Closed [ 6 ]
        Fix Version/s 10.8.1.6 [ 12316676 ]
        Resolution Fixed [ 1 ]
        Hide
        Myrna van Lunteren added a comment -

        backported this change to 10.8 with revision 1160474.

        Show
        Myrna van Lunteren added a comment - backported this change to 10.8 with revision 1160474.
        Myrna van Lunteren made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Myrna van Lunteren added a comment -

        reopening for backport to 10.8

        Show
        Myrna van Lunteren added a comment - reopening for backport to 10.8
        Kathey Marsden made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Kathey Marsden made changes -
        Fix Version/s 10.9.0.0 [ 12316344 ]
        Hide
        Kathey Marsden added a comment -

        I think this would be a good candidate for backport

        Show
        Kathey Marsden added a comment - I think this would be a good candidate for backport
        Kathey Marsden made changes -
        Resolution Fixed [ 1 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Siddharth Srivastava made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Tiago R. Espinha made changes -
        Status In Progress [ 3 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Tiago R. Espinha added a comment -

        suites.All was a go, so I went ahead and committed this patch with revision 1132747.

        Show
        Tiago R. Espinha added a comment - suites.All was a go, so I went ahead and committed this patch with revision 1132747.
        Hide
        Knut Anders Hatlen added a comment -

        I don't think there's anything that needs to be fixed in those two tests. Perhaps it would have been better if ResultSetsFromPreparedStatementTest had cleared the timeout property (by setting it to NULL) instead of setting it to 60, since that would do the right thing even if we at some point change the default timeout, but at least it does reset the timeout to the current default.

        Show
        Knut Anders Hatlen added a comment - I don't think there's anything that needs to be fixed in those two tests. Perhaps it would have been better if ResultSetsFromPreparedStatementTest had cleared the timeout property (by setting it to NULL) instead of setting it to 60, since that would do the right thing even if we at some point change the default timeout, but at least it does reset the timeout to the current default.
        Hide
        Tiago R. Espinha added a comment -

        There's something I just noticed and I'd like to clear before committing the patch. There's two more tests that weren't converted nor mentioned here in the comments:

        • ResultSetsFromPreparedStatementTest.java
          This one seems to have its own mechanism to restore the lock timeout to the default.

        Mamta & Knut, did this test get flagged somewhere? On a very quick visual inspection, it seems like the timeouts are restored.

        • DatabaseMetaDataTest.java
          This test already uses the DatabasePropertyTestSetup decorator. Why was it included in this issue?
        Show
        Tiago R. Espinha added a comment - There's something I just noticed and I'd like to clear before committing the patch. There's two more tests that weren't converted nor mentioned here in the comments: ResultSetsFromPreparedStatementTest.java This one seems to have its own mechanism to restore the lock timeout to the default. Mamta & Knut, did this test get flagged somewhere? On a very quick visual inspection, it seems like the timeouts are restored. DatabaseMetaDataTest.java This test already uses the DatabasePropertyTestSetup decorator. Why was it included in this issue?
        Tiago R. Espinha made changes -
        Attachment d5014_1.diff [ 12481277 ]
        Hide
        Tiago R. Espinha added a comment -

        Removed unnecessary patch hunks.

        Show
        Tiago R. Espinha added a comment - Removed unnecessary patch hunks.
        Hide
        Tiago R. Espinha added a comment -

        Hi Siddharth,

        It seems like the patch has passed but I have a few questions/remarks about the patch.

        1) As mentioned before, please avoid whitespace changes. In this patch, we're patching DatabasePropertyTestSetup.java unnecessarily.

        2) Why do we need to import java.util.Properties into ResultSetsFromPreparedStatementTest.java?

        In the end, is SysinfoTest.java the only one that needs to be changed? If so and if no one else objects, I'm going to change the patch to remove the unnecessary chunks and commit it.

        I should also mention (and Siddharth, in the future it's also good practice to do so on JIRA upon posting a patch) that regressions were ran (suites.All) for this patch and it succeeded without any failures.

        As for what you mentioned about creating a test to check whether the default values have been reset, I think that can be very difficult. The main challenge is: when do you run this test? Would you run it after every single test in JUnit? That would be incredibly time consuming.

        Since these are just tests, we usually just trust that: if suites.All passes, then all the values are being reset, or if they aren't, it's not serious enough that it is not making the whole suite collapse.

        Show
        Tiago R. Espinha added a comment - Hi Siddharth, It seems like the patch has passed but I have a few questions/remarks about the patch. 1) As mentioned before, please avoid whitespace changes. In this patch, we're patching DatabasePropertyTestSetup.java unnecessarily. 2) Why do we need to import java.util.Properties into ResultSetsFromPreparedStatementTest.java? In the end, is SysinfoTest.java the only one that needs to be changed? If so and if no one else objects, I'm going to change the patch to remove the unnecessary chunks and commit it. I should also mention (and Siddharth, in the future it's also good practice to do so on JIRA upon posting a patch) that regressions were ran (suites.All) for this patch and it succeeded without any failures. – As for what you mentioned about creating a test to check whether the default values have been reset, I think that can be very difficult. The main challenge is: when do you run this test? Would you run it after every single test in JUnit? That would be incredibly time consuming. Since these are just tests, we usually just trust that: if suites.All passes, then all the values are being reset, or if they aren't, it's not serious enough that it is not making the whole suite collapse.
        Siddharth Srivastava made changes -
        Attachment d5014_1.diff [ 12480929 ]
        Hide
        Siddharth Srivastava added a comment -

        Attached is the fix for this issue.

        While fixing this I was thinking, where are the default values specified ?
        Is it feasible to have a test to check if the default values have been reset or not ?

        Show
        Siddharth Srivastava added a comment - Attached is the fix for this issue. While fixing this I was thinking, where are the default values specified ? Is it feasible to have a test to check if the default values have been reset or not ?
        Hide
        Knut Anders Hatlen added a comment -

        Hi Siddharth,

        ErrorMessageTest and StressMultiTest look fine to me too. I don't think those tests need to be changed. Sttest is a standalone test that doesn't run under the JUnit framework, so I don't think resetting the properties is necessary in that test either. I haven't checked the last three tests mentioned in the bug description.

        Show
        Knut Anders Hatlen added a comment - Hi Siddharth, ErrorMessageTest and StressMultiTest look fine to me too. I don't think those tests need to be changed. Sttest is a standalone test that doesn't run under the JUnit framework, so I don't think resetting the properties is necessary in that test either. I haven't checked the last three tests mentioned in the bug description.
        Siddharth Srivastava made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Hide
        Siddharth Srivastava added a comment -

        Just went through ErrorMessageTest.java and StressMultiTest.java. Don't they already restore the default timeout and property values or I am misunderstanding something ?
        They use DatabasePropertyTestSetup which resets the values on tearDown.

        Show
        Siddharth Srivastava added a comment - Just went through ErrorMessageTest.java and StressMultiTest.java. Don't they already restore the default timeout and property values or I am misunderstanding something ? They use DatabasePropertyTestSetup which resets the values on tearDown.
        Siddharth Srivastava made changes -
        Assignee Siddharth Srivastava [ siddharthsrivastava ]
        Hide
        Kathey Marsden added a comment -

        I think this would be great as part of a GSOC test and fix project or even before if you want to start working on it.
        showing in your application that you have already started working on some of the issues is always good.

        Show
        Kathey Marsden added a comment - I think this would be great as part of a GSOC test and fix project or even before if you want to start working on it. showing in your application that you have already started working on some of the issues is always good.
        Hide
        Siddharth Srivastava added a comment -

        Any views on including this as Derby Test and Fix project in GSoC 2011 ? (I am considering it to be in mine )

        Show
        Siddharth Srivastava added a comment - Any views on including this as Derby Test and Fix project in GSoC 2011 ? (I am considering it to be in mine )
        Myrna van Lunteren made changes -
        Field Original Value New Value
        Labels derby_triage10_8
        Urgency Normal
        Issue & fix info [Known fix, Newcomer]
        Mamta A. Satoor created issue -

          People

          • Assignee:
            Siddharth Srivastava
            Reporter:
            Mamta A. Satoor
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development