Derby
  1. Derby
  2. DERBY-1802

JUnit tests for DERBY-475 and DERBY-592, Built-in and JDBC escape functions

    Details

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

      Description

      Attached is a patch which contains JUnit tests for DERBY-475 and DERBY-592, the built-in math functions and JDBC escape functions.

      I ran the patch, DERBY-475_DERBY-592_20060831.diff, against these four tests:

      java junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.lang.MathTrigFunctionsTest
      java org.apache.derbyTesting.functionTests.harness.RunTest lang/MathTrigFunctionsTest.junit
      java org.apache.derbyTesting.functionTests.harness.RunTest lang/_Suite.junit
      java org.apache.derbyTesting.functionTests.harness.RunSuite derbylang

      svn status:

      A java\testing\org\apache\derbyTesting\functionTests\tests\lang\MathTrigFunctionsTest.java
      M java\testing\org\apache\derbyTesting\functionTests\tests\lang_Suite.java
      M java\testing\org\apache\derbyTesting\functionTests\tests\lang\LangScripts.java
      (Note: I edited this file, but changed it back to the original contents.)

      I created the patch on Windows XP, tested it on Windows XP and then applied the patch on Linux and ran the above 4 tests on Linux.

      sysinfo output:
      ------------------ Java Information ------------------
      Java Version: 1.4.2_09
      Java Vendor: Sun Microsystems Inc.
      Java home: C:\JDK\jdk1.4.2_09\jre
      Java classpath: C:\derby_src\development_branch\trunk\classes;C:\derby_src\deve
      lopment_branch\trunk\tools\java\junit.jar;C:\derby_src\development_branch\trunk\
      tools\java\jakarta-oro-2.0.8.jar;.
      OS name: Windows XP
      OS architecture: x86
      OS version: 5.1
      Java user name: slc
      Java user home: C:\Documents and Settings\Administrator
      Java user dir: C:\derby_src\development_branch\trunk
      java.specification.name: Java Platform API Specification
      java.specification.version: 1.4
      --------- Derby Information --------
      JRE - JDBC: J2SE 1.4.2 - JDBC 3.0
      [C:\derby_src\development_branch\trunk\classes] 10.3.0.0 alpha - (1)
      ------------------------------------------------------
      ----------------- Locale Information -----------------
      Current Locale : [English/United States [en_US]]
      Found support for locale: [de_DE]
      version: 10.3.0.0 alpha - (1)
      Found support for locale: [es]
      version: 10.3.0.0 alpha - (1)
      Found support for locale: [fr]
      version: 10.3.0.0 alpha - (1)
      Found support for locale: [it]
      version: 10.3.0.0 alpha - (1)
      Found support for locale: [ja_JP]
      version: 10.3.0.0 alpha - (1)
      Found support for locale: [ko_KR]
      version: 10.3.0.0 alpha - (1)
      Found support for locale: [pt_BR]
      version: 10.3.0.0 alpha - (1)
      Found support for locale: [zh_CN]
      version: 10.3.0.0 alpha - (1)
      Found support for locale: [zh_TW]
      version: 10.3.0.0 alpha - (1)
      ------------------------------------------------------

      I would appreciate it if a committer would commit this patch.

      Thanks,

      Susan

        Activity

        Hide
        Myrna van Lunteren added a comment -

        Seems to me this was fixed in 10.3 and backported to 10.2.

        Show
        Myrna van Lunteren added a comment - Seems to me this was fixed in 10.3 and backported to 10.2.
        Hide
        Daniel John Debrunner added a comment -

        Patch DERBY-475_DERBY-592_20060905.diff Committed revision 441097 - thanks Susan.

        Show
        Daniel John Debrunner added a comment - Patch DERBY-475 _ DERBY-592 _20060905.diff Committed revision 441097 - thanks Susan.
        Hide
        Kristian Waagan added a comment -

        Yes, you could do that. By default, output would be disabled. Output can be enabled by passing -Dderby.tests.debug=true, or toggle the debug switch in the TestConfiguration-object.
        Currently, output will be prepended by "DEBUG: ".
        A trivial thing, but I guess there is no println without arguments, so you would have to pass an empty string to get a newline.

        The code for println can be found in BaseTestCase, and the enabling of the debugmode can be found in TestConfiguration, I believe.

        I don't think this feature has been used much. If you find it awkward, please suggest changes to it.

        Show
        Kristian Waagan added a comment - Yes, you could do that. By default, output would be disabled. Output can be enabled by passing -Dderby.tests.debug=true, or toggle the debug switch in the TestConfiguration-object. Currently, output will be prepended by "DEBUG: ". A trivial thing, but I guess there is no println without arguments, so you would have to pass an empty string to get a newline. The code for println can be found in BaseTestCase, and the enabling of the debugmode can be found in TestConfiguration, I believe. I don't think this feature has been used much. If you find it awkward, please suggest changes to it.
        Hide
        Susan Cline added a comment -

        Hi Kristian,

        Regarding 1) - I don't know how the test harness works, so do I just remove both debug methods, the static variable debugFlag and replace all calls to debug() with println()?

        Thanks,

        Susan

        Show
        Susan Cline added a comment - Hi Kristian, Regarding 1) - I don't know how the test harness works, so do I just remove both debug methods, the static variable debugFlag and replace all calls to debug() with println()? Thanks, Susan
        Hide
        Kristian Waagan added a comment -

        Hi Susan,

        Thanks for writing these tests
        I had a look and have a few comments. 2 and 3 are nits, use your own judgement.
        1) Is it possible to use the existing debug method 'println' instead of 'debug'?
        This can be enabled without recompiling the test by passing a property.

        2) What about a line of commentary to the private helper methods, especially the ones that don't take a value?
        I had to spend a little time thinking about why you would need that, until PI popped up in my head

        3) Since each test method is actually testing many things, explanatory messages in the assert-methods could be helpful (if they provide extra value).
        For instance in 'executeNullValues'; assertNull("Null output expected on null input", rValue)

        Regards,

        Show
        Kristian Waagan added a comment - Hi Susan, Thanks for writing these tests I had a look and have a few comments. 2 and 3 are nits, use your own judgement. 1) Is it possible to use the existing debug method 'println' instead of 'debug'? This can be enabled without recompiling the test by passing a property. 2) What about a line of commentary to the private helper methods, especially the ones that don't take a value? I had to spend a little time thinking about why you would need that, until PI popped up in my head 3) Since each test method is actually testing many things, explanatory messages in the assert-methods could be helpful (if they provide extra value). For instance in 'executeNullValues'; assertNull("Null output expected on null input", rValue) Regards,
        Hide
        Susan Cline added a comment -

        Hi Dan,

        I believe I have addressed all of your comments with the attached patch. Let me know if you have any others.

        I ran the patch, DERBY-475_DERBY-592_20060905.diff, against these four tests:

        java junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.lang.MathTrigFunctionsTest
        java org.apache.derbyTesting.functionTests.harness.RunTest lang/MathTrigFunctionsTest.junit
        java org.apache.derbyTesting.functionTests.harness.RunTest lang/_Suite.junit
        java org.apache.derbyTesting.functionTests.harness.RunSuite derbylang

        svn status:

        A java\testing\org\apache\derbyTesting\functionTests\tests\lang\MathTrigFunctionsTest.java
        M java\testing\org\apache\derbyTesting\functionTests\tests\lang_Suite.java

        I created the patch on Windows XP, tested it on Windows XP and then applied the patch on Linux and ran the above 4 tests on Linux.

        sysinfo output:

        ------------------ Java Information ------------------
        Java Version: 1.4.2_09
        Java Vendor: Sun Microsystems Inc.
        Java home: C:\JDK\jdk1.4.2_09\jre
        Java classpath: C:\derby_src\development_branch\trunk\classes;C:\derby_src\deve
        lopment_branch\trunk\tools\java\junit.jar;C:\derby_src\development_branch\trunk\
        tools\java\jakarta-oro-2.0.8.jar;
        OS name: Windows XP
        OS architecture: x86
        OS version: 5.1
        Java user name: slc
        Java user home: C:\Documents and Settings\Administrator
        Java user dir: C:\derby_src\development_branch\trunk
        java.specification.name: Java Platform API Specification
        java.specification.version: 1.4
        --------- Derby Information --------
        JRE - JDBC: J2SE 1.4.2 - JDBC 3.0
        [C:\derby_src\development_branch\trunk\classes] 10.3.0.0 alpha - (1)
        ------------------------------------------------------
        ----------------- Locale Information -----------------
        Current Locale : [English/United States [en_US]]
        Found support for locale: [de_DE]
        version: 10.3.0.0 alpha - (1)
        Found support for locale: [es]
        version: 10.3.0.0 alpha - (1)
        Found support for locale: [fr]
        version: 10.3.0.0 alpha - (1)
        Found support for locale: [it]
        version: 10.3.0.0 alpha - (1)
        Found support for locale: [ja_JP]
        version: 10.3.0.0 alpha - (1)
        Found support for locale: [ko_KR]
        version: 10.3.0.0 alpha - (1)
        Found support for locale: [pt_BR]
        version: 10.3.0.0 alpha - (1)
        Found support for locale: [zh_CN]
        version: 10.3.0.0 alpha - (1)
        Found support for locale: [zh_TW]
        version: 10.3.0.0 alpha - (1)
        ------------------------------------------------------

        Thanks,

        Susan

        Show
        Susan Cline added a comment - Hi Dan, I believe I have addressed all of your comments with the attached patch. Let me know if you have any others. I ran the patch, DERBY-475 _ DERBY-592 _20060905.diff, against these four tests: java junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.lang.MathTrigFunctionsTest java org.apache.derbyTesting.functionTests.harness.RunTest lang/MathTrigFunctionsTest.junit java org.apache.derbyTesting.functionTests.harness.RunTest lang/_Suite.junit java org.apache.derbyTesting.functionTests.harness.RunSuite derbylang svn status: A java\testing\org\apache\derbyTesting\functionTests\tests\lang\MathTrigFunctionsTest.java M java\testing\org\apache\derbyTesting\functionTests\tests\lang_Suite.java I created the patch on Windows XP, tested it on Windows XP and then applied the patch on Linux and ran the above 4 tests on Linux. sysinfo output: ------------------ Java Information ------------------ Java Version: 1.4.2_09 Java Vendor: Sun Microsystems Inc. Java home: C:\JDK\jdk1.4.2_09\jre Java classpath: C:\derby_src\development_branch\trunk\classes;C:\derby_src\deve lopment_branch\trunk\tools\java\junit.jar;C:\derby_src\development_branch\trunk\ tools\java\jakarta-oro-2.0.8.jar; OS name: Windows XP OS architecture: x86 OS version: 5.1 Java user name: slc Java user home: C:\Documents and Settings\Administrator Java user dir: C:\derby_src\development_branch\trunk java.specification.name: Java Platform API Specification java.specification.version: 1.4 --------- Derby Information -------- JRE - JDBC: J2SE 1.4.2 - JDBC 3.0 [C:\derby_src\development_branch\trunk\classes] 10.3.0.0 alpha - (1) ------------------------------------------------------ ----------------- Locale Information ----------------- Current Locale : [English/United States [en_US] ] Found support for locale: [de_DE] version: 10.3.0.0 alpha - (1) Found support for locale: [es] version: 10.3.0.0 alpha - (1) Found support for locale: [fr] version: 10.3.0.0 alpha - (1) Found support for locale: [it] version: 10.3.0.0 alpha - (1) Found support for locale: [ja_JP] version: 10.3.0.0 alpha - (1) Found support for locale: [ko_KR] version: 10.3.0.0 alpha - (1) Found support for locale: [pt_BR] version: 10.3.0.0 alpha - (1) Found support for locale: [zh_CN] version: 10.3.0.0 alpha - (1) Found support for locale: [zh_TW] version: 10.3.0.0 alpha - (1) ------------------------------------------------------ Thanks, Susan
        Hide
        Daniel John Debrunner added a comment -

        Thanks for the comprehensive test Susan (much better than the minimal testing I submitted with the initial changes), some comments:

        • The apache licence header is the first item in a source file, before the package statement and imports, see other .java files for examples:
        • In Java constants like SMALLEST_NEG_DERBY_DOUBLE should be declared final
          private static final double SMALLEST_NEG_DERBY_DOUBLE = -1.79769E+308;
        • For the various constants it might be good to have comments around them indicting what they mean,
          for example it was unclear to me what was special about LITTLE_RADIANS that it needed a constant.
        • Maybe a style issue, but I prefer not to have constants like:
          QUARTER_RADIANS, NEG_QUARTER_RADIANS, HALF_RADIANS, ZERO_RADIANS etc.
          Are they really adding value? Or would the code be clearer just saying
          double radians = 0.25;
          Constants are usually used to hide the specific value (e.g. 0.25) and provide a logical name for understanding,
          here the constant's name is defining the specific value (QUARTER_RADIANS)
        • For PI instead of defining your own constant, PI_RADIANS, you can just use the one from StrictMath directly
          Similar, do the constants for 2*PI etc really add value?
        • For the methods executeNullValues and executeNullFn would it make sense to move the check that the return is
          NULL into the method and not have all the callers check it? Then you could also check within the method that only
          one row is returned and that the wasNull() indicator matched the state of the value.
        • You use static variables which will cause problems in the future when we want to run tests in parallel as there may be multiple
          threads sharing those varaibles as each runs an instance of this class. E.g.
          private static double rValue;

        Unless there's some special need I'm missing theie use seems to be self-contained within each test.
        The correct way to write this is to have local scope variables, not static fields, e.g.

        + for (int i = 0; i < testValues.length; i++)

        { + double expected = java.lang.StrictMath.tan(testValues[i]); <<<<<<< changed line + double rValue = executeValues("TAN", testValues[i]); <<<<<<< changed line + debug("TAN: input value: " + testValues[i] + " expected value: " + + expected + " return value: " + rValue); + assertEquals(expected, rValue, 0.0); + float fValue = executeFn("TAN", testValues[i]); <<<<<<< changed line + assertEquals(expected, fValue, 0.0); + }
        • For checkng the SQL State values
          + BaseJDBCTestCase
          + .assertSQLState(
          + "SQLState was NOT correct",
          + SQLStateConstants.DATA_EXCEPTION_NUMERIC_VALUE_OUT_OF_RANGE,
          + sqlE);

        There's no need to have the BaseJDBCTestCase., you can just say assertSQLState. That owuld match the typical
        use of static assert methods in JUnit tests.

        Since all of the messages you pass into assertSQLState are generic "SQLState was NOT correct", you could use the two argument
        value which has a similar generic message.

        assertSQLState(SQLStateConstants.DATA_EXCEPTION_NUMERIC_VALUE_OUT_OF_RANGE, sqlE);

        The three argument one can be used when there is added benefit to a different message, e.g. "ASIN Conversion return incorrect SQLState"
        In this case I would recommend the two argument version.

        • the patch contains an unrelated change to LangScripts.java
        Show
        Daniel John Debrunner added a comment - Thanks for the comprehensive test Susan (much better than the minimal testing I submitted with the initial changes), some comments: The apache licence header is the first item in a source file, before the package statement and imports, see other .java files for examples: In Java constants like SMALLEST_NEG_DERBY_DOUBLE should be declared final private static final double SMALLEST_NEG_DERBY_DOUBLE = -1.79769E+308; For the various constants it might be good to have comments around them indicting what they mean, for example it was unclear to me what was special about LITTLE_RADIANS that it needed a constant. Maybe a style issue, but I prefer not to have constants like: QUARTER_RADIANS, NEG_QUARTER_RADIANS, HALF_RADIANS, ZERO_RADIANS etc. Are they really adding value? Or would the code be clearer just saying double radians = 0.25; Constants are usually used to hide the specific value (e.g. 0.25) and provide a logical name for understanding, here the constant's name is defining the specific value (QUARTER_RADIANS) For PI instead of defining your own constant, PI_RADIANS, you can just use the one from StrictMath directly Similar, do the constants for 2*PI etc really add value? For the methods executeNullValues and executeNullFn would it make sense to move the check that the return is NULL into the method and not have all the callers check it? Then you could also check within the method that only one row is returned and that the wasNull() indicator matched the state of the value. You use static variables which will cause problems in the future when we want to run tests in parallel as there may be multiple threads sharing those varaibles as each runs an instance of this class. E.g. private static double rValue; Unless there's some special need I'm missing theie use seems to be self-contained within each test. The correct way to write this is to have local scope variables, not static fields, e.g. + for (int i = 0; i < testValues.length; i++) { + double expected = java.lang.StrictMath.tan(testValues[i]); <<<<<<< changed line + double rValue = executeValues("TAN", testValues[i]); <<<<<<< changed line + debug("TAN: input value: " + testValues[i] + " expected value: " + + expected + " return value: " + rValue); + assertEquals(expected, rValue, 0.0); + float fValue = executeFn("TAN", testValues[i]); <<<<<<< changed line + assertEquals(expected, fValue, 0.0); + } For checkng the SQL State values + BaseJDBCTestCase + .assertSQLState( + "SQLState was NOT correct", + SQLStateConstants.DATA_EXCEPTION_NUMERIC_VALUE_OUT_OF_RANGE, + sqlE); There's no need to have the BaseJDBCTestCase., you can just say assertSQLState. That owuld match the typical use of static assert methods in JUnit tests. Since all of the messages you pass into assertSQLState are generic "SQLState was NOT correct", you could use the two argument value which has a similar generic message. assertSQLState(SQLStateConstants.DATA_EXCEPTION_NUMERIC_VALUE_OUT_OF_RANGE, sqlE); The three argument one can be used when there is added benefit to a different message, e.g. "ASIN Conversion return incorrect SQLState" In this case I would recommend the two argument version. the patch contains an unrelated change to LangScripts.java

          People

          • Assignee:
            Susan Cline
            Reporter:
            Susan Cline
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development