harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Nathan Beyer" <nbe...@kc.rr.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 04:26:53 GMT
> -----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));

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


Mime
View raw message