Derby
  1. Derby
  2. DERBY-3088

convert to junit, or otherwise eliminate version in tests which require an update of the master in the release management process

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.4.1.3
    • Component/s: Build tools, Test
    • Labels:
      None

      Description

      There are a number of tests that require a master update every time the version number is bumped.
      This is a tedious and error prone detail in the release management process.

      Currently affected masters are listed in tools/release/build.xml:
      java/testing/org/apache/derbyTesting/functionTests/master/DerbyNetClient/metadata.out
      java/testing/org/apache/derbyTesting/functionTests/master/DerbyNetClient/odbc_metadata.out
      java/testing/org/apache/derbyTesting/functionTests/master/metadata.out
      java/testing/org/apache/derbyTesting/functionTests/master/odbc_metadata.out
      java/testing/org/apache/derbyTesting/functionTests/master/NSinSameJVM.out
      java/testing/org/apache/derbyTesting/functionTests/master/checkToursDB.out
      java/testing/org/apache/derbyTesting/functionTests/master/testclientij.out
      java/testing/org/apache/derbyTesting/functionTests/master/testProperties.out

      It would streamline the release process if these tests in particular were either converted to junit, or if the version numbers would be eliminated.

      Note: there already is a bug for metadata.java conversion: DERBY-2242

      1. DERBY-3088_NetIjTest.diff
        6 kB
        Myrna van Lunteren
      2. DERBY-3088_ServerPropertiesTest_3.diff
        28 kB
        Myrna van Lunteren
      3. DERBY-3088_ServerPropertiesTest_3.stat
        0.4 kB
        Myrna van Lunteren
      4. DERBY-3088_ServerPropertiesTest2.diff
        47 kB
        Myrna van Lunteren
      5. DERBY-3088_ServerPropertiesTest.diff
        25 kB
        Myrna van Lunteren

        Issue Links

          Activity

          Hide
          Myrna van Lunteren added a comment -

          removed metadata* from the list by commenting out the driver version test (it's already in DatabaseMetadata) with revision 587953.

          Show
          Myrna van Lunteren added a comment - removed metadata* from the list by commenting out the driver version test (it's already in DatabaseMetadata) with revision 587953.
          Hide
          Myrna van Lunteren added a comment -

          Attaching an attempt of converting testProperties.java to ServerPropertiesTest.java.
          However, this is only an initial patch, the test isn't workable yet, but I'm sort of stuck.
          I hope someone can review and come up with suggestions.

          The current test has the following awful problems:

          • test fixtures testDefaultProperties, testTraceOn, testTraceOff and testLogConnectionsOn attempt to use a setup that starts a networkserver process, but the setup hangs, or at least, as best as I can tell, there's a timeout waiting for the server to start.
            I tried to imitate what was done in SecureServerTest and SSLTest, but that may be part of the problem (see also DERBY-3248, DERBY-3250, DERBY-3251)
          • test fixture testWrongCommands uses NetworkServerControlMain, which is especially awful, because it somewhere does a System.exit. Also, there is no checking of the result yet. I suspect this will have to do something like the ProcessStream classes of the old test harness.
          • test fixture testDefaultProperties now works for me on windows, but needs to be verified on other platforms.

          One more note re the testDefaultProperties fixture: I did not use a setup for this, because I wanted to test the starting and shutting down of servers with a different port, and the only way I thought of to ensure the priority of the port-setting mechanisms, without hardcoding the port(s), was to not use a setup or decorator, but to start multiple servers in sequence.
          Possibly there is a smarter way to achieve this too...

          Show
          Myrna van Lunteren added a comment - Attaching an attempt of converting testProperties.java to ServerPropertiesTest.java. However, this is only an initial patch, the test isn't workable yet, but I'm sort of stuck. I hope someone can review and come up with suggestions. The current test has the following awful problems: test fixtures testDefaultProperties, testTraceOn, testTraceOff and testLogConnectionsOn attempt to use a setup that starts a networkserver process, but the setup hangs, or at least, as best as I can tell, there's a timeout waiting for the server to start. I tried to imitate what was done in SecureServerTest and SSLTest, but that may be part of the problem (see also DERBY-3248 , DERBY-3250 , DERBY-3251 ) test fixture testWrongCommands uses NetworkServerControlMain, which is especially awful, because it somewhere does a System.exit. Also, there is no checking of the result yet. I suspect this will have to do something like the ProcessStream classes of the old test harness. test fixture testDefaultProperties now works for me on windows, but needs to be verified on other platforms. One more note re the testDefaultProperties fixture: I did not use a setup for this, because I wanted to test the starting and shutting down of servers with a different port, and the only way I thought of to ensure the priority of the port-setting mechanisms, without hardcoding the port(s), was to not use a setup or decorator, but to start multiple servers in sequence. Possibly there is a smarter way to achieve this too...
          Hide
          Daniel John Debrunner added a comment -

          I think this code in the decorateTest() is problematic:

          + NetworkServerTestSetup networkServerTestSetup =
          + new NetworkServerTestSetup(spt,
          + startupProps, startupArgs, true, true,
          + spt._inputStreamHolder);
          + spt.nsTestSetup = networkServerTestSetup;
          + Test testSetup =
          + SecurityManagerSetup.noSecurityManager(networkServerTestSetup);
          + Test test = TestConfiguration.defaultServerDecorator(testSetup);
          + test = TestConfiguration.clientServerDecorator(test);

          Two NetworkServerTestSetup decorators are being used, one explicitly and one by clientServerDecorator. Both of these will try to start the server, I believe on the same port, which wil probably lead to hangs.

          Show
          Daniel John Debrunner added a comment - I think this code in the decorateTest() is problematic: + NetworkServerTestSetup networkServerTestSetup = + new NetworkServerTestSetup(spt, + startupProps, startupArgs, true, true, + spt._inputStreamHolder); + spt.nsTestSetup = networkServerTestSetup; + Test testSetup = + SecurityManagerSetup.noSecurityManager(networkServerTestSetup); + Test test = TestConfiguration.defaultServerDecorator(testSetup); + test = TestConfiguration.clientServerDecorator(test); Two NetworkServerTestSetup decorators are being used, one explicitly and one by clientServerDecorator. Both of these will try to start the server, I believe on the same port, which wil probably lead to hangs.
          Hide
          Daniel John Debrunner added a comment -

          One comment I have to to re-use the code in NetworkServerTestSetup as much as possible and do not duplicate it in this (or any) specific test.
          That is code that starts the server, checks for it coming up, execs the server as a different process etc.

          Show
          Daniel John Debrunner added a comment - One comment I have to to re-use the code in NetworkServerTestSetup as much as possible and do not duplicate it in this (or any) specific test. That is code that starts the server, checks for it coming up, execs the server as a different process etc.
          Hide
          Myrna van Lunteren added a comment -

          Attaching a new patch, which has all 5 tests working.

          If there are no objections, I'd like to commit this.

          I still need to do further recrafting of the first fixture, so it (re)uses junit classes as Dan suggested. I'm open to further suggestions on how to approach this (in this fixture, consecutive servers are started in a variety of mechanisms - that's what's being tested - and I find it hard to envisage how one setup would handle this.)
          The current test also still uses noSecurityManager in one case, (with -p) I am thinking of modifying that to use the test's policy file.
          Finally I want to see if I can figure a way to make this test run with classes - currently you'll get a message complaining about needing derbynet.jar.
          But I'd like to address these issues in a follow up patch.

          Show
          Myrna van Lunteren added a comment - Attaching a new patch, which has all 5 tests working. If there are no objections, I'd like to commit this. I still need to do further recrafting of the first fixture, so it (re)uses junit classes as Dan suggested. I'm open to further suggestions on how to approach this (in this fixture, consecutive servers are started in a variety of mechanisms - that's what's being tested - and I find it hard to envisage how one setup would handle this.) The current test also still uses noSecurityManager in one case, (with -p) I am thinking of modifying that to use the test's policy file. Finally I want to see if I can figure a way to make this test run with classes - currently you'll get a message complaining about needing derbynet.jar. But I'd like to address these issues in a follow up patch.
          Hide
          Myrna van Lunteren added a comment -

          Revision 612684 improves the ServerPropertiesTest a little in that it now runs with either classes or jars - there is one fixture that needs jars, so there's an if in place.
          Also uses a new BaseTestCase method assertExecJavaCmdAsExpected instead of a similar one just in this test, and modified the test NetworkServerControlClientCommandTest to use it instead of the Utilities method.
          The execJavaCmd method in Utilites would hang on the pr.waitFor() method if there was more than a little data in the buffer, so it didn't work for all cases I needed to know a java command executed. I left it in place, for now.

          Show
          Myrna van Lunteren added a comment - Revision 612684 improves the ServerPropertiesTest a little in that it now runs with either classes or jars - there is one fixture that needs jars, so there's an if in place. Also uses a new BaseTestCase method assertExecJavaCmdAsExpected instead of a similar one just in this test, and modified the test NetworkServerControlClientCommandTest to use it instead of the Utilities method. The execJavaCmd method in Utilites would hang on the pr.waitFor() method if there was more than a little data in the buffer, so it didn't work for all cases I needed to know a java command executed. I left it in place, for now.
          Hide
          Myrna van Lunteren added a comment -

          Attaching a patch that reworks the ServerPropertiesTest so the first fixture uses NetworkServerTestSetup.
          Reworked NetworkServerTestSetup; added some helper methods and reworked pingForServerStart (to pingForServerUp) so that the method that pings has intelligence about the expected outcome - if the server is expected up, it will ping for a while until the ping is either successful, or timed out; if the server is expected down, it will ping for a while until unsuccessful or timed out. Also made it ping first, before sleeping.
          Reworked Utilities.ExecJavaCmd to only return the Process; the caller then needs to do the appropriate. Reason for this was that previously, if there was a lot of data from pr.getInputStream, the method would hang itsefl on the pr.waitFor().

          This passed suites.All for me.

          Show
          Myrna van Lunteren added a comment - Attaching a patch that reworks the ServerPropertiesTest so the first fixture uses NetworkServerTestSetup. Reworked NetworkServerTestSetup; added some helper methods and reworked pingForServerStart (to pingForServerUp) so that the method that pings has intelligence about the expected outcome - if the server is expected up, it will ping for a while until the ping is either successful, or timed out; if the server is expected down, it will ping for a while until unsuccessful or timed out. Also made it ping first, before sleeping. Reworked Utilities.ExecJavaCmd to only return the Process; the caller then needs to do the appropriate. Reason for this was that previously, if there was a lot of data from pr.getInputStream, the method would hang itsefl on the pr.waitFor(). This passed suites.All for me.
          Hide
          Myrna van Lunteren added a comment -

          I committed patch DERBY-3088_ServerPropertiesTest_3.diff with revision 616982.
          I saw no problems during my testrun, and the tinderbox also appears clean, and the results for windows at IBM (http://people.apache.org/~fuzzylogic/derby_test_results/main/windows/testSummary-current.html - for 616990) also show no new failures.

          Show
          Myrna van Lunteren added a comment - I committed patch DERBY-3088 _ServerPropertiesTest_3.diff with revision 616982. I saw no problems during my testrun, and the tinderbox also appears clean, and the results for windows at IBM ( http://people.apache.org/~fuzzylogic/derby_test_results/main/windows/testSummary-current.html - for 616990) also show no new failures.
          Hide
          Myrna van Lunteren added a comment - - edited

          I've been looking at the last remaining test master/canon - testclientij.out - that shows the version number.
          It looks like this test was explicitly intended as a .sql test to test DerbyNetClient connection strings, so converting to a .java junit test does not seem appropriate.

          The difference between other tests executing .sql files (e.g. NistScripts, LangScripts, ToolScripts) is that this one emulates the old harness' RunTest mechanism of using ij.main("-fr", "filename") (by extending util.IjTestCase), whereas the other tests use ij.runScript(..).

          -fr is possibly intended to be a undocumented feature, at least, I could find no documentation, even the ij javadoc is very quiet about it, and if you use it incorrectly, (e.g. : java org.apache.derby.tools.ij testclientij.sql) you get an NPE rather than an appropriate message.
          I gather it's supposed to list the file in it's full resource path - e.g.: java org.apache.derby.tools.ij /testclientij.sql (if . is in your classpath and the file sits in the current dir).

          Apparently, and per the code, running main vs runScript results in some slight differences:

          • no version number printed
          • first connection is already started
          • no messages indicating number of rows selected
            These differences do not seem relevant to this particular test.

          I think the util.IjTestCase mechanism may possibly be of use in testing ij in the future, but at present, in NetIjTest, it appears to be just another way of doing the same thing as e.g. NistScripts, a way that has the unfortunate effect of printing the version number.

          We could consider adding the script to the tools directory/_Suite...But I think it is more appropriate in its current spot. Thus, I intend to rewrite NetIjTest to use the same mechanism as NistScripts. When this is done, I intend to get rid of the master-editing target in tools/release/build.xml.
          I'll leave IjTestCase in place for future use. If used, it would need a different compareCanon method in CanonTestCase, one that ignores lines with specified strings (or maybe specifically checks on the version number without it being in the canon).

          Show
          Myrna van Lunteren added a comment - - edited I've been looking at the last remaining test master/canon - testclientij.out - that shows the version number. It looks like this test was explicitly intended as a .sql test to test DerbyNetClient connection strings, so converting to a .java junit test does not seem appropriate. The difference between other tests executing .sql files (e.g. NistScripts, LangScripts, ToolScripts) is that this one emulates the old harness' RunTest mechanism of using ij.main("-fr", "filename") (by extending util.IjTestCase), whereas the other tests use ij.runScript(..). -fr is possibly intended to be a undocumented feature, at least, I could find no documentation, even the ij javadoc is very quiet about it, and if you use it incorrectly, (e.g. : java org.apache.derby.tools.ij testclientij.sql) you get an NPE rather than an appropriate message. I gather it's supposed to list the file in it's full resource path - e.g.: java org.apache.derby.tools.ij /testclientij.sql (if . is in your classpath and the file sits in the current dir). Apparently, and per the code, running main vs runScript results in some slight differences: no version number printed first connection is already started no messages indicating number of rows selected These differences do not seem relevant to this particular test. I think the util.IjTestCase mechanism may possibly be of use in testing ij in the future, but at present, in NetIjTest, it appears to be just another way of doing the same thing as e.g. NistScripts, a way that has the unfortunate effect of printing the version number. We could consider adding the script to the tools directory/_Suite...But I think it is more appropriate in its current spot. Thus, I intend to rewrite NetIjTest to use the same mechanism as NistScripts. When this is done, I intend to get rid of the master-editing target in tools/release/build.xml. I'll leave IjTestCase in place for future use. If used, it would need a different compareCanon method in CanonTestCase, one that ignores lines with specified strings (or maybe specifically checks on the version number without it being in the canon).
          Hide
          Myrna van Lunteren added a comment -

          I rewrote the test NetIjTest which uses the remaining master file - testclientij.out - that shows the version and committed with revision 618472.
          Also with this revision I removed the now unnecessary regex.masters target.

          I then edited the wiki page http://wiki.apache.org/db-derby/DerbySnapshotOrRelease and removed the references to the now no longer existing targets.

          Show
          Myrna van Lunteren added a comment - I rewrote the test NetIjTest which uses the remaining master file - testclientij.out - that shows the version and committed with revision 618472. Also with this revision I removed the now unnecessary regex.masters target. I then edited the wiki page http://wiki.apache.org/db-derby/DerbySnapshotOrRelease and removed the references to the now no longer existing targets.

            People

            • Assignee:
              Myrna van Lunteren
              Reporter:
              Myrna van Lunteren
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development