harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Spark Shen <smallsmallor...@gmail.com>
Subject Re: svn commit: r442438 - in /incubator/harmony/enhanced/classlib/trunk/modules/luni/src/test/java/org/apache/harmony/luni/tests/java/lang: MathTest.java StrictMathTest.java
Date Wed, 13 Sep 2006 05:29:46 GMT
Nathan Beyer 写道:
>> -----Original Message-----
>> From: Spark Shen [mailto:smallsmallorgan@gmail.com]
>>     
>>> +        assertEquals("Should return -0.0", -0.0, Math.cbrt(-0.0), 0D);
>>>
>>>       
>> This assertion is not correct here. The spec of
>> java.lang.Math.cbrt(double) says:
>> 'If the argument is zero, then the result is a zero with the same sign
>> as the argument.'
>> But, assert this way, the sign of the result will be omitted.
>>
>> That is to say,
>> assertEquals("Should return -0.0", -0.0, Math.expm1(+0.0), 0D);
>> will also pass.
>>
>> This occurs in many other places in the commit, I suggest to eliminate
>> all the inappropriate delta.
>>     
>
> The problem isn't the delta, the problem is that these tests were ALL using
> boxing, which meant that assertion was based on Double.equals(Object). As
> such, this makes the tests confusing and unclear, especially with all of the
> assertions working this way. In this specific case, where you want to check
> the sign, you want to use an assertion that's similar to how Double.equals
> works - convert each double into bit representations and assert that all bit
> values are the same. But in cases where you want to assert a specific double
> value, say 3.0, you want to test the double, not the bit values.
>
> My personal opinion is that in test cases around primitives, there should be
> NO boxing/unboxing utilized. Make the tests explicit and clear. For example,
> to completely test this case where -0.0 is returned, I would suggest the
> following assertions:
>
> double actual = Math.cbrt(-0.0);
> // call assertEquals(long, long)
> assertEquals(Double.doubleToLongBits(-0.0),Double.doubleToLongBits(actual));
>   
Hi Nathan:

Agree. And I will refactor the test case and provide a patch soon.
> Compare that to this (which only compiles with Java 5).
>
> // call assertEquals(Object, Object)
> assertEquals(-0.0, Math.cbrt(-0.0));
>
> Isn't the former more obvious and straightforward?
>
>   
>> After looking into the source code JUnit:
>>
>> static public void assertEquals(String message, double expected, double
>> actual, double delta) {
>>         // handle infinity specially since subtracting to infinite
>> values gives NaN and the
>>         // the following test fails
>>         if (Double.isInfinite(expected)) {
>>             if (!(expected == actual))
>>                 failNotEquals(message, new Double(expected), new
>> Double(actual));
>>         } else if (!(Math.abs(expected-actual) <= delta)) // Because
>> comparison with NaN always returns false  (Spark comment: if condition
>> will make sign of zero meaningless)
>>             failNotEquals(message, new Double(expected), new
>> Double(actual));
>>     }
>> This may be a bug of JUnit. Since it does not differentiate +/-0.
>>     
>
> If this is a bug, it's too late now. JUnit has been this way for years.
>
> -Nathan
>
>
> ---------------------------------------------------------------------
> Terms of use : http://incubator.apache.org/harmony/mailing.html
> To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> For additional commands, e-mail: harmony-dev-help@incubator.apache.org
>
>
>   


-- 
Spark Shen
China Software Development Lab, IBM


---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org


Mime
View raw message