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

        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 )
        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 -

        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.
        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.
        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
        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.
        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 -

        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?
        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 -

        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
        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
        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
        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.
        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]

          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