Derby
  1. Derby
  2. DERBY-5300

Change derby.tests.trace to print the class as well as fixture name

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 10.9.1.0
    • Fix Version/s: 10.8.3.3, 10.9.1.0
    • Component/s: Test
    • Labels:
      None
    • Issue & fix info:
      Newcomer

      Description

      I was thinking it would be good for the test output with -Dderby.tests.trace=true to have the class name as well as the fixture as I think if I had a nickel for every time I grepped for a fixture name to find out what class it is in, I would have a pretty big piggy bank.

      It could print the full class name, like this:
      org.apache.derbyTesting.functionTests.tests.lang.SimpleTest.testBasicOperations used 844 ms .

      or strip off the org.apache.derbyTesting.functionTests for less output like:

      tests.lang.SimpleTest.testBugFixes used 6265 ms .

      Any preferences?

      1. classinfixture_Aug182011.txt
        2 kB
        Jayaram Subramanian
      2. classinfixture-Oct132011.diff
        2 kB
        Jayaram Subramanian
      3. classinfixture-oct15.diff
        2 kB
        Jayaram Subramanian
      4. classinfixture-stat-oct132011.out
        0.8 kB
        Jayaram Subramanian
      5. classinfixture-stat-oct15.txt
        0.8 kB
        Jayaram Subramanian
      6. derby-5300-1a-print_jdbc_client.diff
        0.9 kB
        Kristian Waagan
      7. svnstat-classinfixture.txt
        0.8 kB
        Jayaram Subramanian

        Activity

        Kathey Marsden created issue -
        Hide
        Kristian Waagan added a comment -

        I'd prefer a shorter version, such that the lines stays below 80 chars in most cases (looks better in terminal windows with default size).

        Additionally, I'd like to see a hint telling me if the test ran with the embedded or the client driver (something along the lines added by the attached patch 1a):
        (emb)testGetStreamsExceptionOnClose used 663 ms F.
        (emb)testSetMaxFieldSizeLarge used 317 ms .
        (net)testInsertData used 21 ms .
        (net)testBinaryStreamProcessing used 18 ms .

        I chose 'emb' and 'net' because their lengths are equal, but maybe some find 'net' a bit confusing due to the old DB2 driver? Could also do '(E)' and '(C)' or something.

        Ideally, I'd like to see this hint in the error/failure report as well, but I'm not sure if that can be done easily enough to be worth the effort .

        Show
        Kristian Waagan added a comment - I'd prefer a shorter version, such that the lines stays below 80 chars in most cases (looks better in terminal windows with default size). Additionally, I'd like to see a hint telling me if the test ran with the embedded or the client driver (something along the lines added by the attached patch 1a): (emb)testGetStreamsExceptionOnClose used 663 ms F. (emb)testSetMaxFieldSizeLarge used 317 ms . (net)testInsertData used 21 ms . (net)testBinaryStreamProcessing used 18 ms . I chose 'emb' and 'net' because their lengths are equal, but maybe some find 'net' a bit confusing due to the old DB2 driver? Could also do '(E)' and '(C)' or something. Ideally, I'd like to see this hint in the error/failure report as well, but I'm not sure if that can be done easily enough to be worth the effort .
        Kristian Waagan made changes -
        Field Original Value New Value
        Attachment derby-5300-1a-print_jdbc_client.diff [ 12484417 ]
        Kristian Waagan made changes -
        Attachment derby-5090-1a-fix.diff [ 12484424 ]
        Attachment derby-5090-1a-fix.stat [ 12484425 ]
        Attachment derby-5090-2a-test.diff [ 12484426 ]
        Attachment derby-5090-2a-test.stat [ 12484427 ]
        Kristian Waagan made changes -
        Comment [ Attaching patches 1a and 2a.
        Patch 1a is the fix, including some clean-up:

        * java/engine/org/apache/derby/impl/jdbc/EmbedResultSet.java
        Replace NewByteArrayInputStream with ByteArrayInputStream
        Wrap the above stream in CloseFilterInputStream

        * java/engine/org/apache/derby/iapi/services/io/CloseFilterInputStream.java
        Added filter stream that will throw exception after being closed. Modeled after the equally named class in the client.

        * java/engine/org/apache/derby/iapi/services/io/NewByteArrayInputStream.java
        Deleted this class, use a standard API-class instead.

        * java/engine/org/apache/derby/iapi/services/io/AccessibleByteArrayOutputStream.java
        Replace NewByteArrayInputStream with ByteArrayInputStream

        * java/client/org/apache/derby/client/am/ResultSet.java
        Renamed is_ to currentStream.
        Added instance variable currentReader
        Renamed method closeCloseFilterInputStream to closeOpenStreams
        Modified closeOpenStream to close both currentStream and currentReader (only one will be non-null at any time)
        Assigned reader to currentReader in getCharacterStream

        Patch 2b adds two tests, but I'm a bit unsure if I want to keep JDBCSetGet as a separate class. Will anyone else use it?
        If not, it might be better to keep them as private classes in the test class.
        If we want to add it as a JUnit utility class, should it be split in two, i.e. JDBCGet and JDBCSet? Also, if keeping I'd like to make it possible to obtain the name of the last called getter to improve the error reporting.

        Patches ready for review.
        ]
        Kristian Waagan made changes -
        Attachment derby-5090-1a-fix.diff [ 12484424 ]
        Kristian Waagan made changes -
        Attachment derby-5090-1a-fix.stat [ 12484425 ]
        Kristian Waagan made changes -
        Attachment derby-5090-2a-test.diff [ 12484426 ]
        Kristian Waagan made changes -
        Attachment derby-5090-2a-test.stat [ 12484427 ]
        Jayaram Subramanian made changes -
        Assignee Jayaram Subramanian [ rsjay1976 ]
        Hide
        Kathey Marsden added a comment -

        I think maybe (emb) and (cli) as then it will be clear.

        So perhaps a good format would be to strip off "org.apache.derbyTesting.functionTests.tests." where it exists. If a test is run from a class that does not have that prefix, we would print the whole thing, so output would typically look like:

        (emb) lang.SimpleTest.testBasicOperations used 1672 ms .
        (cli) lang.SimpleTest.testBasicOperations used 1703 ms .

        I wonder if it would work to call setName to include this information so it will print with the failure traces.

        Show
        Kathey Marsden added a comment - I think maybe (emb) and (cli) as then it will be clear. So perhaps a good format would be to strip off "org.apache.derbyTesting.functionTests.tests." where it exists. If a test is run from a class that does not have that prefix, we would print the whole thing, so output would typically look like: (emb) lang.SimpleTest.testBasicOperations used 1672 ms . (cli) lang.SimpleTest.testBasicOperations used 1703 ms . I wonder if it would work to call setName to include this information so it will print with the failure traces.
        Hide
        Jayaram Subramanian added a comment -

        On doing getclass().getName0 in basetestcase.java i end up something like
        org.apache.derbyTesting.functionTests.tests.management.JMXTesttestDerbyRegisteredMBeansSimpleInfo used 13 ms

        But i am not able to figure out best way to strip off org.apache.derbyTesting.functionTests.tests. since i dont think we should be hardcoding it in basetestcase.

        is there any better way to get rid off org.apache.derbyTesting.functionTests.tests in the logs?

        Show
        Jayaram Subramanian added a comment - On doing getclass().getName0 in basetestcase.java i end up something like org.apache.derbyTesting.functionTests.tests.management.JMXTesttestDerbyRegisteredMBeansSimpleInfo used 13 ms But i am not able to figure out best way to strip off org.apache.derbyTesting.functionTests.tests. since i dont think we should be hardcoding it in basetestcase. is there any better way to get rid off org.apache.derbyTesting.functionTests.tests in the logs?
        Hide
        Jayaram Subramanian added a comment -

        Attaching the patch

        Show
        Jayaram Subramanian added a comment - Attaching the patch
        Jayaram Subramanian made changes -
        Attachment classinfixture_Aug182011.txt [ 12491029 ]
        Attachment svnstat-classinfixture.txt [ 12491030 ]
        Jayaram Subramanian made changes -
        Issue & fix info [Newcomer] [Newcomer, Patch Available]
        Hide
        Kristian Waagan added a comment -

        I think we have no choice but to hardcode the most common class prefix if we want to shorten the output.
        However, you should make sure the prefix exists before trying to replace it, for instance by using String.startsWith(COMMON_CLASS_PREFIX).

        There's already a reference to the current configuration in runBare, you could consider writing a new (non-static) method to print the trace string.

        Show
        Kristian Waagan added a comment - I think we have no choice but to hardcode the most common class prefix if we want to shorten the output. However, you should make sure the prefix exists before trying to replace it, for instance by using String.startsWith(COMMON_CLASS_PREFIX). There's already a reference to the current configuration in runBare, you could consider writing a new (non-static) method to print the trace string.
        Hide
        Kristian Waagan added a comment -

        This issue describes a useful improvement.
        Jayaram, do you need any more feedback from the community to wrap up the patch?

        Show
        Kristian Waagan added a comment - This issue describes a useful improvement. Jayaram, do you need any more feedback from the community to wrap up the patch?
        Hide
        Jayaram Subramanian added a comment -

        Thanks Kristian,
        I already attached a patch as a fix. See the files classinfixture_Aug182011.txt and svnstat-classinfixture.txt attached to the jira.

        Show
        Jayaram Subramanian added a comment - Thanks Kristian, I already attached a patch as a fix. See the files classinfixture_Aug182011.txt and svnstat-classinfixture.txt attached to the jira.
        Hide
        Kristian Waagan added a comment -

        I already commented on that patch (see comment from 05/Sep/11 10:17).
        There are two issues with it that must be fixed:
        o a missing dot between the class name and the fixture/method name:
        (emb)lang.CollationTest2testDefaultCollation

        o it assumes the prefix is always present, which isn't the case:
        java ... junit.textui.TestRunner org.apache.derbyTesting.unitTests.junit._Suite
        (emb)ayInputStreamTesttestSkipLongMaxValue used 441 ms .
        (emb)ayInputStreamTesttestSkipBytesIntMaxValue used 0 ms .
        (emb)ayInputStreamTesttestSkipNegative used 0 ms .
        (emb)ayInputStreamTesttestSkipBytesNegative used 0 ms .
        (emb)matableBitSetTesttestSetup used 61 ms .
        (emb)matableBitSetTesttestIntCtor0 used 0 ms .
        (emb)matableBitSetTesttestIntCtor1 used 0 ms .

        I don't know if it's worth it, but we could consider doing the shortening in steps:
        a) Check if the prefix is 'org.apache.derbyTesting.functionTests.tests.'
        b) Check if the prefix is 'org.apache.derbyTesting.'
        c) Otherwise use fully qualified class name.

        Thanks,

        Show
        Kristian Waagan added a comment - I already commented on that patch (see comment from 05/Sep/11 10:17). There are two issues with it that must be fixed: o a missing dot between the class name and the fixture/method name: (emb)lang.CollationTest2testDefaultCollation o it assumes the prefix is always present, which isn't the case: java ... junit.textui.TestRunner org.apache.derbyTesting.unitTests.junit._Suite (emb)ayInputStreamTesttestSkipLongMaxValue used 441 ms . (emb)ayInputStreamTesttestSkipBytesIntMaxValue used 0 ms . (emb)ayInputStreamTesttestSkipNegative used 0 ms . (emb)ayInputStreamTesttestSkipBytesNegative used 0 ms . (emb)matableBitSetTesttestSetup used 61 ms . (emb)matableBitSetTesttestIntCtor0 used 0 ms . (emb)matableBitSetTesttestIntCtor1 used 0 ms . I don't know if it's worth it, but we could consider doing the shortening in steps: a) Check if the prefix is 'org.apache.derbyTesting.functionTests.tests.' b) Check if the prefix is 'org.apache.derbyTesting.' c) Otherwise use fully qualified class name. Thanks,
        Hide
        Jayaram Subramanian added a comment -

        Thanks for your suggestions Kristian,
        Attaching the patch addressing the 2 issues.. i am little not sure about your comments on invoking static method.. please review and let me know if i had missed something

        Show
        Jayaram Subramanian added a comment - Thanks for your suggestions Kristian, Attaching the patch addressing the 2 issues.. i am little not sure about your comments on invoking static method.. please review and let me know if i had missed something
        Jayaram Subramanian made changes -
        Attachment classinfixture-Oct132011.diff [ 12498875 ]
        Attachment classinfixture-stat-oct132011.out [ 12498876 ]
        Hide
        Kristian Waagan added a comment -

        Thanks, Jayaram.

        The newest patch looks ok to me - it does the right things.

        Some possible improvements:
        o there are no method JavaDoc
        o is the formatSubString a good method name for this method?
        To me it seems the method is specifically targeted at formatting test class names.
        o COMMON_[FUNCTION]TEST_PREFIX could be constants (private static final)
        o since you use startsWith and want the rest of the string,
        mainString.substring(mainString.indexOf(COMMON_FUNCTIONTEST_PREFIX)+COMMON_FUNCTIONTEST_PREFIX.length(),mainString.length());
        could be simplified to
        mainString.substring(COMMON_FUNCTIONTEST_PREFIX.length())
        o inconsistent use of tabs and spaces for indentation
        o mix of tabs and spaces for indentation (on the same line)
        o inconsistent placement of braces in formatSubString (either one is fine, but it's usually easier to read code where the placement is consistent)

        It's up to you which of the suggestions, if any, you want to address In my opinion, they would increase the code quality. Feel free to ask questions if you have any.

        Regards,

        Show
        Kristian Waagan added a comment - Thanks, Jayaram. The newest patch looks ok to me - it does the right things. Some possible improvements: o there are no method JavaDoc o is the formatSubString a good method name for this method? To me it seems the method is specifically targeted at formatting test class names. o COMMON_ [FUNCTION] TEST_PREFIX could be constants (private static final) o since you use startsWith and want the rest of the string, mainString.substring(mainString.indexOf(COMMON_FUNCTIONTEST_PREFIX)+COMMON_FUNCTIONTEST_PREFIX.length(),mainString.length()); could be simplified to mainString.substring(COMMON_FUNCTIONTEST_PREFIX.length()) o inconsistent use of tabs and spaces for indentation o mix of tabs and spaces for indentation (on the same line) o inconsistent placement of braces in formatSubString (either one is fine, but it's usually easier to read code where the placement is consistent) It's up to you which of the suggestions, if any, you want to address In my opinion, they would increase the code quality. Feel free to ask questions if you have any. Regards,
        Hide
        Jayaram Subramanian added a comment -

        Thanks Kristian,
        Attachin the patch..Implemented all your suggestions except private final static comment.. my eclipse compiler was complaining that only final is permitted in that context. i am using jdk 1.6

        Show
        Jayaram Subramanian added a comment - Thanks Kristian, Attachin the patch..Implemented all your suggestions except private final static comment.. my eclipse compiler was complaining that only final is permitted in that context. i am using jdk 1.6
        Jayaram Subramanian made changes -
        Attachment classinfixture-oct15.diff [ 12499195 ]
        Attachment classinfixture-stat-oct15.txt [ 12499196 ]
        Hide
        Kristian Waagan added a comment -

        Thanks for following up on this issue, Jayaram.

        I committed the patch to trunk with revision 1185330.
        I took the liberty to replace the tabs with spaces.

        You can now update the JIRA issue, i.e. resolve the issue if you believe your work is done.

        BTW, regarding the private final static comment, to do that you would have to move the constants out of the method and make them class variables (not that it matters for this issue).

        Show
        Kristian Waagan added a comment - Thanks for following up on this issue, Jayaram. I committed the patch to trunk with revision 1185330. I took the liberty to replace the tabs with spaces. You can now update the JIRA issue, i.e. resolve the issue if you believe your work is done. BTW, regarding the private final static comment, to do that you would have to move the constants out of the method and make them class variables (not that it matters for this issue).
        Kristian Waagan made changes -
        Fix Version/s 10.9.0.0 [ 12316344 ]
        Issue & fix info Patch Available,Newcomer [ 10102,10423 ] Newcomer [ 10423 ]
        Hide
        Kristian Waagan added a comment -

        Changed code to agree with the Javadoc param comment with revision 1185465.
        This should take care of the Jenkins warning posted to derby-dev.

        Show
        Kristian Waagan added a comment - Changed code to agree with the Javadoc param comment with revision 1185465. This should take care of the Jenkins warning posted to derby-dev.
        Hide
        Jayaram Subramanian added a comment -

        Resolving as per suggestions

        Show
        Jayaram Subramanian added a comment - Resolving as per suggestions
        Jayaram Subramanian made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Kathey Marsden made changes -
        Fix Version/s 10.8.3.1 [ 12323475 ]
        Gavin made changes -
        Workflow jira [ 12618245 ] Default workflow, editable Closed status [ 12802285 ]

          People

          • Assignee:
            Jayaram Subramanian
            Reporter:
            Kathey Marsden
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development