Derby
  1. Derby
  2. DERBY-3156

Convert testing of derby error stream to JUnit

    Details

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

      Description

      Convert the tests that are related to derby error stream properties and functionality.

      Seems to be these two tests:
      lang/logStream.java
      lang/errorStream.java

      1. 3156-diff.stat
        0.5 kB
        Vemund Østgaard
      2. 3156-diff
        25 kB
        Vemund Østgaard
      3. 3156-diffv2
        29 kB
        Vemund Østgaard
      4. 3156-diffv2.stat
        0.5 kB
        Vemund Østgaard
      5. 3156-diffv3
        29 kB
        Vemund Østgaard
      6. 3156-diffv3.stat
        0.5 kB
        Vemund Østgaard
      7. 3156-diffv4
        29 kB
        Vemund Østgaard
      8. 3156-diffv4.stat
        0.5 kB
        Vemund Østgaard
      9. 3156-remove-old-tests-diff
        23 kB
        Vemund Østgaard
      10. 3156-remove-old-tests-diff.stat
        0.4 kB
        Vemund Østgaard

        Issue Links

          Activity

          Hide
          Vemund Østgaard added a comment -

          This patch removes the old lang/errorStream.java and lang/logStream.java tests from derbylang.runall, so they will no longer be run as part of the old harness testing. I have not removed the test code.

          A new test, ErrorStreamTest, is added that runs in the JUnit framework.
          Feeling that this test didn't really belong in the "lang" suite in the first place, I've created a new suite named "engine" for this test and added this suite to the AllPackages suite that is run as part of suites.All. I don't have any strong feelings about where the test really belongs, but from my limited knowledge of the testing this seemed to make most sense.

          The new test runs only in the embedded environment. When I ran it in a networkServer environment it failed with a problem that I logged as a new Jira DERBY-3170. I don't think the bug will have a high priority so I've submitted this version of the test running only in the embedded environment so it will not fail.

          Show
          Vemund Østgaard added a comment - This patch removes the old lang/errorStream.java and lang/logStream.java tests from derbylang.runall, so they will no longer be run as part of the old harness testing. I have not removed the test code. A new test, ErrorStreamTest, is added that runs in the JUnit framework. Feeling that this test didn't really belong in the "lang" suite in the first place, I've created a new suite named "engine" for this test and added this suite to the AllPackages suite that is run as part of suites.All. I don't have any strong feelings about where the test really belongs, but from my limited knowledge of the testing this seemed to make most sense. The new test runs only in the embedded environment. When I ran it in a networkServer environment it failed with a problem that I logged as a new Jira DERBY-3170 . I don't think the bug will have a high priority so I've submitted this version of the test running only in the embedded environment so it will not fail.
          Hide
          Daniel John Debrunner added a comment -

          The test contains a number of static fields, do they all need to be static? Static fields will lead to problems and are not typical in JUnit tests, each test fixture(testXXX method) is an unique instance of a class so either instance fields should be used or even better just pass the information around. E.g. the setUp method will be called for every fixture, so there's no real benefit to storing information in fields since they will not be shared across fixtures.

          Can you comment the reason the test doesn't run with a security manager.

          Show
          Daniel John Debrunner added a comment - The test contains a number of static fields, do they all need to be static? Static fields will lead to problems and are not typical in JUnit tests, each test fixture(testXXX method) is an unique instance of a class so either instance fields should be used or even better just pass the information around. E.g. the setUp method will be called for every fixture, so there's no real benefit to storing information in fields since they will not be shared across fixtures. Can you comment the reason the test doesn't run with a security manager.
          Hide
          Vemund Østgaard added a comment -

          Thank you for looking at the patch, Dan.

          I've now uploaded a new patch (3156-diffv2) with the following changes, based on the comments:

          I looked at the use of static fields. A few fields have to be static for the test to work (code accessed by Derby in a static context), but there shouldn't be any static fields now that don't have to be.

          I had postponed making the test work with a security manager, and forgot about it when I submitted the patch on Friday. I've looked at it now and believe I have it working with the security manager the way it should.

          I added 10 different wrapper-methods for the doPrivileged stuff related to accessing files and redirecting System.err. I guess some of these might be useful enough to move to BaseTestCase for reuse in other tests, but I left them in ErrorStreamTest for the time being.

          Show
          Vemund Østgaard added a comment - Thank you for looking at the patch, Dan. I've now uploaded a new patch (3156-diffv2) with the following changes, based on the comments: I looked at the use of static fields. A few fields have to be static for the test to work (code accessed by Derby in a static context), but there shouldn't be any static fields now that don't have to be. I had postponed making the test work with a security manager, and forgot about it when I submitted the patch on Friday. I've looked at it now and believe I have it working with the security manager the way it should. I added 10 different wrapper-methods for the doPrivileged stuff related to accessing files and redirecting System.err. I guess some of these might be useful enough to move to BaseTestCase for reuse in other tests, but I left them in ErrorStreamTest for the time being.
          Hide
          Knut Anders Hatlen added a comment - - edited

          Hi Vemund,

          The patch generally looks good. A couple of comments/questions:

          1) Do the original tests run with the client driver? It seems to me they don't, so I think it's okay to run the new test only in embedded mode.

          2) I think it would be good to null out the fields when the test has completed (in tearDown()) so that the objects they reference can be garbage collected.

          3) Instead of using File.deleteOnExit(), perhaps it's cleaner to delete the files explicitly in tearDown()? Seems that it would also remove the need for the runNo field.

          4) I'm wondering if this code

          + new File(getSystemProperty("derby.system.home") +"foo",
          + makeStreamFilename("file")).getCanonicalPath()); // erroneous path

          would be more robust if it was written as

          new File(new File(getSystemProperty("derby.system.home"), "foo"), ...)

          As it is now, I think it depends on derby.system.home ending with the path separator character.

          Show
          Knut Anders Hatlen added a comment - - edited Hi Vemund, The patch generally looks good. A couple of comments/questions: 1) Do the original tests run with the client driver? It seems to me they don't, so I think it's okay to run the new test only in embedded mode. 2) I think it would be good to null out the fields when the test has completed (in tearDown()) so that the objects they reference can be garbage collected. 3) Instead of using File.deleteOnExit(), perhaps it's cleaner to delete the files explicitly in tearDown()? Seems that it would also remove the need for the runNo field. 4) I'm wondering if this code + new File(getSystemProperty("derby.system.home") +"foo", + makeStreamFilename("file")).getCanonicalPath()); // erroneous path would be more robust if it was written as new File(new File(getSystemProperty("derby.system.home"), "foo"), ...) As it is now, I think it depends on derby.system.home ending with the path separator character.
          Hide
          Vemund Østgaard added a comment -

          Thank you for the comments Knut, I've uploaded a v3 of the patch that addresses your comments as follows:

          1. I don't think it has run with the client driver before, it would have failed with the same bug that I hit.

          2. The fields are now nulled out in tearDown().

          3. The files are now deleted directly in tearDown(). I haven't removed the use of the runNo field, somehow it gives me a warm feeling knowing that the filenames used are unique, so that the chance of one of the tests passing by accident is less (The correct setting for the property carries over from a previous execution or the file wasn't deleted and was accidentally there with the correct name, etc.). So, I've left the use of the runNo field as it was.

          4. I've changed the code as you suggest, I agree it is more predictable.

          Show
          Vemund Østgaard added a comment - Thank you for the comments Knut, I've uploaded a v3 of the patch that addresses your comments as follows: 1. I don't think it has run with the client driver before, it would have failed with the same bug that I hit. 2. The fields are now nulled out in tearDown(). 3. The files are now deleted directly in tearDown(). I haven't removed the use of the runNo field, somehow it gives me a warm feeling knowing that the filenames used are unique, so that the chance of one of the tests passing by accident is less (The correct setting for the property carries over from a previous execution or the file wasn't deleted and was accidentally there with the correct name, etc.). So, I've left the use of the runNo field as it was. 4. I've changed the code as you suggest, I agree it is more predictable.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for updating the patch, Vemund.

          I tried to run the test standalone, but then all test cases failed with errors like this one:

          1) testDefault(org.apache.derbyTesting.functionTests.tests.engine.ErrorStreamTest)java.io.FileNotFoundException: /tmp/derbytst/system
          /method-1.log (No such file or directory)
          at java.io.FileOutputStream.open(Native Method)
          at java.io.FileOutputStream.<init>(FileOutputStream.java:179)
          at java.io.FileOutputStream.<init>(FileOutputStream.java:131)
          at org.apache.derbyTesting.functionTests.tests.engine.ErrorStreamTest$9.run(ErrorStreamTest.java:502)
          at java.security.AccessController.doPrivileged(Native Method)
          at org.apache.derbyTesting.functionTests.tests.engine.ErrorStreamTest.newFileOutputStream(ErrorStreamTest.java:499)
          at org.apache.derbyTesting.functionTests.tests.engine.ErrorStreamTest.openStreams(ErrorStreamTest.java:360)
          at org.apache.derbyTesting.functionTests.tests.engine.ErrorStreamTest.setUp(ErrorStreamTest.java:100)
          at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:95)

          If I first run another test and don't delete the 'system' directory, it works fine. Seems like openStreams() requires that the database directory exists.

          nits:

          The method deleteFileOnExit() is now unused and can be deleted.

          Perhaps the assert methods that throw IOException would have been simpler if they had thrown Exception instead? Then we could skip the try/catch+cast. The PrivilegedActionException will be linked to the IOException, so we will still get the stack trace, and if someone changes the code so that we can get other exceptions than IOException, we get the original exception instead of a ClassCastException.

          Show
          Knut Anders Hatlen added a comment - Thanks for updating the patch, Vemund. I tried to run the test standalone, but then all test cases failed with errors like this one: 1) testDefault(org.apache.derbyTesting.functionTests.tests.engine.ErrorStreamTest)java.io.FileNotFoundException: /tmp/derbytst/system /method-1.log (No such file or directory) at java.io.FileOutputStream.open(Native Method) at java.io.FileOutputStream.<init>(FileOutputStream.java:179) at java.io.FileOutputStream.<init>(FileOutputStream.java:131) at org.apache.derbyTesting.functionTests.tests.engine.ErrorStreamTest$9.run(ErrorStreamTest.java:502) at java.security.AccessController.doPrivileged(Native Method) at org.apache.derbyTesting.functionTests.tests.engine.ErrorStreamTest.newFileOutputStream(ErrorStreamTest.java:499) at org.apache.derbyTesting.functionTests.tests.engine.ErrorStreamTest.openStreams(ErrorStreamTest.java:360) at org.apache.derbyTesting.functionTests.tests.engine.ErrorStreamTest.setUp(ErrorStreamTest.java:100) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:95) If I first run another test and don't delete the 'system' directory, it works fine. Seems like openStreams() requires that the database directory exists. nits: The method deleteFileOnExit() is now unused and can be deleted. Perhaps the assert methods that throw IOException would have been simpler if they had thrown Exception instead? Then we could skip the try/catch+cast. The PrivilegedActionException will be linked to the IOException, so we will still get the stack trace, and if someone changes the code so that we can get other exceptions than IOException, we get the original exception instead of a ClassCastException.
          Hide
          Vemund Østgaard added a comment -

          Thanks again Knut, made a new patch and hope it works better now. It had worked standalone until I made the last change in the previous patch, of course...

          Also removed the deleteFileOnExit() method.

          About the IOExceptions I'm not really sure. This:

          public Object run() throws IOException {

          will not compile if the code in run() suddenly starts throwing other exceptions, so we shouldn't be getting any ClassCastException from the catch() all of a sudden. If it did change to throw other exceptions you might want these assertmethods to throw both exceptions without any wrapping to make it possible to make different choices easily. I also think the methods are more useful when they wrap away the uninteresting PrivilegedActionException for you.

          Show
          Vemund Østgaard added a comment - Thanks again Knut, made a new patch and hope it works better now. It had worked standalone until I made the last change in the previous patch, of course... Also removed the deleteFileOnExit() method. About the IOExceptions I'm not really sure. This: public Object run() throws IOException { will not compile if the code in run() suddenly starts throwing other exceptions, so we shouldn't be getting any ClassCastException from the catch() all of a sudden. If it did change to throw other exceptions you might want these assertmethods to throw both exceptions without any wrapping to make it possible to make different choices easily. I also think the methods are more useful when they wrap away the uninteresting PrivilegedActionException for you.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks Vemund! I have committed your patch to trunk (revision 594169). Should we also remove the old tests before we close the issue?

          Show
          Knut Anders Hatlen added a comment - Thanks Vemund! I have committed your patch to trunk (revision 594169). Should we also remove the old tests before we close the issue?
          Hide
          Knut Anders Hatlen added a comment -

          Seems like this caused an AccessControlException on Windows. Please take a look at DERBY-3202.

          Show
          Knut Anders Hatlen added a comment - Seems like this caused an AccessControlException on Windows. Please take a look at DERBY-3202 .
          Hide
          Vemund Østgaard added a comment -

          Added a patch 3156-remove-old-tests-diff that removes the files for the old tests that is no longer in use:
          errorStream.java
          errorStream.policy
          errorStream_app.properties
          logStream.java
          logStream_app.properties

          Show
          Vemund Østgaard added a comment - Added a patch 3156-remove-old-tests-diff that removes the files for the old tests that is no longer in use: errorStream.java errorStream.policy errorStream_app.properties logStream.java logStream_app.properties
          Hide
          Knut Anders Hatlen added a comment -

          Thanks Vemund. Committed revision 596285.

          I also removed lang/errorStream.java from DerbyNet.exclude and DerbyNetClient.exclude.

          Show
          Knut Anders Hatlen added a comment - Thanks Vemund. Committed revision 596285. I also removed lang/errorStream.java from DerbyNet.exclude and DerbyNetClient.exclude.
          Hide
          Mike Matrigali added a comment -

          I am seeing junit failures in this test when run on j9 - see DERBY-3217. Unfortunately the machine running these
          tests had a number of problems so I don't know exactly when the issue started, but it has been happening at least
          since 11/12/2007.

          Show
          Mike Matrigali added a comment - I am seeing junit failures in this test when run on j9 - see DERBY-3217 . Unfortunately the machine running these tests had a number of problems so I don't know exactly when the issue started, but it has been happening at least since 11/12/2007.
          Hide
          Knut Anders Hatlen added a comment -

          Could we also remove the canons now? (errorStream.out and logStream.out)

          Show
          Knut Anders Hatlen added a comment - Could we also remove the canons now? (errorStream.out and logStream.out)
          Hide
          Knut Anders Hatlen added a comment -

          Removed errorStream.out and logStream.out and committed revision 597356.

          Show
          Knut Anders Hatlen added a comment - Removed errorStream.out and logStream.out and committed revision 597356.

            People

            • Assignee:
              Vemund Østgaard
              Reporter:
              Vemund Østgaard
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development