Derby
  1. Derby
  2. DERBY-5726

Make it more difficult to forget calling super.tearDown() from BaseJDBCTestCase's subclasses

    Details

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

      Description

      Many of the classes that extend BaseJDBCTestCase and override the tearDown() method, forget to call super.tearDown(), and thereby prevent resources from being freed after completion. We should add a mechanism that enforces the correct behaviour.

      If we were starting from scratch, we might have made BaseJDBCTestCase.tearDown() final and added a new overridable method that was called from BaseJDBCTestCase.tearDown() before it freed the statements and connections. Then there would be no way to prevent BaseJDBCTestCase.tearDown() from running in the subclasses. That would however require us to change all existing overrides of BaseJDBCTestCase.tearDown() (current count: 131), which would be a chunk of work.

      I'd rather suggest that we add an override of runBare() in BaseJDBCTestCase that asserts that the connection has been cleared out when a test case has completed successfully. Something like this:

      public void runBare() throws Throwable

      { super.runBare(); // It's quite common to forget to call super.tearDown() when // overriding tearDown() in sub-classes. assertNull( "Connection should be null by now. " + "Missing call to super.tearDown()?", conn); }

      Then it would still be possible to forget to call super.tearDown(), but it would be discovered when trying to run the test.

      Adding the above method to BaseJDBCTestCase and running InternationalConnectTest gave this result:

      .....F.F....F
      Time: 5,748
      There were 3 failures:
      1) testDriverManagerConnect(org.apache.derbyTesting.functionTests.tests.jdbcapi.InternationalConnectTest)junit.framework.AssertionFailedError: Connection should be null by now. Missing call to super.tearDown()?
      at org.apache.derbyTesting.junit.BaseJDBCTestCase.runBare(BaseJDBCTestCase.java:431)
      at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
      at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
      at junit.extensions.TestSetup.run(TestSetup.java:25)
      at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
      2) testBoundaries(org.apache.derbyTesting.functionTests.tests.jdbcapi.InternationalConnectTest)junit.framework.AssertionFailedError: Connection should be null by now. Missing call to super.tearDown()?
      at org.apache.derbyTesting.junit.BaseJDBCTestCase.runBare(BaseJDBCTestCase.java:431)
      (...)

      1. d5726-1a.diff
        3 kB
        Knut Anders Hatlen

        Issue Links

          Activity

          Hide
          Knut Anders Hatlen added a comment -

          If someone wants to run the tests with limited amount of memory, it might make more sense to have a way to disable known memory-hungry tests, as forking the tests would still require much memory in the sub-process. I'm not sure we'd want to introduce a hard limit on 64MB for running suites.All, though, as that was a number I picked rather arbitrarily to smoke out the leaks.

          Show
          Knut Anders Hatlen added a comment - If someone wants to run the tests with limited amount of memory, it might make more sense to have a way to disable known memory-hungry tests, as forking the tests would still require much memory in the sub-process. I'm not sure we'd want to introduce a hard limit on 64MB for running suites.All, though, as that was a number I picked rather arbitrarily to smoke out the leaks.
          Hide
          Dag H. Wanvik added a comment -

          Would it make sense to run these memory hungry tests in forked VMs, so we can have them as part of the regression tests, but keep the main VM to 64MB requirement you have squeezed it down to?

          Show
          Dag H. Wanvik added a comment - Would it make sense to run these memory hungry tests in forked VMs, so we can have them as part of the regression tests, but keep the main VM to 64MB requirement you have squeezed it down to?
          Hide
          Knut Anders Hatlen added a comment -

          Committed revision 1332967.

          Show
          Knut Anders Hatlen added a comment - Committed revision 1332967.
          Hide
          Knut Anders Hatlen added a comment -

          Hi Kathey,

          I think this cleanup is important because memory leaks in the test framework may prevent us from discovering real memory leaks in the product, and also because if we keep letting new leaks into the tests, the amount of memory required for running the tests will just keep growing.

          For the record, with all the latest patches, suites.All now passes with 64MB heap in my environment if I disable the following test cases:

          TriggerTest

          • testBlobInTriggerTable

          Derby2017LayerATest

          • cs_FailedStreamInsertCharBufferBoundaries
          • cs_StreamInsertCharBufferBoundary

          StatementJdbc30Test

          • xtestMaxOpenStatementsWithQueryTimeout

          ClobReclamationTest

          • testBlobLinkedListReclamationOnRollback

          When running the tests standalone, TriggerTest requires 165MB heap to pass, and the other three tests pass with 85MB.

          Show
          Knut Anders Hatlen added a comment - Hi Kathey, I think this cleanup is important because memory leaks in the test framework may prevent us from discovering real memory leaks in the product, and also because if we keep letting new leaks into the tests, the amount of memory required for running the tests will just keep growing. For the record, with all the latest patches, suites.All now passes with 64MB heap in my environment if I disable the following test cases: TriggerTest testBlobInTriggerTable Derby2017LayerATest cs_FailedStreamInsertCharBufferBoundaries cs_StreamInsertCharBufferBoundary StatementJdbc30Test xtestMaxOpenStatementsWithQueryTimeout ClobReclamationTest testBlobLinkedListReclamationOnRollback When running the tests standalone, TriggerTest requires 165MB heap to pass, and the other three tests pass with 85MB.
          Hide
          Kathey Marsden added a comment -

          This sounds like a good idea to ensure teardown gets called and things get cleaned up. I remember sometime back doing a test cleanup cleanup effort, but can't exactly remember my motivation. It might have been making sure things run in the default heap or maybe it had something to do with permgen space. Why is the cleanup important?

          Show
          Kathey Marsden added a comment - This sounds like a good idea to ensure teardown gets called and things get cleaned up. I remember sometime back doing a test cleanup cleanup effort, but can't exactly remember my motivation. It might have been making sure things run in the default heap or maybe it had something to do with permgen space. Why is the cleanup important?
          Hide
          Knut Anders Hatlen added a comment -

          Attaching a patch that adds the assert to BaseJDBCTestCase.runBare(), makes runBare() final to prevent sub-classes from being able to bypass the check, and add an overridable method (runBareOverridable) that is called from the final runBare() method.

          I successfully ran suites.All and perf.basic._Suite (which is the suite that contains the only tests that override BaseJDBCTestCase.runBare()).

          Show
          Knut Anders Hatlen added a comment - Attaching a patch that adds the assert to BaseJDBCTestCase.runBare(), makes runBare() final to prevent sub-classes from being able to bypass the check, and add an overridable method (runBareOverridable) that is called from the final runBare() method. I successfully ran suites.All and perf.basic._Suite (which is the suite that contains the only tests that override BaseJDBCTestCase.runBare()).
          Hide
          Knut Anders Hatlen added a comment -

          There were no failures in suites.All when running with the above mentioned runBare() method in BaseJDBCTestCase, as well as the uncommitted patches attached to DERBY-5721, DERBY-5722, DERBY-5723 and DERBY-5725. So at least it looks like we've smoked out most of these problems now.

          I'll see if I can modify the patch as Rick suggested. There is actually just one override of BaseJDBCTestCase.runBare(), and that's in JDBCPerfTestCase. The other runBare() overrides are in direct subclasses of BaseTestCase, which doesn't have any connections or statements to close.

          Show
          Knut Anders Hatlen added a comment - There were no failures in suites.All when running with the above mentioned runBare() method in BaseJDBCTestCase, as well as the uncommitted patches attached to DERBY-5721 , DERBY-5722 , DERBY-5723 and DERBY-5725 . So at least it looks like we've smoked out most of these problems now. I'll see if I can modify the patch as Rick suggested. There is actually just one override of BaseJDBCTestCase.runBare(), and that's in JDBCPerfTestCase. The other runBare() overrides are in direct subclasses of BaseTestCase, which doesn't have any connections or statements to close.
          Hide
          Rick Hillegas added a comment -

          Hi Knut,

          That sounds like a good idea. Maybe you can apply your first suggestion to the runBare() solution, making runBare() final and having it call an overridable method. Shouldn't be too messy. I only see overrides of runBare() in a handful of classes.

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - Hi Knut, That sounds like a good idea. Maybe you can apply your first suggestion to the runBare() solution, making runBare() final and having it call an overridable method. Shouldn't be too messy. I only see overrides of runBare() in a handful of classes. Thanks, -Rick

            People

            • Assignee:
              Knut Anders Hatlen
              Reporter:
              Knut Anders Hatlen
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development