Derby
  1. Derby
  2. DERBY-5300

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

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • 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. derby-5300-1a-print_jdbc_client.diff
        0.9 kB
        Kristian Waagan
      2. classinfixture_Aug182011.txt
        2 kB
        Jayaram Subramanian
      3. svnstat-classinfixture.txt
        0.8 kB
        Jayaram Subramanian
      4. classinfixture-Oct132011.diff
        2 kB
        Jayaram Subramanian
      5. classinfixture-stat-oct132011.out
        0.8 kB
        Jayaram Subramanian
      6. classinfixture-oct15.diff
        2 kB
        Jayaram Subramanian
      7. classinfixture-stat-oct15.txt
        0.8 kB
        Jayaram Subramanian

        Activity

        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 .
        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
        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
        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
        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).
        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
        Hide
        Knut Anders Hatlen added a comment -

        [bulk update] Close all resolved issues that haven't been updated for more than one year.

        Show
        Knut Anders Hatlen added a comment - [bulk update] Close all resolved issues that haven't been updated for more than one year.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development