Uploaded image for project: 'Derby'
  1. Derby
  2. DERBY-2419

Tighten encapsulation of state in TestConfiguration

Details

    • Improvement
    • Status: Closed
    • Minor
    • Resolution: Fixed
    • 10.3.1.4
    • 10.7.1.1
    • Test
    • None

    Description

      Parts of the state of TestConfiguration has been made public, which they should not be; DEFAULT_PORT and DEFAULT_HOSTNAME.
      Using these directly from the outside can cause settings overridden by the user to be ignored by tests. Further, a test should not care if the host/port it uses is the Derby default or the values set by the user running the test.

      To obtain a hostname and a port number, use the methods getPort and getHostName in TestConfiguration.

      Attachments

        1. derby-2419-1a-alternative1.diff
          0.7 kB
          Kristian Waagan
        2. derby-2419-2a-alternative2.diff
          4 kB
          Kristian Waagan
        3. derby-2419-2a-alternative2.stat
          0.4 kB
          Kristian Waagan
        4. derby-2419-3a-further_encapsulation.diff
          16 kB
          Kristian Waagan
        5. derby-2419-refactor-01.diff
          2 kB
          Richard N. Hillegas

        Issue Links

          Activity

            I think I understand why the default hostname and port number were made public now

            The problem is that when you change to a client setup from an embedded setup, you basically have no values to supply to the TestConfiguration constructor. You can't use the values from the embedded setup, because they are 'null' and -1. You definitely don't want to supply your own custom values, as this will (possibly) mess up concurrent runs (for instance two suites.All on the same machine).

            I had a brief discussion offline with Dag, and we identified the following scenarios wrt port numbers:
            1) The default Derby port number (1527)
            2) The default port number for the test run/configuration (as 1, or as specified by user by a property)
            3) A different port for running multiple servers in the same test (typically as 2 with an offset)

            Making the default Derby hostname and port public is not the right solution, as this will make independent runs always use the same hostname/port and could cause collision. Thus supporting only 1 is not a good solution.

            By supporting option 2, and giving the user the possibility to override the port number used by the tests, we allow independent test runs to execute in parallel on a single machine. There are various implementation details here as well, such as if the property should only be read when the TestConfiguration class is loaded (static initializer block) or on instantiation. I prefer the first, as the second can lead to strange failures if the port number is changed "on-the-fly".

            Option 3 is just a little extra, but as I don't think there is any use for it yet (??), it can be left for later. It certainly require more thought to avoid port usage to get out of control...

            I have a few ideas for fixing this, but I need some more time with the initial patch.
            If people have comments on this issue, please put them forth so they can be discussed and/or incorporated in the patch.

            Note that there is a big difference between concurrent independent runs and parallelizing the execution of the tests. In the latter you run tests in parallel by invoking a single command and let the "harness" manage parallel execution. This demands more elaborate port handling, and I have not registered any community initiatives to implement parallel test execution.

            kristwaa Kristian Waagan added a comment - I think I understand why the default hostname and port number were made public now The problem is that when you change to a client setup from an embedded setup, you basically have no values to supply to the TestConfiguration constructor. You can't use the values from the embedded setup, because they are 'null' and -1. You definitely don't want to supply your own custom values, as this will (possibly) mess up concurrent runs (for instance two suites.All on the same machine). I had a brief discussion offline with Dag, and we identified the following scenarios wrt port numbers: 1) The default Derby port number (1527) 2) The default port number for the test run/configuration (as 1, or as specified by user by a property) 3) A different port for running multiple servers in the same test (typically as 2 with an offset) Making the default Derby hostname and port public is not the right solution, as this will make independent runs always use the same hostname/port and could cause collision. Thus supporting only 1 is not a good solution. By supporting option 2, and giving the user the possibility to override the port number used by the tests, we allow independent test runs to execute in parallel on a single machine. There are various implementation details here as well, such as if the property should only be read when the TestConfiguration class is loaded (static initializer block) or on instantiation. I prefer the first, as the second can lead to strange failures if the port number is changed "on-the-fly". Option 3 is just a little extra, but as I don't think there is any use for it yet (??), it can be left for later. It certainly require more thought to avoid port usage to get out of control... I have a few ideas for fixing this, but I need some more time with the initial patch. If people have comments on this issue, please put them forth so they can be discussed and/or incorporated in the patch. Note that there is a big difference between concurrent independent runs and parallelizing the execution of the tests. In the latter you run tests in parallel by invoking a single command and let the "harness" manage parallel execution. This demands more elaborate port handling, and I have not registered any community initiatives to implement parallel test execution.

            I have attached two draft alternatives for this issue. A brief description of them both:
            (1) Make non-network configurations (embedded) return valid values for host and port.
            (2) Create client/server configurations through a static methods, which has some logic to check the host name and the port of the current configuration. If they are not valid (hostname null or "", port < 1), which will be the case if the current configuration is embedded, use defaults.

            I have also thought a bit about limiting the possibility for test writers to choose which host/port they can use, i.e. in (2) you don't ever specify host/port though a public API.
            My intention was that either the default Derby port (1527) and 'localhost' are used, or the user specify values somehow when she starts the test runner. Is this too little flexibility?

            I have not addressed the functionality for running multiple servers in a single test (different ports).

            Feedback is appreciated, as this issue kind of blocks a few other issues.

            I ran suites.All without failures for patch 2a, but did not run derbyall.
            These patches are not ready for commit.

            kristwaa Kristian Waagan added a comment - I have attached two draft alternatives for this issue. A brief description of them both: (1) Make non-network configurations (embedded) return valid values for host and port. (2) Create client/server configurations through a static methods, which has some logic to check the host name and the port of the current configuration. If they are not valid (hostname null or "", port < 1), which will be the case if the current configuration is embedded, use defaults. I have also thought a bit about limiting the possibility for test writers to choose which host/port they can use, i.e. in (2) you don't ever specify host/port though a public API. My intention was that either the default Derby port (1527) and 'localhost' are used, or the user specify values somehow when she starts the test runner. Is this too little flexibility? I have not addressed the functionality for running multiple servers in a single test (different ports). Feedback is appreciated, as this issue kind of blocks a few other issues. I ran suites.All without failures for patch 2a, but did not run derbyall. These patches are not ready for commit.

            Attaching derby-2419-refactor-01.diff (I am running regression tests now). This patch removes the need for SecureServerTest to mention the default hostname and port number constants. It does this by refactoring TestConfiguration.clientServerDecorator(). The guts of that routine now live in a new method TestConfiguration.defaultServerDecorator(), which SecureServerTest calls.

            The new method preserves something fishy from the previous code: TestConfiguration exposes a public static method which allows callers to use the hardcoded hostname and port number constants regardless of whether they are declared public or private. This fishy code appears to be invoked in a lot of places already.(I counted 44 references to it).

            Along the way, I tried another solution to my problem. That solution was to create a ServerSetup using the hostname and port gleaned from TestConfiguration.getCurrent(). That gave me a port number of -1, which wasn't very helpful. So I abandoned that approach.

            This patch cleans up SecureServerTest and creates one more invocation of the fishy code. Someone who understands the secret machinations of TestConfiguration will probably want to fix the fishy code itself. This patch touches the following files:

            M java/testing/org/apache/derbyTesting/functionTests/tests/derbynet/SecureServerTest.java
            M java/testing/org/apache/derbyTesting/junit/TestConfiguration.java

            rhillegas Richard N. Hillegas added a comment - Attaching derby-2419-refactor-01.diff (I am running regression tests now). This patch removes the need for SecureServerTest to mention the default hostname and port number constants. It does this by refactoring TestConfiguration.clientServerDecorator(). The guts of that routine now live in a new method TestConfiguration.defaultServerDecorator(), which SecureServerTest calls. The new method preserves something fishy from the previous code: TestConfiguration exposes a public static method which allows callers to use the hardcoded hostname and port number constants regardless of whether they are declared public or private. This fishy code appears to be invoked in a lot of places already.(I counted 44 references to it). Along the way, I tried another solution to my problem. That solution was to create a ServerSetup using the hostname and port gleaned from TestConfiguration.getCurrent(). That gave me a port number of -1, which wasn't very helpful. So I abandoned that approach. This patch cleans up SecureServerTest and creates one more invocation of the fishy code. Someone who understands the secret machinations of TestConfiguration will probably want to fix the fishy code itself. This patch touches the following files: M java/testing/org/apache/derbyTesting/functionTests/tests/derbynet/SecureServerTest.java M java/testing/org/apache/derbyTesting/junit/TestConfiguration.java

            Committed derby-2419-refactor-01.diff at subversion revision 519945.

            rhillegas Richard N. Hillegas added a comment - Committed derby-2419-refactor-01.diff at subversion revision 519945.

            Attaching patch 'derby-2419-3a-further_encapsulation.diff', which makes further changes to TestConfiguration and the affected tests.
            I introduced the interface DerbyConstants to hold some constants used by the JUnit test framework and the tests themselves.
            Changes:

            • made port and hostname private
            • removed unused constant DEFAULT_SSL
            • moved TEST_DBO from TestConfiguration to DerbyConstants

            Regression tests running.
            Patch ready for review.

            kristwaa Kristian Waagan added a comment - Attaching patch 'derby-2419-3a-further_encapsulation.diff', which makes further changes to TestConfiguration and the affected tests. I introduced the interface DerbyConstants to hold some constants used by the JUnit test framework and the tests themselves. Changes: made port and hostname private removed unused constant DEFAULT_SSL moved TEST_DBO from TestConfiguration to DerbyConstants Regression tests running. Patch ready for review.

            Tests passed (12715), got one failure which is network related:

            testPing(org.apache.derbyTesting.functionTests.tests.derbynet.NetworkServerControlClientCommandTest) FAILURE:
            junit.framework.AssertionFailedError
            at junit.framework.Assert.fail(Assert.java:47)
            at junit.framework.Assert.assertTrue(Assert.java:20)
            at junit.framework.Assert.assertFalse(Assert.java:34)
            at junit.framework.Assert.assertFalse(Assert.java:41)
            at org.apache.derbyTesting.junit.BaseTestCase.assertExecJavaCmdAsExpected(BaseTestCase.java:484)
            at org.apache.derbyTesting.functionTests.tests.derbynet.NetworkServerControlClientCommandTest.assertFailedPing(NetworkServerControlClientCommandTest.java:115)
            at org.apache.derbyTesting.functionTests.tests.derbynet.NetworkServerControlClientCommandTest.testPing(NetworkServerControlClientCommandTest.java:80)
            at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
            at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
            at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
            at java.lang.reflect.Method.invoke(Method.java:597)
            at junit.framework.TestCase.runTest(TestCase.java:164)
            at junit.framework.TestCase.runBare(TestCase.java:130)
            at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:109)
            at junit.framework.TestResult$1.protect(TestResult.java:106)
            at junit.framework.TestResult.runProtected(TestResult.java:124)
            at junit.framework.TestResult.run(TestResult.java:109)
            at junit.framework.TestCase.run(TestCase.java:120)
            at junit.framework.TestSuite.runTest(TestSuite.java:230)
            at junit.framework.TestSuite.run(TestSuite.java:225)
            at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
            at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
            at junit.framework.TestResult.runProtected(TestResult.java:124)
            at junit.extensions.TestSetup.run(TestSetup.java:25)
            at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
            at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
            at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
            at junit.framework.TestResult.runProtected(TestResult.java:124)
            at junit.extensions.TestSetup.run(TestSetup.java:25)
            at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
            at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
            at junit.framework.TestResult.runProtected(TestResult.java:124)
            at junit.extensions.TestSetup.run(TestSetup.java:25)
            at junit.framework.TestSuite.runTest(TestSuite.java:230)
            at junit.framework.TestSuite.run(TestSuite.java:225)
            at junit.framework.TestSuite.runTest(TestSuite.java:230)
            at junit.framework.TestSuite.run(TestSuite.java:225)
            at kah.TestRunner.main(TestRunner.java:77)

            I have seen this earlier when running tests in parallel, so I don't think it is caused by the patch. It's a mystery to me what is started on the port in question that makes the ping succeed.
            In any case, I do plan to post a follow-up patch with a method to obtain a "bogus port".

            kristwaa Kristian Waagan added a comment - Tests passed (12715), got one failure which is network related: testPing(org.apache.derbyTesting.functionTests.tests.derbynet.NetworkServerControlClientCommandTest) FAILURE: junit.framework.AssertionFailedError at junit.framework.Assert.fail(Assert.java:47) at junit.framework.Assert.assertTrue(Assert.java:20) at junit.framework.Assert.assertFalse(Assert.java:34) at junit.framework.Assert.assertFalse(Assert.java:41) at org.apache.derbyTesting.junit.BaseTestCase.assertExecJavaCmdAsExpected(BaseTestCase.java:484) at org.apache.derbyTesting.functionTests.tests.derbynet.NetworkServerControlClientCommandTest.assertFailedPing(NetworkServerControlClientCommandTest.java:115) at org.apache.derbyTesting.functionTests.tests.derbynet.NetworkServerControlClientCommandTest.testPing(NetworkServerControlClientCommandTest.java:80) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at junit.framework.TestCase.runTest(TestCase.java:164) at junit.framework.TestCase.runBare(TestCase.java:130) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:109) at junit.framework.TestResult$1.protect(TestResult.java:106) at junit.framework.TestResult.runProtected(TestResult.java:124) at junit.framework.TestResult.run(TestResult.java:109) at junit.framework.TestCase.run(TestCase.java:120) at junit.framework.TestSuite.runTest(TestSuite.java:230) at junit.framework.TestSuite.run(TestSuite.java:225) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.framework.TestResult.runProtected(TestResult.java:124) at junit.extensions.TestSetup.run(TestSetup.java:25) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.framework.TestResult.runProtected(TestResult.java:124) at junit.extensions.TestSetup.run(TestSetup.java:25) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.framework.TestResult.runProtected(TestResult.java:124) at junit.extensions.TestSetup.run(TestSetup.java:25) at junit.framework.TestSuite.runTest(TestSuite.java:230) at junit.framework.TestSuite.run(TestSuite.java:225) at junit.framework.TestSuite.runTest(TestSuite.java:230) at junit.framework.TestSuite.run(TestSuite.java:225) at kah.TestRunner.main(TestRunner.java:77) I have seen this earlier when running tests in parallel, so I don't think it is caused by the patch. It's a mystery to me what is started on the port in question that makes the ping succeed. In any case, I do plan to post a follow-up patch with a method to obtain a "bogus port".

            See DERBY-4700 for the bogus port issue.

            Committed patch 3a to trunk with revision 955529.

            kristwaa Kristian Waagan added a comment - See DERBY-4700 for the bogus port issue. Committed patch 3a to trunk with revision 955529.
            kristwaa Kristian Waagan added a comment - - edited

            I'm marking this as fixed.
            The issue has been hanging around for a long time. Tiago improved the situation a lot with DERBY-4217, and Rick's concern about the use of the default port has been addressed (maybe by the same Jira?).
            The issue Rick pointed out is still valid for DEFAULT_HOSTNAME, but that is less critical than using DEFAULT_PORT. It can be dealt with under a separate Jira issue.

            Feel free to reopen if you think marking this issue as fixed is wrong.

            kristwaa Kristian Waagan added a comment - - edited I'm marking this as fixed. The issue has been hanging around for a long time. Tiago improved the situation a lot with DERBY-4217 , and Rick's concern about the use of the default port has been addressed (maybe by the same Jira?). The issue Rick pointed out is still valid for DEFAULT_HOSTNAME, but that is less critical than using DEFAULT_PORT. It can be dealt with under a separate Jira issue. Feel free to reopen if you think marking this issue as fixed is wrong.

            Closing issue.

            kristwaa Kristian Waagan added a comment - Closing issue.

            People

              kristwaa Kristian Waagan
              kristwaa Kristian Waagan
              Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: