[
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 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
> 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: 10.3.0.0
> 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] 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
--
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
