Hi Stuart,
On Aug 1, 2013, at 7:54 PM, Stuart Marks wrote:
> Looks good. Always a good idea to make sure that failures are reported
> properly. :-)
Indeed.
> It does look like the following cases are repeated, in both tests:
>
> {valueOf(Long.MIN_VALUE+1), valueOf(Long.MAX_VALUE), MINUS_ONE},
> {valueOf(Long.MIN_VALUE+1).negate(), valueOf(Long.MAX_VALUE), ZERO},
>
> Is this right?
Yes. I actually did not add any tests or test cases to BigDecimal, only fixed
the intended use of the ones present. What was happening is this. Each test
case was of the form
{a, b, expected}
where "expected" is the correct result of a.compareTo(b). Each test case was
used like this:
failures += compareToTest(a, b, expected);
failures += compareToTest(a.negate(), b, -1);
failures += compareToTest(a, b.negate(), 1);
failures += compareToTest(a.negate(), b.negate(), -expected);
which could instead be written as one test
failures += compareToTest(this, other, expected);
with four text cases
{a, b, expected},
{-a, b, -1},
{a, -b, 1),
{-a, -b, -expected}
which is patently incorrect for some of the test cases. What I did to fix this
was to remove the two middle tests for each test case and add a new test case
defined as
{-a, b, expected2}
Another way to state this is that the original test case becomes two test cases
{a, b, expected}
{-a, b, expected2}
and the four tests become two
failures += compareToTest(a, b, expected);
failures += compareToTest(a.negate(), b.negate(), -expected);
So the test cases and tests are really the same as before, but the expected
value is different in some instances as needed.
> The first four cases are (excuse the poor cross product notation):
>
> {max, max-1, min, min+1} x {1, -1} compareTo max
>
> but then there's simply
>
> {min} x {1, -1} compareTo min
>
> followed by the duplicate
>
> {min+1} x {1, -1} compareTo max
>
> Just following the pattern, I'd expect
>
> {max, max-1, min, min+1} x {1, -1} compareTo min
>
> though I'm not sure that all of these cases are necessary.
You are absolutely correct and I have updated the webrev accordingly:
http://cr.openjdk.java.net/~bpb/8022094/
Now only a Reviewer approval is needed.
Thanks,
Brian
