db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Daniel John Debrunner (JIRA)" <derby-...@db.apache.org>
Subject [jira] Commented: (DERBY-1802) JUnit tests for DERBY-475 and DERBY-592, Built-in and JDBC escape functions
Date Sat, 02 Sep 2006 17:11:23 GMT
    [ http://issues.apache.org/jira/browse/DERBY-1802?page=comments#action_12432294 ] 
Daniel John Debrunner commented on DERBY-1802:

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
   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:
     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
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",
+							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.


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

> JUnit tests for DERBY-475 and DERBY-592, Built-in and JDBC escape functions
> ---------------------------------------------------------------------------
>                 Key: DERBY-1802
>                 URL: http://issues.apache.org/jira/browse/DERBY-1802
>             Project: Derby
>          Issue Type: Test
>          Components: Test
>    Affects Versions:
>            Reporter: Susan Cline
>         Assigned To: Susan Cline
>            Priority: Minor
>         Attachments: DERBY-475_DERBY-592_20060831.diff
> 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] alpha - (1)
> ------------------------------------------------------
> ----------------- Locale Information -----------------
> Current Locale :  [English/United States [en_US]]
> Found support for locale: [de_DE]
>          version: alpha - (1)
> Found support for locale: [es]
>          version: alpha - (1)
> Found support for locale: [fr]
>          version: alpha - (1)
> Found support for locale: [it]
>          version: alpha - (1)
> Found support for locale: [ja_JP]
>          version: alpha - (1)
> Found support for locale: [ko_KR]
>          version: alpha - (1)
> Found support for locale: [pt_BR]
>          version: alpha - (1)
> Found support for locale: [zh_CN]
>          version: alpha - (1)
> Found support for locale: [zh_TW]
>          version: alpha - (1)
> ------------------------------------------------------
> I would appreciate it if a committer would commit this patch.
> Thanks,
> Susan

This message is automatically generated by JIRA.
If you think it was sent incorrectly contact one of the administrators: http://issues.apache.org/jira/secure/Administrators.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira


View raw message